Re: i915 hibernation patch (was: Re: [Suspend-devel] 2.6.25-rc2System no longer powers off aftersuspend-to-disk. Screen becomes green.)

[Linus Torvalds]

On Sat, 23 Feb 2008, Rafael J. Wysocki wrote:

> --- linux-2.6.orig/drivers/char/drm/i915_drv.c
> +++ linux-2.6/drivers/char/drm/i915_drv.c
> @@ -222,6 +222,7 @@ static void i915_restore_vga(struct drm_
>  			   dev_priv->saveGR[0x18]);
>  
>  	/* Attribute controller registers */
> +	inb(st01);
>  	for (i = 0; i < 20; i++)
>  		i915_write_ar(st01, i, dev_priv->saveAR[i], 0);
>  	inb(st01); /* switch back to index mode */

I'm doing this part separately, please drop it - it has nothing to do with 
the rest of the patch.

I'd also suggest that you just add a helper function like

	int pm_event_powerdown(struct pm_message mesg)
	{
		return mesg.event >= PM_EVENT_SUSPEND;
	}

or something, so that you can have code like

	if (pm_event_powerdown(mesg))
		pci_set_power_state(pdev, PCI_D3hot);

instead of the test for EVENT_SUSPEND/HIBERNATE explicitly.

Of course, the places that already do a switch-statement are much better 
kept that way and just add PM_EVENT_HIBERNATE to the list.

> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -554,6 +554,7 @@ pci_power_t pci_choose_state(struct pci_
>  	case PM_EVENT_PRETHAW:
>  		/* REVISIT both freeze and pre-thaw "should" use D0 */
>  	case PM_EVENT_SUSPEND:
> +	case PM_EVENT_HIBERNATE:
>  		return PCI_D3hot;

Didn't you miss the apci_pci_choose_state() thing that also needs this 
extension?

> Index: linux-2.6/drivers/usb/host/u132-hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/u132-hcd.c
> +++ linux-2.6/drivers/usb/host/u132-hcd.c
> @@ -3214,14 +3214,18 @@ static int u132_suspend(struct platform_
>                  return -ESHUTDOWN;
>          } else {
>                  int retval = 0;
> -                if (state.event == PM_EVENT_FREEZE) {
> +
> +		switch (state.event) {
> +		case PM_EVENT_FREEZE:
>                          retval = u132_bus_suspend(hcd);
> -                } else if (state.event == PM_EVENT_SUSPEND) {
> +			break;
> +		case PM_EVENT_SUSPEND:
> +		case PM_EVENT_HIBERNATE:
>                          int ports = MAX_U132_PORTS;
>                          while (ports-- > 0) {
>                                  port_power(u132, ports, 0);
>                          }
> -                }
> +			break;
>                  if (retval == 0)
>                          pdev->dev.power.power_state = state;
>                  return retval;

Looks like a missing close-brace to me there - you removed the final '}'.

Or am I blind?

Apart from those issues it looks fine to me.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/