Skip to contentSkip to conversation

PNPACPI: Enable PNPACPI _PSx Support, v3

PATCH Applied Witold Szczeponik 5 participants 9 messages Mar 30, 2009 – Jul 28, 2009 1 message not in the archive

Subsystems (2)
ACPI Pnp support
lore
Share & tools
lore

Witold Szczeponik

author
#0 lore raw

PATCH 1/1 PNPACPI: Enable PNPACPI _PSx Support, v3

Subject: Enable PNPACPI _PSx Support, v3

(This is an update to the patch presented earlier in
http://lkml.org/lkml/2008/12/8/284, with new error handling.)

This patch sets the power of PnP ACPI devices to D0 when they are activated and to D3 when they are disabled. The latter is in correspondence with the ACPI 3.0 specification, whereas the former is added in order to be able to power up a device after it has been previously disabled (or when booting up a system). (As a consequence, the patch makes the PnP ACPI code more ACPI compliant.)

Section 6.2.2 of the ACPI Specification (at least versions 1.0b and 3.0a) states: "Prior to running this control method [_DIS], the OS[PM] will have already put the device in the D3 state." Unfortunately, there is no clear statement as to when to put a device in the D0 state. :-( Therefore, the patch executes the method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the device is disabled, _SRS enables the device at the specified resources." (From the ACPI 3.0a Specification.)

The patch fixes a problem with some IBM ThinkPads (at least the 600E and the 600X) where the serial ports have a dedicated power source that needs to be brought up before the serial port can be used. Without this patch, the serial port is enabled but has no power. (In the past, the tpctl utility had to be utilized to turn on the power, but support for this feature stopped with version 5.9 as it did not support the more recent kernel versions.)

The error handlers that handle any errors that can occur during the power up/power down phases return the error codes to the caller directly. Comments welcome! :-)

No regressions were observed on hardware that does not require this patch.

The patch is applied against 2.6.27.x.

Signed-off-by Witold Szczeponik <[email protected]>
drivers/pnp/pnpacpi/core.c +15 -7
Index: linux/drivers/pnp/pnpacpi/core.c
===================================================================
@@ -84,7 +84,6 @@ static int pnpacpi_set_resources(struct
 	acpi_handle handle = dev->data;
 	struct acpi_buffer buffer;
 	int ret;
-	acpi_status status;
 
 	dev_dbg(&dev->dev, "set resources\n");
 	ret = pnpacpi_build_resource_template(dev, &buffer);

@@ -95,21 +94,30 @@ static int pnpacpi_set_resources(struct
 		kfree(buffer.pointer);
 		return ret;
 	}
-	status = acpi_set_current_resources(handle, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(acpi_set_current_resources(handle, &buffer)))
 		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);
 	kfree(buffer.pointer);
 	return ret;
 }
 
 static int pnpacpi_disable_resources(struct pnp_dev *dev)
 {
-	acpi_status status;
+	acpi_handle handle = dev->data;
+	int ret;
 
+	dev_dbg(&dev->dev, "disable resources\n");
 	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	ret = 0;
+	if (acpi_bus_power_manageable(handle)) {
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+		if (ret)
+			return ret;
+	}
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
+		ret = -ENODEV;
+	return ret;
 }
 
 #ifdef CONFIG_ACPI_SLEEP
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Witold Szczeponik

Subject: Enable PNPACPI _PSx Support, v3

Very good patch.
Do you have the hardware on which the dedicated power source is
required by the serial port(PnP device) in your hand?

When the Pnp ACPI device is started, it should be changed to D0 state if there exists the _PSX object or the power resource is required. When it is disabled, it should be changed to D3 state.

Acked-by Zhao Yakui <[email protected]>

Thanks.

Quoted 56 lines

(This is an update to the patch presented earlier in
http://lkml.org/lkml/2008/12/8/284, with new error handling.)

This patch sets the power of PnP ACPI devices to D0 when they are activated and to D3 when they are disabled. The latter is in correspondence with the ACPI 3.0 specification, whereas the former is added in order to be able to power up a device after it has been previously disabled (or when booting up a system). (As a consequence, the patch makes the PnP ACPI code more ACPI compliant.)

Section 6.2.2 of the ACPI Specification (at least versions 1.0b and 3.0a) states: "Prior to running this control method [_DIS], the OS[PM] will have already put the device in the D3 state." Unfortunately, there is no clear statement as to when to put a device in the D0 state. :-( Therefore, the patch executes the method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the device is disabled, _SRS enables the device at the specified resources." (From the ACPI 3.0a Specification.)

The patch fixes a problem with some IBM ThinkPads (at least the 600E and the 600X) where the serial ports have a dedicated power source that needs to be brought up before the serial port can be used. Without this patch, the serial port is enabled but has no power. (In the past, the tpctl utility had to be utilized to turn on the power, but support for this feature stopped with version 5.9 as it did not support the more recent kernel versions.)

The error handlers that handle any errors that can occur during the power up/power down phases return the error codes to the caller directly. Comments welcome! :-)

No regressions were observed on hardware that does not require this patch.

The patch is applied against 2.6.27.x.

Signed-off-by Witold Szczeponik <[email protected]>

Index: linux/drivers/pnp/pnpacpi/core.c
===================================================================

drivers/pnp/pnpacpi/core.c +0 -1
@@ -84,7 +84,6 @@ static int pnpacpi_set_resources(struct
  	acpi_handle handle = dev->data;
  	struct acpi_buffer buffer;
  	int ret;
-	acpi_status status;
 
  	dev_dbg(&dev->dev, "set resources\n");
  	ret = pnpacpi_build_resource_template(dev, &buffer);

drivers/pnp/pnpacpi/core.c static int pnpacpi_set_resources(struct · line 100

Quoted 9 lines
drivers/pnp/pnpacpi/core.c +3 -2
@@ -95,21 +94,30 @@ static int pnpacpi_set_resources(struct
  		kfree(buffer.pointer);
  		return ret;
  	}
-	status = acpi_set_current_resources(handle, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(acpi_set_current_resources(handle, &buffer)))
  		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);

How about changing the power state before calling _SRS object?

Quoted 31 lines
drivers/pnp/pnpacpi/core.c +12 -5
  	kfree(buffer.pointer);
  	return ret;
  }
 
  static int pnpacpi_disable_resources(struct pnp_dev *dev)
  {
-	acpi_status status;
+	acpi_handle handle = dev->data;
+	int ret;
 
+	dev_dbg(&dev->dev, "disable resources\n");
  	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	ret = 0;
+	if (acpi_bus_power_manageable(handle)) {
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+		if (ret)
+			return ret;
+	}
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
+		ret = -ENODEV;
+	return ret;
  }
 
  #ifdef CONFIG_ACPI_SLEEP
--

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

author
#2 lore raw
In reply to yakui_zhao
Quoted 4 lines · yakui_zhao

Very good patch.
Do you have the hardware on which the dedicated power source is
required by the serial port(PnP device) in your hand?

Yes, I have an IBM ThinkPad 600E. I need this patch in order to be able receive accurate time using a DCF77 receiver attached to the serial port.

[snip]

Quoted 6 lines

drivers/pnp/pnpacpi/core.c static int pnpacpi_set_resources(struct · line 100

Quoted 5 lines
drivers/pnp/pnpacpi/core.c +3 -1
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(acpi_set_current_resources(handle, &buffer)))
  		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);

How about changing the power state before calling _SRS object?

I recall reading something in the ACPI spec that says that the power needs to be set after the device was enabled. But the main driver for this order (_SRS followed by _PS0) is the symmetry to the required _PS3 followed by _DIS (according to the spec).

[snip]

--- Witold

reviewer Acked-by Zhao Yakui
#5 lore raw
Quoted 1 line

From 6328a57401dc5f5cf9931738eb7268fcd8058c49 Mon Sep 17 00:00:00 2001

From: Witold Szczeponik <[email protected]>
Date: Mon, 30 Mar 2009 19:31:06 +0200
Subject: [PATCH] Enable PNPACPI _PSx Support, v3
X-Patchwork-Hint: ignore

(This is an update to the patch presented earlier in
http://lkml.org/lkml/2008/12/8/284, with new error handling.)

This patch sets the power of PnP ACPI devices to D0 when they are activated and to D3 when they are disabled. The latter is in correspondence with the ACPI 3.0 specification, whereas the former is added in order to be able to power up a device after it has been previously disabled (or when booting up a system). (As a consequence, the patch makes the PnP ACPI code more ACPI compliant.)

Section 6.2.2 of the ACPI Specification (at least versions 1.0b and 3.0a) states: "Prior to running this control method [_DIS], the OS[PM] will have already put the device in the D3 state." Unfortunately, there is no clear statement as to when to put a device in the D0 state. :-( Therefore, the patch executes the method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the device is disabled, _SRS enables the device at the specified resources." (From the ACPI 3.0a Specification.)

The patch fixes a problem with some IBM ThinkPads (at least the 600E and the 600X) where the serial ports have a dedicated power source that needs to be brought up before the serial port can be used. Without this patch, the serial port is enabled but has no power. (In the past, the tpctl utility had to be utilized to turn on the power, but support for this feature stopped with version 5.9 as it did not support the more recent kernel versions.)

The error handlers that handle any errors that can occur during the power up/power down phases return the error codes to the caller directly. Comments welcome! :-)

No regressions were observed on hardware that does not require this patch.

The patch is applied against 2.6.27.x.

Signed-off-by Witold Szczeponik <[email protected]>
Acked-by Zhao Yakui <[email protected]>
Signed-off-by Len Brown <[email protected]>

---

(not immediately clear what tree the orginal patch was based on top of, but here is what I checked-in on top of 2.6.29 -Len)

 drivers/pnp/pnpacpi/core.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)
drivers/pnp/pnpacpi/core.c +16 -7
index 2834846..9a3a682 100644
@@ -83,7 +83,6 @@ static int pnpacpi_set_resources(struct pnp_dev *dev)
 	acpi_handle handle = dev->data;
 	struct acpi_buffer buffer;
 	int ret;
-	acpi_status status;
 
 	pnp_dbg(&dev->dev, "set resources\n");
 	ret = pnpacpi_build_resource_template(dev, &buffer);

@@ -94,21 +93,31 @@ static int pnpacpi_set_resources(struct pnp_dev *dev)
 		kfree(buffer.pointer);
 		return ret;
 	}
-	status = acpi_set_current_resources(handle, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(acpi_set_current_resources(handle, &buffer)))
 		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);
 	kfree(buffer.pointer);
 	return ret;
 }
 
 static int pnpacpi_disable_resources(struct pnp_dev *dev)
 {
-	acpi_status status;
+	acpi_handle handle = dev->data;
+	int ret;
+
+	dev_dbg(&dev->dev, "disable resources\n");
 
 	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */
-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	ret = 0;
+	if (acpi_bus_power_manageable(handle)) {
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+		if (ret)
+			return ret;
+	}
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DIS", NULL, NULL)))
+		ret = -ENODEV;
+	return ret;
 }
 
 #ifdef CONFIG_ACPI_SLEEP
Signature · 1 line
1.6.2.2.404.ge96f3
#7 lore raw
In reply to a message not in the archive
Quoted 49 lines · Thomas Renninger

Hi,

I remember a bug (serial device not working) related to serial and docking on Lenovo.
I wonder whether this could be related to this fix.

This one also looks worth adding to stable kernel, at least after 2.6.30 does not show regressions for a while.

Quoted 39 lines · On Monday 30 March 2009 19:31:06 Witold Szczeponik wrote:

Subject: Enable PNPACPI _PSx Support, v3

(This is an update to the patch presented earlier in
http://lkml.org/lkml/2008/12/8/284, with new error handling.)

This patch sets the power of PnP ACPI devices to D0 when they are activated and to D3 when they are disabled. The latter is in correspondence with the ACPI 3.0 specification, whereas the former is added in order to be able to power up a device after it has been previously disabled (or when booting up a system). (As a consequence, the patch makes the PnP ACPI code more ACPI compliant.)

Section 6.2.2 of the ACPI Specification (at least versions 1.0b and 3.0a) states: "Prior to running this control method [_DIS], the OS[PM] will have already put the device in the D3 state." Unfortunately, there is no clear statement as to when to put a device in the D0 state. :-( Therefore, the patch executes the method calls as _PS3/_DIS and _SRS/_PS0. What is clear: "If the device is disabled, _SRS enables the device at the specified resources." (From the ACPI 3.0a Specification.)

The patch fixes a problem with some IBM ThinkPads (at least the 600E and the 600X) where the serial ports have a dedicated power source that needs to be brought up before the serial port can be used. Without this patch, the serial port is enabled but has no power. (In the past, the tpctl utility had to be utilized to turn on the power, but support for this feature stopped with version 5.9 as it did not support the more recent kernel versions.)

The error handlers that handle any errors that can occur during the power up/power down phases return the error codes to the caller directly. Comments welcome! :-)

No regressions were observed on hardware that does not require this patch.

Quoted 6 lines
Quoted 4 lines

The patch is applied against 2.6.27.x.

Signed-off-by Witold Szczeponik <[email protected]>

CC: [email protected]
...

What specific git commit id in Linus's tree is this?

thanks,

greg k-h

author
#8 lore raw

Greg KH wrote:

[snip]

Quoted 7 lines

What specific git commit id in Linus's tree is this?

thanks,

greg k-h

Hi Greg,

the git commit id of this patch would be 6328a57401dc5f5cf9931738eb7268fcd8058c49, but the patch triggered a regression which is described in http://bugzilla.kernel.org/show_bug.cgi?id=13243 and which was fixed in 19bde778c1fd2574cc020a618d7d576f260271ca.

Hope this helps.

--- Witold

#9 lore raw
Quoted 12 lines · Witold Szczeponik

Greg KH wrote:

[snip]

Quoted 7 lines

What specific git commit id in Linus's tree is this?

thanks,

greg k-h

Quoted 7 lines

Hi Greg,

the git commit id of this patch would be 6328a57401dc5f5cf9931738eb7268fcd8058c49, but the patch triggered a regression which is described in
http://bugzilla.kernel.org/show_bug.cgi?id=13243 and which was fixed in 19bde778c1fd2574cc020a618d7d576f260271ca.

Ok, I've queued up both of these for the next .27 stable release.

thanks,

greg k-h