[PATCH] ACPI: Do not fail acpi_bind_one() if device is already bound correctly

[PATCH] ACPI: Do not fail acpi_bind_one() if device is already bound correctly

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---
 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);
 

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

[Lan Tianyu]

2013/8/2 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>  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?

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards
Tianyu Lan

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

[Rafael J. Wysocki]

On Friday, August 02, 2013 10:48:49 AM Lan Tianyu wrote:
> 2013/8/2 Rafael J. Wysocki <rjw@sisk.pl>:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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.]

Do you have any problems with my $subject patch?

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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Lan Tianyu]

2013/8/2 Rafael J. Wysocki <rjw@sisk.pl>:
> On Friday, August 02, 2013 10:48:49 AM Lan Tianyu wrote:
>> 2013/8/2 Rafael J. Wysocki <rjw@sisk.pl>:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > 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@intel.com>
>> > ---
>> >  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@intel.com>

>
> 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@vger.kernel.org
>> > 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

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

[Toshi Kani]

On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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.

While it seems reasonable to allow such case, I do not think we will hit
this case under the normal scenarios.  So, I do not think we need to
make this change now unless it actually solves Yasuaki's issue (which I
am guessing not).

Thanks,
-Toshi


> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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);
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rafael J. Wysocki]

On Friday, August 02, 2013 04:38:38 PM Toshi Kani wrote:
> On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > 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.
> 
> While it seems reasonable to allow such case, I do not think we will hit
> this case under the normal scenarios.  So, I do not think we need to
> make this change now unless it actually solves Yasuaki's issue (which I
> am guessing not).

In theory it should be possible to call acpi_bind_one() twice in a row
for the same dev and the same handle without failure, that simply is
logical.  The patch may not fix any problems visible now, but returning an
error code in such a case is simply incorrect.

Thanks,
Rafael


> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  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);
> >  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Toshi Kani]

On Sat, 2013-08-03 at 02:47 +0200, Rafael J. Wysocki wrote:
> On Friday, August 02, 2013 04:38:38 PM Toshi Kani wrote:
> > On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > 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.
> > 
> > While it seems reasonable to allow such case, I do not think we will hit
> > this case under the normal scenarios.  So, I do not think we need to
> > make this change now unless it actually solves Yasuaki's issue (which I
> > am guessing not).
> 
> In theory it should be possible to call acpi_bind_one() twice in a row
> for the same dev and the same handle without failure, that simply is
> logical.  The patch may not fix any problems visible now, but returning an
> error code in such a case is simply incorrect.

We changed acpi_bus_device_attach() to not call the handler or driver
again if it is already bound.  So, I was under impression that we
prevent from attaching a same device twice.  But I may be missing
something...

Thanks,
-Toshi

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

[Rafael J. Wysocki]

On Saturday, August 03, 2013 06:32:02 PM Toshi Kani wrote:
> On Sat, 2013-08-03 at 02:47 +0200, Rafael J. Wysocki wrote:
> > On Friday, August 02, 2013 04:38:38 PM Toshi Kani wrote:
> > > On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > 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.
> > > 
> > > While it seems reasonable to allow such case, I do not think we will hit
> > > this case under the normal scenarios.  So, I do not think we need to
> > > make this change now unless it actually solves Yasuaki's issue (which I
> > > am guessing not).
> > 
> > In theory it should be possible to call acpi_bind_one() twice in a row
> > for the same dev and the same handle without failure, that simply is
> > logical.  The patch may not fix any problems visible now, but returning an
> > error code in such a case is simply incorrect.
> 
> We changed acpi_bus_device_attach() to not call the handler or driver
> again if it is already bound.  So, I was under impression that we
> prevent from attaching a same device twice.  But I may be missing
> something...

acpi_bind_one() may be called in code paths that don't start from
acpi_bus_device_attach(), like acpi_platform_notify(), where the result
depends on how .find_device() is implemented by the the given bus type,
for example.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Toshi Kani]

On Sun, 2013-08-04 at 16:03 +0200, Rafael J. Wysocki wrote:
> On Saturday, August 03, 2013 06:32:02 PM Toshi Kani wrote:
> > On Sat, 2013-08-03 at 02:47 +0200, Rafael J. Wysocki wrote:
> > > On Friday, August 02, 2013 04:38:38 PM Toshi Kani wrote:
> > > > On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > 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.
> > > > 
> > > > While it seems reasonable to allow such case, I do not think we will hit
> > > > this case under the normal scenarios.  So, I do not think we need to
> > > > make this change now unless it actually solves Yasuaki's issue (which I
> > > > am guessing not).
> > > 
> > > In theory it should be possible to call acpi_bind_one() twice in a row
> > > for the same dev and the same handle without failure, that simply is
> > > logical.  The patch may not fix any problems visible now, but returning an
> > > error code in such a case is simply incorrect.
> > 
> > We changed acpi_bus_device_attach() to not call the handler or driver
> > again if it is already bound.  So, I was under impression that we
> > prevent from attaching a same device twice.  But I may be missing
> > something...
> 
> acpi_bind_one() may be called in code paths that don't start from
> acpi_bus_device_attach(), like acpi_platform_notify(), where the result
> depends on how .find_device() is implemented by the the given bus type,
> for example.

acpi_bind_one() always returns with 0 when it sets a handle to the
device.  So, acpi_platform_notify() should not call .find_device() in
this case.  acpi_bus_check_add() is also protected from adding a same
device twice.  But I see your point that the callers of acpi_bind_one()
could be changed / implementation dependent.  So, I agree that it would
be prudent to have this change. 

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

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

[Rafael J. Wysocki]

On Monday, August 05, 2013 04:20:19 PM Toshi Kani wrote:
> On Sun, 2013-08-04 at 16:03 +0200, Rafael J. Wysocki wrote:
> > On Saturday, August 03, 2013 06:32:02 PM Toshi Kani wrote:
> > > On Sat, 2013-08-03 at 02:47 +0200, Rafael J. Wysocki wrote:
> > > > On Friday, August 02, 2013 04:38:38 PM Toshi Kani wrote:
> > > > > On Fri, 2013-08-02 at 00:33 +0200, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > While it seems reasonable to allow such case, I do not think we will hit
> > > > > this case under the normal scenarios.  So, I do not think we need to
> > > > > make this change now unless it actually solves Yasuaki's issue (which I
> > > > > am guessing not).
> > > > 
> > > > In theory it should be possible to call acpi_bind_one() twice in a row
> > > > for the same dev and the same handle without failure, that simply is
> > > > logical.  The patch may not fix any problems visible now, but returning an
> > > > error code in such a case is simply incorrect.
> > > 
> > > We changed acpi_bus_device_attach() to not call the handler or driver
> > > again if it is already bound.  So, I was under impression that we
> > > prevent from attaching a same device twice.  But I may be missing
> > > something...
> > 
> > acpi_bind_one() may be called in code paths that don't start from
> > acpi_bus_device_attach(), like acpi_platform_notify(), where the result
> > depends on how .find_device() is implemented by the the given bus type,
> > for example.
> 
> acpi_bind_one() always returns with 0 when it sets a handle to the
> device.  So, acpi_platform_notify() should not call .find_device() in
> this case.  acpi_bus_check_add() is also protected from adding a same
> device twice.  But I see your point that the callers of acpi_bind_one()
> could be changed / implementation dependent.  So, I agree that it would
> be prudent to have this change. 
> 
> Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks!

I wonder what you think about this patch:

https://patchwork.kernel.org/patch/2838675/

Rafael