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

PATCH In review Yasuaki Ishimatsu 2 participants 7 messages Sep 2, 2014 – Sep 3, 2014

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Yasuaki Ishimatsu

author

2014-09-02 02:02 UTC #0 lore raw

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?

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

Rafael J. Wysocki

2014-09-02 21:10 UTC #1 lore raw

Quoted 1 line · Yasuaki Ishimatsu

Hi Rafael,

Hi,

Quoted 2 lines

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.

Quoted 9 lines

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?

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Signature · 2 lines

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

Yasuaki Ishimatsu

author

2014-09-03 02:15 UTC #2 lore raw

Hi Rafael,

Thank you for your explanation.

Quoted 16 lines · Rafael J. Wysocki

Quoted 1 line · Yasuaki Ishimatsu
Hi Rafael,
Hi,
Quoted 2 lines
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.
Quoted 5 lines
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 8 lines

Quoted 3 lines
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?

Quoted 8 lines

Quoted 5 lines
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.

Quoted 8 lines

Quoted 2 lines
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

Quoted 10 lines

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

Yasuaki Ishimatsu

author

2014-09-03 04:40 UTC #3 lore raw

PATCH 1/1 acpi : not caching sun value in acpi_device_pnp

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 <[email protected]>

---
 drivers/acpi/scan.c     | 15 ++++++++-------
 include/acpi/acpi_bus.h |  1 -
 2 files changed, 8 insertions(+), 8 deletions(-)

drivers/acpi/scan.c +8 -7

index 9a92989..3bf7764 100644
@@ -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")) {

include/acpi/acpi_bus.h +0 -1

index bcfd808..c1c9de1 100644
@@ -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)

Rafael J. Wysocki

2014-09-03 21:21 UTC #4 lore raw

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

Quoted 9 lines · 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.

Quoted 6 lines

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.

Quoted 69 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
  drivers/acpi/scan.c     | 15 ++++++++-------
  include/acpi/acpi_bus.h |  1 -
  2 files changed, 8 insertions(+), 8 deletions(-)

drivers/acpi/scan.c +8 -7

index 9a92989..3bf7764 100644
@@ -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")) {

include/acpi/acpi_bus.h +0 -2

index bcfd808..c1c9de1 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Rafael J. Wysocki

2014-09-03 21:26 UTC #6 lore raw

In reply to Yasuaki Ishimatsu

Quoted 1 line · Yasuaki Ishimatsu

Hi Rafael,

Hi,

Quoted 24 lines

Thank you for your explanation.
Quoted 21 lines · Rafael J. Wysocki
Quoted 1 line · Yasuaki Ishimatsu
Hi Rafael,
Hi,
Quoted 2 lines
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.
Quoted 9 lines
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.

Quoted 4 lines

Quoted 1 line
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.

Quoted 15 lines

Quoted 8 lines
Quoted 3 lines
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."

Quoted 7 lines

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

Yasuaki Ishimatsu

author

2014-09-03 23:46 UTC #5 lore raw

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

In reply to Rafael J. Wysocki

Quoted 15 lines · Rafael J. Wysocki

Quoted 14 lines · 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.

Quoted 7 lines

Quoted 1 line
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

Quoted 71 lines

Quoted 70 lines

Signed-off-by Yasuaki Ishimatsu <[email protected]>

---
   drivers/acpi/scan.c     | 15 ++++++++-------
   include/acpi/acpi_bus.h |  1 -
   2 files changed, 8 insertions(+), 8 deletions(-)

drivers/acpi/scan.c +8 -7

index 9a92989..3bf7764 100644
@@ -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")) {

include/acpi/acpi_bus.h +0 -2

index bcfd808..c1c9de1 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html