Question of " 202317a : ACPI / scan: Add acpi_device objects for all device nodes in the namespace" commit

Question of " 202317a : ACPI / scan: Add acpi_device objects for all device nodes in the namespace" commit

[Yasuaki Ishimatsu]

Hi Rafael,

Why is _SUN method decoded and the sun sysfs created even if _STA reports
that the device is not present and not functional?

By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
device nodes in the namespace", all device nodes in the namespace are
shown under /sys/bus/acpi/devices direcotry even if the devices are not
present and not functional.

But ACPI specification says against return value information of _STA:
A device can only decode its hardware resources if both bits 0 and 1 are set.
If the device is not present (bit 0 cleared) or not enabled (bit 1 cleared),
then the device must not decode its resources.

In my understanding, when the device is not present and not functional,
_SUN of the device must not be decoded. Is my understanding worng?

In our box, when the device is not present and not functional, _SUN method
of the device returns wrong value. And the values is cached to
acpi_device->pnp.sun. So when hot adding the device and changing _SUN value,
the information is not reflected to acpi_device->pnp.sun.

I think the issue is solved by not caching sun value. But at first, I want
to know whether current implementation is correct or not.

Thanks,
Yasuaki Ishimatsu

Re: Question of " 202317a : ACPI / scan: Add acpi_device objects for all device nodes in the namespace" commit

[Rafael J. Wysocki]

On Tuesday, September 02, 2014 10:53:09 AM Yasuaki Ishimatsu wrote:
> Hi Rafael,

Hi,

> Why is _SUN method decoded and the sun sysfs created even if _STA reports
> that the device is not present and not functional?

sun sysfs is created for stuff that has _SUN.  It should be empty for devices
that aren't present, though, apparently.

> By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
> device nodes in the namespace", all device nodes in the namespace are
> shown under /sys/bus/acpi/devices direcotry even if the devices are not
> present and not functional.
> 
> But ACPI specification says against return value information of _STA:
> A device can only decode its hardware resources if both bits 0 and 1 are set.
> If the device is not present (bit 0 cleared) or not enabled (bit 1 cleared),
> then the device must not decode its resources.

But _SUN is not about hardware resources, is it?

> In my understanding, when the device is not present and not functional,
> _SUN of the device must not be decoded. Is my understanding worng?

If my understanding of the _INI section of the spec is correct, you have a
point, but the dependency is rather indirect as far as I can say.  If the
device is not present, we are not supposed to execute _INI for it and _SUN
can only be executed after _INI.

> In our box, when the device is not present and not functional, _SUN method
> of the device returns wrong value. And the values is cached to
> acpi_device->pnp.sun. So when hot adding the device and changing _SUN value,
> the information is not reflected to acpi_device->pnp.sun.

I see.

> I think the issue is solved by not caching sun value. But at first, I want
> to know whether current implementation is correct or not.

No, it isn't, but there are two ways to fix it.

A short term fix would be to evaluate _STA and print an empty string in
acpi_device_sun_show() if we don't like the return value.  If we do like it,
evaluate _SUN and print the result.

A long term fix would require us to track the execution of _INI and decide
what to print on this basis (and probably avoid to execute some other methods
too if _INI has not been executed).  I'll try to do something along these lines,
but I can't promise any particular time frame, sorry about that.

KR,
Rafael


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

Re: Question of " 202317a : ACPI / scan: Add acpi_device objects for all device nodes in the namespace" commit

[Yasuaki Ishimatsu]

Hi Rafael,

Thank you for your explanation.

(2014/09/03 6:29), Rafael J. Wysocki wrote:
> On Tuesday, September 02, 2014 10:53:09 AM Yasuaki Ishimatsu wrote:
>> Hi Rafael,
>
> Hi,
>
>> Why is _SUN method decoded and the sun sysfs created even if _STA reports
>> that the device is not present and not functional?
>
> sun sysfs is created for stuff that has _SUN.  It should be empty for devices
> that aren't present, though, apparently.
>
>> By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
>> device nodes in the namespace", all device nodes in the namespace are
>> shown under /sys/bus/acpi/devices direcotry even if the devices are not
>> present and not functional.
>>
>> But ACPI specification says against return value information of _STA:
>> A device can only decode its hardware resources if both bits 0 and 1 are set.
>> If the device is not present (bit 0 cleared) or not enabled (bit 1 cleared),
>> then the device must not decode its resources.
>
> But _SUN is not about hardware resources, is it?

I think so. But ACPI spec does not define what the hardware resources are.
So someone says _SUN is about hardware resrouces.

>
>> In my understanding, when the device is not present and not functional,
>> _SUN of the device must not be decoded. Is my understanding worng?
>
> If my understanding of the _INI section of the spec is correct, you have a
> point, but the dependency is rather indirect as far as I can say.  If the
> device is not present, we are not supposed to execute _INI for it and _SUN
> can only be executed after _INI.

I know the spec. According to the _INI section of the spec, it says:

"If the _STA method indicates that the device is not present and is not
functional, OSPM will not run the _INI and will not examine the children
of the device for _INI methods."

In my understanding, if a device has both _SUN and _INI method, and _STA
method indicates that the deviceis not present and is not functional,
_SUN method cannot be executed.

If a device has _SUN method and does not have _INI method, and _STA
method indicates that the deviceis not present and is not functional,
can we execute _SUN method?

>
>> In our box, when the device is not present and not functional, _SUN method
>> of the device returns wrong value. And the values is cached to
>> acpi_device->pnp.sun. So when hot adding the device and changing _SUN value,
>> the information is not reflected to acpi_device->pnp.sun.
>
> I see.
>
>> I think the issue is solved by not caching sun value. But at first, I want
>> to know whether current implementation is correct or not.
>
> No, it isn't, but there are two ways to fix it.
>
> A short term fix would be to evaluate _STA and print an empty string in
> acpi_device_sun_show() if we don't like the return value.  If we do like it,
> evaluate _SUN and print the result.

I will create a shot term fix soon.

Thanks,
Yasuaki Ishimatsu

>
> A long term fix would require us to track the execution of _INI and decide
> what to print on this basis (and probably avoid to execute some other methods
> too if _INI has not been executed).  I'll try to do something along these lines,
> but I can't promise any particular time frame, sorry about that.
>
> KR,
> Rafael
>
>

[PATCH] acpi : not caching sun value in acpi_device_pnp

[Yasuaki Ishimatsu]

By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
device nodes in the namespace", all device nodes in the namespace are
shown under /sys/bus/acpi/devices directory even if the devices are not
present and not functional. And if a device node has _SUN method, it is
decoded and the sun value is cached to acpi_device_pnp->sun.

But when device is not present and not functional, some firmware returns
wrong sun value. And the wrong value is cached to acpi_device_pnp->sun.

Therefore, even if a device is hot added and firmware returns correct
sun value of the device, the value is not reflected to
acpi_device_pnp->sun. So user cannot know correct sun value of the
device.

By not caching sun value, the patch fixes the issue.

Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
  drivers/acpi/scan.c     | 15 ++++++++-------
  include/acpi/acpi_bus.h |  1 -
  2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 9a92989..3bf7764 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -667,8 +667,14 @@ static ssize_t
  acpi_device_sun_show(struct device *dev, struct device_attribute *attr,
  		     char *buf) {
  	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	acpi_status status;
+	unsigned long long sun;
+
+	status = acpi_evaluate_integer(acpi_dev->handle, "_SUN", NULL, &sun);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;

-	return sprintf(buf, "%lu\n", acpi_dev->pnp.sun);
+	return sprintf(buf, "%llu\n", sun);
  }
  static DEVICE_ATTR(sun, 0444, acpi_device_sun_show, NULL);

@@ -690,7 +696,6 @@ static int acpi_device_setup_files(struct acpi_device *dev)
  {
  	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
  	acpi_status status;
-	unsigned long long sun;
  	int result = 0;

  	/*
@@ -731,14 +736,10 @@ static int acpi_device_setup_files(struct acpi_device *dev)
  	if (dev->pnp.unique_id)
  		result = device_create_file(&dev->dev, &dev_attr_uid);

-	status = acpi_evaluate_integer(dev->handle, "_SUN", NULL, &sun);
-	if (ACPI_SUCCESS(status)) {
-		dev->pnp.sun = (unsigned long)sun;
+	if (acpi_has_method(dev->handle, "_SUN")) {
  		result = device_create_file(&dev->dev, &dev_attr_sun);
  		if (result)
  			goto end;
-	} else {
-		dev->pnp.sun = (unsigned long)-1;
  	}

  	if (acpi_has_method(dev->handle, "_STA")) {
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index bcfd808..c1c9de1 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -246,7 +246,6 @@ struct acpi_device_pnp {
  	acpi_device_name device_name;	/* Driver-determined */
  	acpi_device_class device_class;	/*        "          */
  	union acpi_object *str_obj;	/* unicode string for _STR method */
-	unsigned long sun;		/* _SUN */
  };

  #define acpi_device_bid(d)	((d)->pnp.bus_id)

Re: [PATCH] acpi : not caching sun value in acpi_device_pnp

[Rafael J. Wysocki]

On Wednesday, September 03, 2014 01:39:13 PM Yasuaki Ishimatsu wrote:
> By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
> device nodes in the namespace", all device nodes in the namespace are
> shown under /sys/bus/acpi/devices directory even if the devices are not
> present and not functional. And if a device node has _SUN method, it is
> decoded and the sun value is cached to acpi_device_pnp->sun.
> 
> But when device is not present and not functional, some firmware returns
> wrong sun value. And the wrong value is cached to acpi_device_pnp->sun.
> 
> Therefore, even if a device is hot added and firmware returns correct
> sun value of the device, the value is not reflected to
> acpi_device_pnp->sun. So user cannot know correct sun value of the
> device.
> 
> By not caching sun value, the patch fixes the issue.

Queued up for 3.17-rc4, but with a different changelog, because in my opinion
the main problem is that _SUN is not guaranteed to return the same value
every time it is evaluated, so either it should not be cached, or we should
update the cached value whenever the device status changes.

> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/scan.c     | 15 ++++++++-------
>   include/acpi/acpi_bus.h |  1 -
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 9a92989..3bf7764 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -667,8 +667,14 @@ static ssize_t
>   acpi_device_sun_show(struct device *dev, struct device_attribute *attr,
>   		     char *buf) {
>   	struct acpi_device *acpi_dev = to_acpi_device(dev);
> +	acpi_status status;
> +	unsigned long long sun;
> +
> +	status = acpi_evaluate_integer(acpi_dev->handle, "_SUN", NULL, &sun);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> 
> -	return sprintf(buf, "%lu\n", acpi_dev->pnp.sun);
> +	return sprintf(buf, "%llu\n", sun);
>   }
>   static DEVICE_ATTR(sun, 0444, acpi_device_sun_show, NULL);
> 
> @@ -690,7 +696,6 @@ static int acpi_device_setup_files(struct acpi_device *dev)
>   {
>   	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
>   	acpi_status status;
> -	unsigned long long sun;
>   	int result = 0;
> 
>   	/*
> @@ -731,14 +736,10 @@ static int acpi_device_setup_files(struct acpi_device *dev)
>   	if (dev->pnp.unique_id)
>   		result = device_create_file(&dev->dev, &dev_attr_uid);
> 
> -	status = acpi_evaluate_integer(dev->handle, "_SUN", NULL, &sun);
> -	if (ACPI_SUCCESS(status)) {
> -		dev->pnp.sun = (unsigned long)sun;
> +	if (acpi_has_method(dev->handle, "_SUN")) {
>   		result = device_create_file(&dev->dev, &dev_attr_sun);
>   		if (result)
>   			goto end;
> -	} else {
> -		dev->pnp.sun = (unsigned long)-1;
>   	}
> 
>   	if (acpi_has_method(dev->handle, "_STA")) {
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index bcfd808..c1c9de1 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -246,7 +246,6 @@ struct acpi_device_pnp {
>   	acpi_device_name device_name;	/* Driver-determined */
>   	acpi_device_class device_class;	/*        "          */
>   	union acpi_object *str_obj;	/* unicode string for _STR method */
> -	unsigned long sun;		/* _SUN */
>   };
> 
>   #define acpi_device_bid(d)	((d)->pnp.bus_id)
> 
> 
> --
> 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 : not caching sun value in acpi_device_pnp

[Yasuaki Ishimatsu]

(2014/09/04 6:41), Rafael J. Wysocki wrote:
> On Wednesday, September 03, 2014 01:39:13 PM Yasuaki Ishimatsu wrote:
>> By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
>> device nodes in the namespace", all device nodes in the namespace are
>> shown under /sys/bus/acpi/devices directory even if the devices are not
>> present and not functional. And if a device node has _SUN method, it is
>> decoded and the sun value is cached to acpi_device_pnp->sun.
>>
>> But when device is not present and not functional, some firmware returns
>> wrong sun value. And the wrong value is cached to acpi_device_pnp->sun.
>>
>> Therefore, even if a device is hot added and firmware returns correct
>> sun value of the device, the value is not reflected to
>> acpi_device_pnp->sun. So user cannot know correct sun value of the
>> device.
>>
>> By not caching sun value, the patch fixes the issue.
>

> Queued up for 3.17-rc4, but with a different changelog, because in my opinion
> the main problem is that _SUN is not guaranteed to return the same value
> every time it is evaluated, so either it should not be cached, or we should
> update the cached value whenever the device status changes.

I see. I have no objection to changing the changelog.
Please change it.

Thanks,
Yasuaki Ishimatsu
>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>    drivers/acpi/scan.c     | 15 ++++++++-------
>>    include/acpi/acpi_bus.h |  1 -
>>    2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 9a92989..3bf7764 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -667,8 +667,14 @@ static ssize_t
>>    acpi_device_sun_show(struct device *dev, struct device_attribute *attr,
>>    		     char *buf) {
>>    	struct acpi_device *acpi_dev = to_acpi_device(dev);
>> +	acpi_status status;
>> +	unsigned long long sun;
>> +
>> +	status = acpi_evaluate_integer(acpi_dev->handle, "_SUN", NULL, &sun);
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>>
>> -	return sprintf(buf, "%lu\n", acpi_dev->pnp.sun);
>> +	return sprintf(buf, "%llu\n", sun);
>>    }
>>    static DEVICE_ATTR(sun, 0444, acpi_device_sun_show, NULL);
>>
>> @@ -690,7 +696,6 @@ static int acpi_device_setup_files(struct acpi_device *dev)
>>    {
>>    	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
>>    	acpi_status status;
>> -	unsigned long long sun;
>>    	int result = 0;
>>
>>    	/*
>> @@ -731,14 +736,10 @@ static int acpi_device_setup_files(struct acpi_device *dev)
>>    	if (dev->pnp.unique_id)
>>    		result = device_create_file(&dev->dev, &dev_attr_uid);
>>
>> -	status = acpi_evaluate_integer(dev->handle, "_SUN", NULL, &sun);
>> -	if (ACPI_SUCCESS(status)) {
>> -		dev->pnp.sun = (unsigned long)sun;
>> +	if (acpi_has_method(dev->handle, "_SUN")) {
>>    		result = device_create_file(&dev->dev, &dev_attr_sun);
>>    		if (result)
>>    			goto end;
>> -	} else {
>> -		dev->pnp.sun = (unsigned long)-1;
>>    	}
>>
>>    	if (acpi_has_method(dev->handle, "_STA")) {
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index bcfd808..c1c9de1 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -246,7 +246,6 @@ struct acpi_device_pnp {
>>    	acpi_device_name device_name;	/* Driver-determined */
>>    	acpi_device_class device_class;	/*        "          */
>>    	union acpi_object *str_obj;	/* unicode string for _STR method */
>> -	unsigned long sun;		/* _SUN */
>>    };
>>
>>    #define acpi_device_bid(d)	((d)->pnp.bus_id)
>>
>>
>> --
>> 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: Question of " 202317a : ACPI / scan: Add acpi_device objects for all device nodes in the namespace" commit

[Rafael J. Wysocki]

On Wednesday, September 03, 2014 11:14:02 AM Yasuaki Ishimatsu wrote:
> Hi Rafael,

Hi,

> Thank you for your explanation.
> 
> (2014/09/03 6:29), Rafael J. Wysocki wrote:
> > On Tuesday, September 02, 2014 10:53:09 AM Yasuaki Ishimatsu wrote:
> >> Hi Rafael,
> >
> > Hi,
> >
> >> Why is _SUN method decoded and the sun sysfs created even if _STA reports
> >> that the device is not present and not functional?
> >
> > sun sysfs is created for stuff that has _SUN.  It should be empty for devices
> > that aren't present, though, apparently.
> >
> >> By this commit, "202317a : ACPI / scan: Add acpi_device objects for all
> >> device nodes in the namespace", all device nodes in the namespace are
> >> shown under /sys/bus/acpi/devices direcotry even if the devices are not
> >> present and not functional.
> >>
> >> But ACPI specification says against return value information of _STA:
> >> A device can only decode its hardware resources if both bits 0 and 1 are set.
> >> If the device is not present (bit 0 cleared) or not enabled (bit 1 cleared),
> >> then the device must not decode its resources.
> >
> > But _SUN is not about hardware resources, is it?
> 
> I think so. But ACPI spec does not define what the hardware resources are.
> So someone says _SUN is about hardware resrouces.

Well, that is a very far fetched interpretation that I can't see any kind of
support for in the spec or elsewhere.

Nevertheless, it is not required that the return value of _SUN be the same
every time it is evaluated, so caching it is questionable.

> >
> >> In my understanding, when the device is not present and not functional,
> >> _SUN of the device must not be decoded. Is my understanding worng?
> >
> > If my understanding of the _INI section of the spec is correct, you have a
> > point, but the dependency is rather indirect as far as I can say.  If the
> > device is not present, we are not supposed to execute _INI for it and _SUN
> > can only be executed after _INI.
> 
> I know the spec. According to the _INI section of the spec, it says:
> 
> "If the _STA method indicates that the device is not present and is not
> functional, OSPM will not run the _INI and will not examine the children
> of the device for _INI methods."
> 
> In my understanding, if a device has both _SUN and _INI method, and _STA
> method indicates that the deviceis not present and is not functional,
> _SUN method cannot be executed.
> 
> If a device has _SUN method and does not have _INI method, and _STA
> method indicates that the deviceis not present and is not functional,
> can we execute _SUN method?

I'd think so.  I don't see anything preventing us from doing that in the spec.

Rafael