Re: [PATCH] ACPI: Do not fail acpi_bind_one() if device is alreadybound correctly

[Lan Tianyu]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/