PNPACPI: Enable Power Management

Subject: Enable PNPACI Power Management

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.)

The patch fixes the 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.

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

The patch is applied against 2.6.27.7 (vanilla).

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

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

drivers/pnp/pnpacpi/core.c +6 -3

-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+	status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
+	if (ACPI_FAILURE(status))
+		ret = -ENODEV;
+	return ret;
  }
 
  #ifdef CONFIG_ACPI_SLEEP

Quoted 1 line · Witold Szczeponik

Subject: Enable PNPACI Power Management

Hi Witold,

Thanks for your patch. I'm glad somebody is paying attention to PNP and power. I CC'd Adam and Rafael because they care about this area, too, but might not read everything on linux-acpi.

Do you know of anything that specifies the order of the _CRS/_PS0 and the _PS3/_DIS evaluation? I don't know much about power management, and I couldn't find anything obvious in the spec. It seems plausible that we should run _CRS before turning on the power, but I really don't know.

Is pnpacpi_set_resources() the only place that needs this change? For active devices, we normally don't call pnpacpi_set_resources() at all. So I suppose on these ThinkPads, we exercise this path because the serial ports are initially disabled?

Quoted 9 lines

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

The patch is applied against 2.6.27.7 (vanilla).

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

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

Quoted 10 lines

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

drivers/pnp/pnpacpi/core.c +2 -0

@@ -98,18 +98,24 @@ static int pnpacpi_set_resources(struct
  	status = acpi_set_current_resources(handle, &buffer);
  	if (ACPI_FAILURE(status))
  		ret = -EINVAL;
+	else if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D0);

I don't really like testing acpi_bus_power_manageable() first. I think we should just call acpi_bus_set_power() and let *it* bail out if the device doesn't support it.

Quoted 11 lines

drivers/pnp/pnpacpi/core.c +2 -0

  	kfree(buffer.pointer);
  	return ret;
  }
 
  static int pnpacpi_disable_resources(struct pnp_dev *dev)
  {
+	acpi_handle handle = dev->data;
+	int ret = 0;
  	acpi_status status;
 

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

Quoted 1 line

drivers/pnp/pnpacpi/core.c +0 -1

-	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */

Can you leave the "unregister_gsi" comment there, since it's not related to your patch? It's a reminder that we need to think about how to handle interrupts when enabling/disabling devices.

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

Quoted 11 lines

drivers/pnp/pnpacpi/core.c +6 -3

-	status = acpi_evaluate_object((acpi_handle) dev->data,
-				      "_DIS", NULL, NULL);
-	return ACPI_FAILURE(status) ? -ENODEV : 0;
+	if (acpi_bus_power_manageable(handle))
+		ret = acpi_bus_set_power(handle, ACPI_STATE_D3);
+	status = acpi_evaluate_object(handle, "_DIS", NULL, NULL);
+	if (ACPI_FAILURE(status))
+		ret = -ENODEV;
+	return ret;
  }

Quoted 2 lines

drivers/pnp/pnpacpi/core.c +0 -0

  #ifdef CONFIG_ACPI_SLEEP

Quoted 5 lines · Bjorn Helgaas

Do you know of anything that specifies the order of the _CRS/_PS0 and the _PS3/_DIS evaluation? I don't know much about power management, and I couldn't find anything obvious in the spec. It seems plausible that we should run _CRS before turning on the power, but I really don't know.

There's some info the ACPI Specification that says a device needs to be put to D3 before _DIS is called (section 6.2.2) but there is no clear statement as to when to put a device to D0... :-(

I think it should be _SRS/_PS0 and _PS3/_DIS.

Quoted 4 lines

Is pnpacpi_set_resources() the only place that needs this change? For active devices, we normally don't call pnpacpi_set_resources() at all. So I suppose on these ThinkPads, we exercise this path because the serial ports are initially disabled?

I'm not sure. I would guess that we need to put any device that is enabled (either via _SRS or by default) into D0. My patch does that for the _SRS case but the generic ACPI code does not. On my 600E, the serial port has power when the machine is booted but has no power once GRUB kicks in. It remains in this state until the 8250-pnp module gets loaded, where my patch enables it.

The ACPI Specification says for _SRS: the device is enabled after the method has returned. There is no statement as to when to call _PS0...

Quoted 11 lines

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

Quoted 7 lines

  static int pnpacpi_disable_resources(struct pnp_dev *dev)
  {

drivers/pnp/pnpacpi/core.c +2 -1

+	acpi_handle handle = dev->data;
+	int ret = 0;
  	acpi_status status;
 
-	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */

Can you leave the "unregister_gsi" comment there, since it's not related to your patch? It's a reminder that we need to think about how to handle interrupts when enabling/disabling devices.

I'd rather remove the comment as it is misleading, IMHO. This call should be made by a driver. After all, the PNPACPI core does not register any IRQs, either. Otherwise, we need to think about "pnp_irq(dev, 1)", too. Either way, with my patch there should be no IRQ handling possible.

Quoted 1 line

I think it should be _SRS/_PS0 and _PS3/_DIS.

Thanks for the pointer. That makes sense, and your proposed order makes the enable/disable paths symmetric, so I think you're right. Can you put that spec pointer in your changelog?

Quoted 10 lines

Quoted 4 lines

Is pnpacpi_set_resources() the only place that needs this change? For active devices, we normally don't call pnpacpi_set_resources() at all. So I suppose on these ThinkPads, we exercise this path because the serial ports are initially disabled?

I'm not sure. I would guess that we need to put any device that is enabled (either via _SRS or by default) into D0. My patch does that for the _SRS case but the generic ACPI code does not. On my 600E, the serial port has power when the machine is booted but has no power once GRUB kicks in. It remains in this state until the 8250-pnp module gets loaded, where my patch enables it.

Interesting. I'd be surprised if GRUB does anything with ACPI to disable the port. I don't know how you determine when the port has power. Maybe the power-up default state is powered, and the BIOS turns it off before launching GRUB?

I wonder if _STA is influenced by the power state. I would think if _STA said a device was "enabled and decoding resources," that would only make sense if the device were already in D0. But let's say we start with an active device, then run _PS3 and _DIS. What would _STA say in the interval between _PS3 and _DIS?

This is just idle curiousity on my part, I guess. I just don't know much about ACPI power states, and I'm afraid of getting in trouble if we change too much. The _SRS path is a little less scary because it won't be exercised much (most devices are already enabled, and we don't change their settings).

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

drivers/pnp/pnpacpi/core.c +0 -1

-	/* acpi_unregister_gsi(pnp_irq(dev, 0)); */

Can you leave the "unregister_gsi" comment there, since it's not related to your patch? It's a reminder that we need to think about how to handle interrupts when enabling/disabling devices.

Quoted 5 lines

I'd rather remove the comment as it is misleading, IMHO. This call should be made by a driver. After all, the PNPACPI core does not register any IRQs, either. Otherwise, we need to think about "pnp_irq(dev, 1)", too. Either way, with my patch there should be no IRQ handling possible.

The comment is logically unrelated to the rest of your patch, so I would at least split it into a separate patch just on that grounds.

The PNP core *does* register IRQs: we call acpi_register_gsi() in pnpacpi_parse_allocated_irqresource(), but there's no corresponding unregister. I think this asymmetry is a bug, but I haven't looked at how to handle it yet. PNP currently does the registration "eagerly," when the device is discovered. I think we should instead do the registration/unregistration "lazily," when a driver claims the device, as PCI does.

I haven't changed this yet because there are some issues related to pcibios_penalize_isa_irq() -- we don't know the IRQ to penalize until we register the GSI.

Anyway, that's a long-winded explanation of why that stupid-looking comment still has some value to me :-)

Bjorn