2013/8/2 Rafael J. Wysocki <rjw@xxxxxxx>:
> On Friday, August 02, 2013 10:48:49 AM Lan Tianyu wrote:
>> 2013/8/2 Rafael J. Wysocki <rjw@xxxxxxx>:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> >
>> > Modify acpi_bind_one() so that it doesn't fail if the device
>> > represented by its first argument has already been bound to the
>> > given ACPI handle (second argument), because that is not a good
>> > enough reason for returning an error code.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> > ---
>> > drivers/acpi/glue.c | 15 +++++++++++----
>> > 1 file changed, 11 insertions(+), 4 deletions(-)
>> >
>> > Index: linux-pm/drivers/acpi/glue.c
>> > ===================================================================
>> > --- linux-pm.orig/drivers/acpi/glue.c
>> > +++ linux-pm/drivers/acpi/glue.c
>> > @@ -143,7 +143,10 @@ int acpi_bind_one(struct device *dev, ac
>> > list_for_each_entry(pn, &acpi_dev->physical_node_list, node)
>> > if (pn->dev == dev) {
>> > dev_warn(dev, "Already associated with ACPI node\n");
>> > - goto err_free;
>> > + if (ACPI_HANDLE(dev) == handle)
>> > + retval = 0;
>> > +
>> > + goto out_free;
>> > }
>> >
>> > /* allocate physical node id according to physical_node_id_bitmap */
>> > @@ -152,7 +155,7 @@ int acpi_bind_one(struct device *dev, ac
>> > ACPI_MAX_PHYSICAL_NODE);
>> > if (physical_node->node_id >= ACPI_MAX_PHYSICAL_NODE) {
>> > retval = -ENOSPC;
>> > - goto err_free;
>> > + goto out_free;
>> > }
>> >
>> > set_bit(physical_node->node_id, acpi_dev->physical_node_id_bitmap);
>> > @@ -185,10 +188,14 @@ int acpi_bind_one(struct device *dev, ac
>> > put_device(dev);
>> > return retval;
>> >
>> > - err_free:
>> > + out_free:
>> > mutex_unlock(&acpi_dev->physical_node_lock);
>> > kfree(physical_node);
>> > - goto err;
>> > + if (retval)
>> > + goto err;
>> > +
>> > + put_device(dev);
>> > + return 0;
>> > }
>> > EXPORT_SYMBOL_GPL(acpi_bind_one);
>>
>> Hi Rafael:
>> How about the following change?
>
> It is incorrect, because the "problem" it attempts to "fix" is actually
> intentional behavior. [And you could ask if it was intentional in the first
> place instead of assuming that it was a mistake. It wasn't.]
>
Ok.
> Do you have any problems with my $subject patch?
No, I have no problem now.
Reviewed-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
>
> Rafael
>
>
>>
>> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
>> index f70cc45..35f375e 100644
>> --- a/drivers/acpi/glue.c
>> +++ b/drivers/acpi/glue.c
>> @@ -183,19 +183,15 @@ int acpi_bind_one(struct device *dev, acpi_handle handle)
>>
>> return 0;
>>
>> - err:
>> - ACPI_HANDLE_SET(dev, NULL);
>> - put_device(dev);
>> - return retval;
>> -
>> out_free:
>> mutex_unlock(&acpi_dev->physical_node_lock);
>> kfree(physical_node);
>> - if (retval)
>> - goto err;
>>
>> + err:
>> + if (retval)
>> + ACPI_HANDLE_SET(dev, NULL);
>> put_device(dev);
>> - return 0;
>> + return retval;
>> }
>> EXPORT_SYMBOL_GPL(acpi_bind_one);
>> ---------------
>>
>> When I reviewed this patch, found the dev's acpi handle always
>> is set to NULL if there is error. This seems make no sense for
>> the case that the handle has been set to dev before binding.
>>
>> For this case, the acpi handle has been found before binding.
>> Actually, the device driver could control any resources under ACPI
>> node even if the binding failed. So adding one flag to differentiate
>> it.
>>
>> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
>> index 35f375e..c868e51 100644
>> --- a/drivers/acpi/glue.c
>> +++ b/drivers/acpi/glue.c
>> @@ -113,6 +113,7 @@ int acpi_bind_one(struct device *dev, acpi_handle handle)
>> acpi_status status;
>> struct acpi_device_physical_node *physical_node, *pn;
>> char physical_node_name[sizeof(PHYSICAL_NODE_STRING) + 2];
>> + bool has_handle = false;
>> int retval = -EINVAL;
>>
>> if (ACPI_HANDLE(dev)) {
>> @@ -121,6 +122,7 @@ int acpi_bind_one(struct device *dev, acpi_handle handle)
>> return -EINVAL;
>> } else {
>> handle = ACPI_HANDLE(dev);
>> + has_handle = true;
>> }
>> }
>> if (!handle)
>> @@ -188,7 +190,7 @@ int acpi_bind_one(struct device *dev, acpi_handle handle)
>> kfree(physical_node);
>>
>> err:
>> - if (retval)
>> + if (retval && !has_handle)
>> ACPI_HANDLE_SET(dev, NULL);
>> put_device(dev);
>> return retval;
>>
>>
>>
>> >
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> > the body of a message to majordomo@xxxxxxxxxxxxxxx
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
