[PATCH] fix tulip suspend/resume

[PATCH] fix tulip suspend/resume

[Karsten Keil]

Hi,

following patch fix the suspend/resume for tulip based
cards, so suspend on disk work now for me and tulip based
cardbus cards.


Signed-off-by: Karsten Keil <kkeil@suse.de>

--- linux/drivers/net/tulip/tulip_core.c.orig	2005-03-23 23:54:43.000000000 +0100
+++ linux/drivers/net/tulip/tulip_core.c	2005-05-26 17:29:14.000000000 +0200
@@ -1755,12 +1755,16 @@
 static int tulip_suspend (struct pci_dev *pdev, pm_message_t state)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	int err;
 
+	pci_save_state(pdev);
 	if (dev && netif_running (dev) && netif_device_present (dev)) {
 		netif_device_detach (dev);
 		tulip_down (dev);
 		/* pci_power_off(pdev, -1); */
 	}
+	if ((err = pci_set_power_state(pdev, PCI_D3hot)))
+		printk(KERN_ERR "%s: pci_set_power_state D3hot return %d\n", dev->name, err);
 	return 0;
 }
 
@@ -1768,7 +1772,11 @@
 static int tulip_resume(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	int err;
 
+	if ((err = pci_set_power_state(pdev, PCI_D0)))
+		printk(KERN_ERR "%s: pci_set_power_state D0 return %d\n", dev->name, err);
+	pci_restore_state(pdev);
 	if (dev && netif_running (dev) && !netif_device_present (dev)) {
 #if 1
 		pci_enable_device (pdev);

-- 
Karsten Keil
SuSE Labs
ISDN development

Re: [PATCH] fix tulip suspend/resume

[Linus Torvalds]

Jeff, 
 this looks ok, but I'll leave the decision to you. Things like this often 
break.

Andrew, maybe at least a few days in -mm to see if there's some outcry?

		Linus

On Tue, 7 Jun 2005, Karsten Keil wrote:
> 
> following patch fix the suspend/resume for tulip based
> cards, so suspend on disk work now for me and tulip based
> cardbus cards.
> 
> 
> Signed-off-by: Karsten Keil <kkeil@suse.de>
> 
> --- linux/drivers/net/tulip/tulip_core.c.orig	2005-03-23 23:54:43.000000000 +0100
> +++ linux/drivers/net/tulip/tulip_core.c	2005-05-26 17:29:14.000000000 +0200
> @@ -1755,12 +1755,16 @@
>  static int tulip_suspend (struct pci_dev *pdev, pm_message_t state)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int err;
>  
> +	pci_save_state(pdev);
>  	if (dev && netif_running (dev) && netif_device_present (dev)) {
>  		netif_device_detach (dev);
>  		tulip_down (dev);
>  		/* pci_power_off(pdev, -1); */
>  	}
> +	if ((err = pci_set_power_state(pdev, PCI_D3hot)))
> +		printk(KERN_ERR "%s: pci_set_power_state D3hot return %d\n", dev->name, err);
>  	return 0;
>  }
>  
> @@ -1768,7 +1772,11 @@
>  static int tulip_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int err;
>  
> +	if ((err = pci_set_power_state(pdev, PCI_D0)))
> +		printk(KERN_ERR "%s: pci_set_power_state D0 return %d\n", dev->name, err);
> +	pci_restore_state(pdev);
>  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
>  #if 1
>  		pci_enable_device (pdev);
> 
> -- 
> Karsten Keil
> SuSE Labs
> ISDN development
> 

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Mon, Jun 06, 2005 at 05:04:07PM -0700, Linus Torvalds wrote:
> 
> Jeff, 
>  this looks ok, but I'll leave the decision to you. Things like this often 
> break.
> 
> Andrew, maybe at least a few days in -mm to see if there's some outcry?
> 
> 		Linus

This patch is an improvement, but there may still be some issues.
Specifically, it looks to me like the the interrupt handler remains
registered.  This could cause some problems when another device is sharing
the interrupt because the tulip driver must read from a hardware register
to determine if it triggered the interrupt. When the hardware has been
physically powered off, things might not go well.

I can't comment on the netdev class aspect of this routine, but following
a similar strategy to its original author, a fix might look like this:

--- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
+++ b/drivers/net/tulip/tulip_core.c	2005-06-06 22:14:25.850846400 -0400
@@ -1759,7 +1759,12 @@
 	if (dev && netif_running (dev) && netif_device_present (dev)) {
 		netif_device_detach (dev);
 		tulip_down (dev);
-		/* pci_power_off(pdev, -1); */
+
+		pci_save_state(pdev);
+
+		free_irq(dev->irq, dev);
+		pci_disable_device(pdev);
+		pci_set_power_state(pdev, pci_choose_state(pdev, state));
 	}
 	return 0;
 }
@@ -1768,12 +1773,19 @@
 static int tulip_resume(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	int retval;
 
 	if (dev && netif_running (dev) && !netif_device_present (dev)) {
-#if 1
-		pci_enable_device (pdev);
-#endif
-		/* pci_power_on(pdev); */
+		pci_set_power_state(pdev, PCI_D0);
+		pci_restore_state(pdev);
+
+		pci_enable_device(pdev);
+
+		if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
+			printk (KERN_ERR "tulip: request_irq failed in resume\n");
+			return retval;
+		}
+		
 		tulip_up (dev);
 		netif_device_attach (dev);
 	}

I don't have this hardware, so any testing would be appreciated.

Thanks,
Adam


P.S.: I noticed this function in the tulip driver:

static void tulip_set_power_state (struct tulip_private *tp,
				   int sleep, int snooze)
{
	if (tp->flags & HAS_ACPI) {
		u32 tmp, newtmp;
		pci_read_config_dword (tp->pdev, CFDD, &tmp);
		newtmp = tmp & ~(CFDD_Sleep | CFDD_Snooze);
		if (sleep)
			newtmp |= CFDD_Sleep;
		else if (snooze)
			newtmp |= CFDD_Snooze;
		if (tmp != newtmp)
			pci_write_config_dword (tp->pdev, CFDD, newtmp);
	}

}

Currently we aren't using CFDD_Sleep.  Should we call this in suspend?  It
could be important if the hardware doesn't support PCI PM.  I don't really
have any specifications or information about the hardware, so I'm at a loss
here.

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> This patch is an improvement, but there may still be some issues.
> Specifically, it looks to me like the the interrupt handler remains
> registered.  This could cause some problems when another device is sharing
> the interrupt because the tulip driver must read from a hardware register
> to determine if it triggered the interrupt. When the hardware has been
> physically powered off, things might not go well.
> 
> I can't comment on the netdev class aspect of this routine, but following
> a similar strategy to its original author, a fix might look like this:

Don't like it, see below.

> @@ -1768,12 +1773,19 @@
>  static int tulip_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int retval;
>  
>  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
> -#if 1
> -		pci_enable_device (pdev);
> -#endif
> -		/* pci_power_on(pdev); */
> +		pci_set_power_state(pdev, PCI_D0);

Oh well... you are turning power back on ... but you don't know if you
_can_ ... what if your parent bridge has been disabled in some way ? Or
the clock generation on the bus stopped already ?

I think the kernel should warn & disable the IRQ if that happens
(basically you should return NOT_HANDLED)

> +		pci_restore_state(pdev);
> +
> +		pci_enable_device(pdev);
> +
> +		if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
> +			printk (KERN_ERR "tulip: request_irq failed in resume\n");
> +			return retval;
> +		}
> +		
>  		tulip_up (dev);
>  		netif_device_attach (dev);
>  	}

Also, isn't that racy vs. the code in suspend() anyway ? You need to
make sure you program your chip not to issue any interrupt and
synchronize proerly, then just "ignore" (don't handle) interrupts coming
in as they should not be for you.

> I don't have this hardware, so any testing would be appreciated.
> 
> Thanks,
> Adam
> 
> 
> P.S.: I noticed this function in the tulip driver:
> 
> static void tulip_set_power_state (struct tulip_private *tp,
> 				   int sleep, int snooze)
> {
> 	if (tp->flags & HAS_ACPI) {
> 		u32 tmp, newtmp;
> 		pci_read_config_dword (tp->pdev, CFDD, &tmp);
> 		newtmp = tmp & ~(CFDD_Sleep | CFDD_Snooze);
> 		if (sleep)
> 			newtmp |= CFDD_Sleep;
> 		else if (snooze)
> 			newtmp |= CFDD_Snooze;
> 		if (tmp != newtmp)
> 			pci_write_config_dword (tp->pdev, CFDD, newtmp);
> 	}
> 
> }
> 
> Currently we aren't using CFDD_Sleep.  Should we call this in suspend?  It
> could be important if the hardware doesn't support PCI PM.  I don't really
> have any specifications or information about the hardware, so I'm at a loss
> here.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, 2005-06-07 at 13:34 +1000, Benjamin Herrenschmidt wrote:
> > This patch is an improvement, but there may still be some issues.
> > Specifically, it looks to me like the the interrupt handler remains
> > registered.  This could cause some problems when another device is sharing
> > the interrupt because the tulip driver must read from a hardware register
> > to determine if it triggered the interrupt. When the hardware has been
> > physically powered off, things might not go well.
> > 
> > I can't comment on the netdev class aspect of this routine, but following
> > a similar strategy to its original author, a fix might look like this:
> 
> Don't like it, see below.
> 
> > @@ -1768,12 +1773,19 @@
> >  static int tulip_resume(struct pci_dev *pdev)
> >  {
> >  	struct net_device *dev = pci_get_drvdata(pdev);
> > +	int retval;
> >  
> >  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
> > -#if 1
> > -		pci_enable_device (pdev);
> > -#endif
> > -		/* pci_power_on(pdev); */
> > +		pci_set_power_state(pdev, PCI_D0);
> 
> Oh well... you are turning power back on ... but you don't know if you
> _can_ ... what if your parent bridge has been disabled in some way ? Or
> the clock generation on the bus stopped already ?

We don't support PCI bus power management, so we don't have any idea
what the parent is doing.  Also, we don't have a pci bridge driver (one
that uses "struct pci_driver" to handle bridge resumes properly.  I'm
working on these issues.  I also have some changes in mind for the PM
model to make it more friendly to the power dependency problem.  So in
short, I think this is fine for now, as every other driver is doing it
incorrectly, and in general it is working ok for suspend and resume.
They're all broken in this respect, and we need to gradually fix them.
But first we need the infrastructure to make this possible.

I haven't decided yet, but we could probably hide much of this inside
pci_set_power_state().

> 
> I think the kernel should warn & disable the IRQ if that happens
> (basically you should return NOT_HANDLED)
> 
> > +		pci_restore_state(pdev);
> > +
> > +		pci_enable_device(pdev);
> > +
> > +		if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
> > +			printk (KERN_ERR "tulip: request_irq failed in resume\n");
> > +			return retval;
> > +		}
> > +		
> >  		tulip_up (dev);
> >  		netif_device_attach (dev);
> >  	}
> 
> Also, isn't that racy vs. the code in suspend() anyway ? You need to
> make sure you program your chip not to issue any interrupt and
> synchronize proerly, then just "ignore" (don't handle) interrupts coming
> in as they should not be for you.

Yeah, that's exactly what I had in mind.  As I understand, tulip_down
does tells the chip not to issue interrupts.  Then we unregister the
interrupt handler before powering down the device to avoid any issues
with shared interrupts.  The best way of ignoring interrupts is to
unregister the handler.  Do you still see a race condition?

Thanks,
Adam

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> We don't support PCI bus power management, so we don't have any idea
> what the parent is doing. 

Ugh ? You don't know, thus you can't assume it's working. A rule of the
device model is, once you have been suspended, you can't assume your
parent is still there and thus that you can talk to your device. On ppc
or embedded, the arch has ways to shut down clocks and/or power to
entire bus segment and that may have happened anytime.

> Also, we don't have a pci bridge driver (one
> that uses "struct pci_driver" to handle bridge resumes properly.  I'm
> working on these issues.

I know, but there may be arch thingies going on anyway. So the basic
"model" of turning back the chip on is wrong.

> I also have some changes in mind for the PM
> model to make it more friendly to the power dependency problem.  So in
> short, I think this is fine for now, as every other driver is doing it
> incorrectly, and in general it is working ok for suspend and resume.

No. just return IRQ_NONE. That is the only sane thing to do.

> They're all broken in this respect, and we need to gradually fix them.
> But first we need the infrastructure to make this possible.

No. That specific bit can be easily fixed for PCI drivers like that.
Just return IRQ_NONE, you shouldn't be emitting any IRQ yourself anyway.

> I haven't decided yet, but we could probably hide much of this inside
> pci_set_power_state().

No need.

> > Also, isn't that racy vs. the code in suspend() anyway ? You need to
> > make sure you program your chip not to issue any interrupt and
> > synchronize proerly, then just "ignore" (don't handle) interrupts coming
> > in as they should not be for you.
> 
> Yeah, that's exactly what I had in mind.  As I understand, tulip_down
> does tells the chip not to issue interrupts.  Then we unregister the
> interrupt handler before powering down the device to avoid any issues
> with shared interrupts.  The best way of ignoring interrupts is to
> unregister the handler.  Do you still see a race condition?

Well, if we have told the chip not to issue interrupts, then it's safe
to just have the handler return IRQ_NONE... we don't even need to
unregister the handler. (That's actually equivalent to some regard).

To not be racy, the best is to synchronize though. Something like this
pseudo code:

suspend():

  1) chip_disable_irq(); /* disable emission of IRQs on the chip,
                          * maybe do that & below in a spinlock_irq
                          * to make sure no other driver code path
                          * re-enables them
                          */

  2) me->sleeping = 1;  /* tells the rest of the driver I'm not there
                         * anymore, can be some netif_* thingy.
                         */

  3) synchronize_irq(me->irq); /* make sure above is visible to IRQs and
                                * any pending one competes on another
                                * CPU
                                */

  4) pci_set_power_state(), maybe free_irq(), etc...


my_irq_handler():

  if (me->sleeping)
    return IRQ_NONE;

That's it.

Ben.

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, 2005-06-07 at 14:26 +1000, Benjamin Herrenschmidt wrote:
> > We don't support PCI bus power management, so we don't have any idea
> > what the parent is doing. 
> 
> Ugh ? You don't know, thus you can't assume it's working. A rule of the
> device model is, once you have been suspended, you can't assume your
> parent is still there and thus that you can talk to your device. On ppc
> or embedded, the arch has ways to shut down clocks and/or power to
> entire bus segment and that may have happened anytime.

This device isn't accessed after *suspend.  By the time we reach
*resume, we know that the parent has had a *resume call first.  So if we
had a pci bus driver, we could enable the bridge device before this
network card reaches *resume.

> 
> > Also, we don't have a pci bridge driver (one
> > that uses "struct pci_driver" to handle bridge resumes properly.  I'm
> > working on these issues.
> 
> I know, but there may be arch thingies going on anyway. So the basic
> "model" of turning back the chip on is wrong.
> 
> > I also have some changes in mind for the PM
> > model to make it more friendly to the power dependency problem.  So in
> > short, I think this is fine for now, as every other driver is doing it
> > incorrectly, and in general it is working ok for suspend and resume.
> 
> No. just return IRQ_NONE. That is the only sane thing to do.

I was referring to the pci bus power management issue, not the irq
handler.  I'm sorry I wasn't clear about this.


> > > Also, isn't that racy vs. the code in suspend() anyway ? You need to
> > > make sure you program your chip not to issue any interrupt and
> > > synchronize proerly, then just "ignore" (don't handle) interrupts coming
> > > in as they should not be for you.
> > 
> > Yeah, that's exactly what I had in mind.  As I understand, tulip_down
> > does tells the chip not to issue interrupts.  Then we unregister the
> > interrupt handler before powering down the device to avoid any issues
> > with shared interrupts.  The best way of ignoring interrupts is to
> > unregister the handler.  Do you still see a race condition?
> 
> Well, if we have told the chip not to issue interrupts, then it's safe
> to just have the handler return IRQ_NONE... we don't even need to
> unregister the handler. (That's actually equivalent to some regard).

I think unregistering the handler is the equivalent and easier to get
right.  Otherwise, the driver developer needs to check a flag in the
interrupt handler to see if the device is sleeping, and if it is then
return IRQ_NONE.  Both options would work fine, but I don't see a race
condition with free_irq().

> 
> To not be racy, the best is to synchronize though. Something like this
> pseudo code:
> 
> suspend():
> 
>   1) chip_disable_irq(); /* disable emission of IRQs on the chip,
>                           * maybe do that & below in a spinlock_irq
>                           * to make sure no other driver code path
>                           * re-enables them
>                           */
> 
>   2) me->sleeping = 1;  /* tells the rest of the driver I'm not there
>                          * anymore, can be some netif_* thingy.
>                          */
> 
>   3) synchronize_irq(me->irq); /* make sure above is visible to IRQs and
>                                 * any pending one competes on another
>                                 * CPU
>                                 */

free_irq doesn't return until all pending irqs have completed, so we
don't need to do this if we're using the method I proposed.  In fact,
I think it calls synchronize_irq.

> 
>   4) pci_set_power_state(), maybe free_irq(), etc...
> 
> 
> my_irq_handler():
> 
>   if (me->sleeping)
>     return IRQ_NONE;
> 
> That's it.
> 
> Ben.

Thanks,
Adam

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> 
> I think unregistering the handler is the equivalent and easier to get
> right.  Otherwise, the driver developer needs to check a flag in the
> interrupt handler to see if the device is sleeping, and if it is then
> return IRQ_NONE.  Both options would work fine, but I don't see a race
> condition with free_irq().

You still need to disable IRQs on chip tho before you free_irq or you'll
put other devices sharing your interrupt in real pain in case your hw
accidentally emits one :)
 
> > To not be racy, the best is to synchronize though. Something like this
> > pseudo code:
> > 
> > suspend():
> > 
> >   1) chip_disable_irq(); /* disable emission of IRQs on the chip,
> >                           * maybe do that & below in a spinlock_irq
> >                           * to make sure no other driver code path
> >                           * re-enables them
> >                           */
> > 
> >   2) me->sleeping = 1;  /* tells the rest of the driver I'm not there
> >                          * anymore, can be some netif_* thingy.
> >                          */
> > 
> >   3) synchronize_irq(me->irq); /* make sure above is visible to IRQs and
> >                                 * any pending one competes on another
> >                                 * CPU
> >                                 */
> 
> free_irq doesn't return until all pending irqs have completed, so we
> don't need to do this if we're using the method I proposed.  In fact,
> I think it calls synchronize_irq.

Yes. free_irq above would be equivalent to synchronize_irq() and a good
replacement for it. With it, you don't even need the me->sleeping & test
in the IRQ handler since you simply cant call the IRQ handler after it's
free'd :)

> > 
> >   4) pci_set_power_state(), maybe free_irq(), etc...
> > 
> > 
> > my_irq_handler():
> > 
> >   if (me->sleeping)
> >     return IRQ_NONE;
> > 
> > That's it.
> > 
> > Ben.
> 
> Thanks,
> Adam
> 

Re: [PATCH] fix tulip suspend/resume

[Karsten Keil]

On Mon, Jun 06, 2005 at 10:50:55PM -0400, Adam Belay wrote:
> On Mon, Jun 06, 2005 at 05:04:07PM -0700, Linus Torvalds wrote:
> > 
> > Jeff, 
> >  this looks ok, but I'll leave the decision to you. Things like this often 
> > break.
> > 
> > Andrew, maybe at least a few days in -mm to see if there's some outcry?
> > 
> > 		Linus
> 
> This patch is an improvement, but there may still be some issues.
> Specifically, it looks to me like the the interrupt handler remains
> registered.  This could cause some problems when another device is sharing
> the interrupt because the tulip driver must read from a hardware register
> to determine if it triggered the interrupt. When the hardware has been
> physically powered off, things might not go well.

No, I also looked into this, it is not needed for the tulip driver, it
detects that it has no access to the hardware (reading 0xffffffff) in the
interrupt functions.

> 
> I can't comment on the netdev class aspect of this routine, but following
> a similar strategy to its original author, a fix might look like this:
> 
> --- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
> +++ b/drivers/net/tulip/tulip_core.c	2005-06-06 22:14:25.850846400 -0400
> @@ -1759,7 +1759,12 @@
>  	if (dev && netif_running (dev) && netif_device_present (dev)) {
>  		netif_device_detach (dev);
>  		tulip_down (dev);
> -		/* pci_power_off(pdev, -1); */
> +
> +		pci_save_state(pdev);
> +
> +		free_irq(dev->irq, dev);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
>  	}
>  	return 0;
>  }

Pavel told me, that pci_choose_state is not always safe, some cards maybe
do only work with PCI_D3hot properly, so it's better to use this now.
For my hardware it also seems to work with pci_choose_state.

> @@ -1768,12 +1773,19 @@
>  static int tulip_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int retval;
>  
>  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
> -#if 1
> -		pci_enable_device (pdev);
> -#endif
> -		/* pci_power_on(pdev); */
> +		pci_set_power_state(pdev, PCI_D0);
> +		pci_restore_state(pdev);
> +

I think restoring the PCI config should be done always, not only if the
device was in the up state, also powerup should be done.
If not you will run into problems to use the device later.

> +		pci_enable_device(pdev);
> +
> +		if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
> +			printk (KERN_ERR "tulip: request_irq failed in resume\n");
> +			return retval;
> +		}
> +		
>  		tulip_up (dev);
>  		netif_device_attach (dev);
>  	}
> 
> I don't have this hardware, so any testing would be appreciated.
> 
> Thanks,
> Adam
> 
> 
> P.S.: I noticed this function in the tulip driver:
> 
> static void tulip_set_power_state (struct tulip_private *tp,
> 				   int sleep, int snooze)
> {
> 	if (tp->flags & HAS_ACPI) {
> 		u32 tmp, newtmp;
> 		pci_read_config_dword (tp->pdev, CFDD, &tmp);
> 		newtmp = tmp & ~(CFDD_Sleep | CFDD_Snooze);
> 		if (sleep)
> 			newtmp |= CFDD_Sleep;
> 		else if (snooze)
> 			newtmp |= CFDD_Snooze;
> 		if (tmp != newtmp)
> 			pci_write_config_dword (tp->pdev, CFDD, newtmp);
> 	}
> 
> }
> 
> Currently we aren't using CFDD_Sleep.  Should we call this in suspend?  It
> could be important if the hardware doesn't support PCI PM.  I don't really
> have any specifications or information about the hardware, so I'm at a loss
> here.

-- 
Karsten Keil
SuSE Labs
ISDN development

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, Jun 07, 2005 at 12:55:52PM +0200, Karsten Keil wrote:
> On Mon, Jun 06, 2005 at 10:50:55PM -0400, Adam Belay wrote:
> > On Mon, Jun 06, 2005 at 05:04:07PM -0700, Linus Torvalds wrote:
> > > 
> > > Jeff, 
> > >  this looks ok, but I'll leave the decision to you. Things like this often 
> > > break.
> > > 
> > > Andrew, maybe at least a few days in -mm to see if there's some outcry?
> > > 
> > > 		Linus
> > 
> > This patch is an improvement, but there may still be some issues.
> > Specifically, it looks to me like the the interrupt handler remains
> > registered.  This could cause some problems when another device is sharing
> > the interrupt because the tulip driver must read from a hardware register
> > to determine if it triggered the interrupt. When the hardware has been
> > physically powered off, things might not go well.
> 
> No, I also looked into this, it is not needed for the tulip driver, it
> detects that it has no access to the hardware (reading 0xffffffff) in the
> interrupt functions.

Hmm, doesn't look like it:

>	/* Let's see whether the interrupt really is for us */
>	csr5 = ioread32(ioaddr + CSR5);
>
>	if (tp->flags & HAS_PHY_IRQ) 
>	        handled = phy_interrupt (dev);
>    
>	if ((csr5 & (NormalIntr|AbnormalIntr)) == 0)
>		return IRQ_RETVAL(handled);

Upon reading the value 0xffffffff for csr5, the if statement would be false.
Maybe it bails out later or something, but really why risk it?  As Ben noted,
it's easy to have race conditions between power off and interrupt handler
routines.  Also, there is a transition delay between D0 and D3.  I prefer to
play it safe and call free_irq().

There are other reasons to use free_irq().  For example, if we want to
support runtime device suspends, leaving a stale interrupt handler may
subtract from the performance of other devices sharing that interrupt...
especially in this case, where it's making pci transactions and reading
back 0xffffffff.

> > 
> > I can't comment on the netdev class aspect of this routine, but following
> > a similar strategy to its original author, a fix might look like this:
> > 
> > --- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
> > +++ b/drivers/net/tulip/tulip_core.c	2005-06-06 22:14:25.850846400 -0400
> > @@ -1759,7 +1759,12 @@
> >  	if (dev && netif_running (dev) && netif_device_present (dev)) {
> >  		netif_device_detach (dev);
> >  		tulip_down (dev);
> > -		/* pci_power_off(pdev, -1); */
> > +
> > +		pci_save_state(pdev);
> > +
> > +		free_irq(dev->irq, dev);
> > +		pci_disable_device(pdev);
> > +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >  	}
> >  	return 0;
> >  }
> 
> Pavel told me, that pci_choose_state is not always safe, some cards maybe
> do only work with PCI_D3hot properly, so it's better to use this now.
> For my hardware it also seems to work with pci_choose_state.

I think it's ok.

pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
{
	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
		return PCI_D0;

	switch (state) {
	case 0: return PCI_D0;
	case 3: return PCI_D3hot;
	default:
		printk("They asked me for state %d\n", state);
		BUG();
	}
	return PCI_D0;
}

It just checks if PCI function supports the power management capability.  If
it does we go to D3hot, if not, we stay at D0.  So the only case where it
wouldn't go to D3hot is when the device can't physically make the transition
through the PCI PM mechanism.

> 
> > @@ -1768,12 +1773,19 @@
> >  static int tulip_resume(struct pci_dev *pdev)
> >  {
> >  	struct net_device *dev = pci_get_drvdata(pdev);
> > +	int retval;
> >  
> >  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
> > -#if 1
> > -		pci_enable_device (pdev);
> > -#endif
> > -		/* pci_power_on(pdev); */
> > +		pci_set_power_state(pdev, PCI_D0);
> > +		pci_restore_state(pdev);
> > +
> 
> I think restoring the PCI config should be done always, not only if the
> device was in the up state, also powerup should be done.
> If not you will run into problems to use the device later.

Yeah, I think you're right.

Also, after looking at some other netdev suspend routines, it appears that
"netif_device_present" isn't necessary.  Jeff, is this correct?

Thanks,
Adam


How does this patch look:

--- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
+++ b/drivers/net/tulip/tulip_core.c	2005-06-07 16:34:13.641177456 -0400
@@ -1756,11 +1756,19 @@
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
 
-	if (dev && netif_running (dev) && netif_device_present (dev)) {
-		netif_device_detach (dev);
-		tulip_down (dev);
-		/* pci_power_off(pdev, -1); */
-	}
+	if (!dev)
+		return -EINVAL;
+
+	if (netif_running(dev))
+		tulip_down(dev);
+
+	netif_device_detach(dev);
+	pci_save_state(pdev);
+
+	free_irq(dev->irq, dev);
+	pci_disable_device(pdev);
+	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+
 	return 0;
 }
 
@@ -1768,15 +1776,26 @@
 static int tulip_resume(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	int retval;
 
-	if (dev && netif_running (dev) && !netif_device_present (dev)) {
-#if 1
-		pci_enable_device (pdev);
-#endif
-		/* pci_power_on(pdev); */
-		tulip_up (dev);
-		netif_device_attach (dev);
+	if (!dev)
+		return -EINVAL;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+
+	pci_enable_device(pdev);
+
+	if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
+		printk (KERN_ERR "tulip: request_irq failed in resume\n");
+		return retval;
 	}
+
+	netif_device_attach(dev);
+
+	if (netif_running(dev))
+		tulip_up(dev);
+
 	return 0;
 }
 

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> {
> 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> 		return PCI_D0;
> 
> 	switch (state) {
> 	case 0: return PCI_D0;
> 	case 3: return PCI_D3hot;
> 	default:
> 		printk("They asked me for state %d\n", state);
> 		BUG();
> 	}
> 	return PCI_D0;
> }

Gack ! I need to remember to fix that one before I change PMSG_FREEZE
definition to be different than PMSG_SUSPEND upstream.

Pavel, do you know that there are other ways to deal with errors than
just BUG()'ing all over the place ? :)

Ben.

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Wed, 2005-06-08 at 10:26 +1000, Benjamin Herrenschmidt wrote:
> > pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> > {
> > 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > 		return PCI_D0;
> > 
> > 	switch (state) {
> > 	case 0: return PCI_D0;
> > 	case 3: return PCI_D3hot;
> > 	default:
> > 		printk("They asked me for state %d\n", state);
> > 		BUG();
> > 	}
> > 	return PCI_D0;
> > }
> 
> Gack ! I need to remember to fix that one before I change PMSG_FREEZE
> definition to be different than PMSG_SUSPEND upstream.
> 
> Pavel, do you know that there are other ways to deal with errors than
> just BUG()'ing all over the place ? :)
> 
> Ben.

I think we should also use the pm_message_t defines.  We will need to
add PMSG_FREEZE eventually.  I decided to default to the current state
rather than panic.  Does this patch look ok?

Thanks,
Adam

--- a/drivers/pci/pci.c	2005-05-27 22:06:02.000000000 -0400
+++ b/drivers/pci/pci.c	2005-06-07 22:10:02.066151280 -0400
@@ -320,13 +320,15 @@
 		return PCI_D0;
 
 	switch (state) {
-	case 0: return PCI_D0;
-	case 3: return PCI_D3hot;
+	case PMSG_ON:
+		return PCI_D0;
+	case PMSG_SUSPEND:
+		return PCI_D3hot;
 	default:
-		printk("They asked me for state %d\n", state);
-		BUG();
+		printk(KERN_ERR "PCI: invalid PM message state - %d\n", state);
 	}
-	return PCI_D0;
+
+	return dev->current_state;
 }
 
 EXPORT_SYMBOL(pci_choose_state);

Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

Hi!
> > > pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> > > {
> > > 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > > 		return PCI_D0;
> > > 
> > > 	switch (state) {
> > > 	case 0: return PCI_D0;
> > > 	case 3: return PCI_D3hot;
> > > 	default:
> > > 		printk("They asked me for state %d\n", state);
> > > 		BUG();
> > > 	}
> > > 	return PCI_D0;
> > > }
> > 
> > Gack ! I need to remember to fix that one before I change PMSG_FREEZE
> > definition to be different than PMSG_SUSPEND upstream.
> > 
> > Pavel, do you know that there are other ways to deal with errors than
> > just BUG()'ing all over the place ? :)
> > 
> > Ben.
> 
> I think we should also use the pm_message_t defines.  We will need to
> add PMSG_FREEZE eventually.  I decided to default to the current state
> rather than panic.  Does this patch look ok?

No.

> --- a/drivers/pci/pci.c	2005-05-27 22:06:02.000000000 -0400
> +++ b/drivers/pci/pci.c	2005-06-07 22:10:02.066151280 -0400
> @@ -320,13 +320,15 @@
>  		return PCI_D0;
>  
>  	switch (state) {
> -	case 0: return PCI_D0;
> -	case 3: return PCI_D3hot;
> +	case PMSG_ON:
> +		return PCI_D0;
> +	case PMSG_SUSPEND:
> +		return PCI_D3hot;

Please don't do this; it will not compile when I turn on type checking
on pm_message_t. I have this:

/**
 * pci_choose_state - Choose the power state of a PCI device
 * @dev: PCI device to be suspended
 * @state: target sleep state for the whole system. This is the value
 *      that is passed to suspend() function.
 *
 * Returns PCI power state suitable for given device and given system
 * message.
 */

pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
{
        switch (state.event) {
        case PM_EVENT_ON:
                return PCI_D0;
        case PM_EVENT_FREEZE:
        case PM_EVENT_SUSPEND:
                return PCI_D3hot;
        default:
                printk("They asked me for state %d\n", state.event);
                BUG();
        }
        return PCI_D0;
}

>  	default:
> -		printk("They asked me for state %d\n", state);
> -		BUG();
> +		printk(KERN_ERR "PCI: invalid PM message state - %d\n", state);
>  	}
> -	return PCI_D0;
> +
> +	return dev->current_state;
>  }

You passed invalid argument; I see no reason why you should paper over
it and risk continuing. This happens during system suspend; it is
quite possible that user will not see your printk when machine powers
off just after that; and remember that it will not be in syslog after
resume.

								Pavel

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> > I think we should also use the pm_message_t defines.  We will need to
> > add PMSG_FREEZE eventually.  I decided to default to the current state
> > rather than panic.  Does this patch look ok?
> 
> No.

Hrm... I don't follow you anymore here ...

>         case PM_EVENT_ON:
>                 return PCI_D0;
>         case PM_EVENT_FREEZE:
>         case PM_EVENT_SUSPEND:
>                 return PCI_D3hot;

What are these new PM_EVENT_* things ? I though we defined PMSG_* ?

> You passed invalid argument; I see no reason why you should paper over
> it and risk continuing. This happens during system suspend; it is
> quite possible that user will not see your printk when machine powers
> off just after that; and remember that it will not be in syslog after
> resume.

Crap. I don't think a BUG() makes any useful help neither in this place,
and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
in there (and I wonder in how many other places).

Ben.

Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

Hi!

> > > I think we should also use the pm_message_t defines.  We will need to
> > > add PMSG_FREEZE eventually.  I decided to default to the current state
> > > rather than panic.  Does this patch look ok?
> > 
> > No.
> 
> Hrm... I don't follow you anymore here ...
> 
> >         case PM_EVENT_ON:
> >                 return PCI_D0;
> >         case PM_EVENT_FREEZE:
> >         case PM_EVENT_SUSPEND:
> >                 return PCI_D3hot;
> 
> What are these new PM_EVENT_* things ? I though we defined PMSG_* ?

PMSG_* are for struct; you can't case on struct.

> > You passed invalid argument; I see no reason why you should paper over
> > it and risk continuing. This happens during system suspend; it is
> > quite possible that user will not see your printk when machine powers
> > off just after that; and remember that it will not be in syslog after
> > resume.
> 
> Crap. I don't think a BUG() makes any useful help neither in this place,
> and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
> in there (and I wonder in how many other places).

At least you can see & report that error... That would not be a case
for simple printk.

This is the patch I'd like to go in. I hope it makes it
clear... Please base your development on top of this one...
									Pavel

---

Turn pm_message_t into struct, so that it is typechecked properly (and
so that we can add flags field in future). This should not go in
before 2.6.12.

Note: this rejects against -mm tree. Some rejects are caused by
	state_store getting more arguments, and some by added hook
	in pci_choose_state. I still do not like that pci_choose_state
	hook, it really should return pci_power_t, not int.

Signed-off-by: Pavel Machek <pavel@suse.cz>

---
commit a249072c4e0ef136c27c9e59d664e5be0d677ddc
tree 24a21ff5302734d40e08c400b14c0c1624cceded
parent f4bed68f59d9f32a4460288f40bb7f2af463babd
author <pavel@amd.(none)> Tue, 31 May 2005 11:57:50 +0200
committer <pavel@amd.(none)> Tue, 31 May 2005 11:57:50 +0200

 drivers/base/power/resume.c    |    8 ++++----
 drivers/base/power/runtime.c   |    8 ++++----
 drivers/base/power/suspend.c   |   12 ++++++------
 drivers/base/power/sysfs.c     |    8 ++++----
 drivers/ide/ide.c              |    4 ++--
 drivers/pci/pci.c              |   14 +++++++-------
 drivers/serial/pmac_zilog.c    |    2 +-
 drivers/usb/core/hub.c         |   18 +++++++++---------
 drivers/usb/core/usb.c         |    2 +-
 drivers/usb/host/ehci-dbg.c    |    2 +-
 drivers/usb/host/ohci-dbg.c    |    2 +-
 drivers/usb/host/sl811-hcd.c   |    6 +++---
 drivers/video/aty/atyfb_base.c |    4 ++--
 drivers/video/aty/radeon_pm.c  |   12 ++++++------
 drivers/video/i810/i810_main.c |    6 +++---
 include/linux/pm.h             |   14 ++++++++++----
 16 files changed, 64 insertions(+), 58 deletions(-)

Index: drivers/base/power/resume.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/resume.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/resume.c  (mode:100644)
@@ -23,11 +23,11 @@
 int resume_device(struct device * dev)
 {
 	if (dev->power.pm_parent
-			&& dev->power.pm_parent->power.power_state) {
+			&& dev->power.pm_parent->power.power_state.event) {
 		dev_err(dev, "PM: resume from %d, parent %s still %d\n",
-			dev->power.power_state,
+			dev->power.power_state.event,
 			dev->power.pm_parent->bus_id,
-			dev->power.pm_parent->power.power_state);
+			dev->power.pm_parent->power.power_state.event);
 	}
 	if (dev->bus && dev->bus->resume) {
 		dev_dbg(dev,"resuming\n");
@@ -50,7 +50,7 @@
 		list_add_tail(entry, &dpm_active);
 
 		up(&dpm_list_sem);
-		if (!dev->power.prev_state)
+		if (!dev->power.prev_state.event)
 			resume_device(dev);
 		down(&dpm_list_sem);
 		put_device(dev);
Index: drivers/base/power/runtime.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/runtime.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/runtime.c  (mode:100644)
@@ -13,10 +13,10 @@
 static void runtime_resume(struct device * dev)
 {
 	dev_dbg(dev, "resuming\n");
-	if (!dev->power.power_state)
+	if (!dev->power.power_state.event)
 		return;
 	if (!resume_device(dev))
-		dev->power.power_state = 0;
+		dev->power.power_state = PMSG_ON;
 }
 
 
@@ -49,10 +49,10 @@
 	int error = 0;
 
 	down(&dpm_sem);
-	if (dev->power.power_state == state)
+	if (dev->power.power_state.event == state.event)
 		goto Done;
 
-	if (dev->power.power_state)
+	if (dev->power.power_state.event)
 		runtime_resume(dev);
 
 	if (!(error = suspend_device(dev, state)))
Index: drivers/base/power/suspend.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/suspend.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/suspend.c  (mode:100644)
@@ -39,22 +39,22 @@
 {
 	int error = 0;
 
-	if (dev->power.power_state) {
+	if (dev->power.power_state.event) {
 		dev_dbg(dev, "PM: suspend %d-->%d\n",
-			dev->power.power_state, state);
+			dev->power.power_state.event, state.event);
 	}
 	if (dev->power.pm_parent
-			&& dev->power.pm_parent->power.power_state) {
+			&& dev->power.pm_parent->power.power_state.event) {
 		dev_err(dev,
 			"PM: suspend %d->%d, parent %s already %d\n",
-			dev->power.power_state, state,
+			dev->power.power_state.event, state.event,
 			dev->power.pm_parent->bus_id,
-			dev->power.pm_parent->power.power_state);
+			dev->power.pm_parent->power.power_state.event);
 	}
 
 	dev->power.prev_state = dev->power.power_state;
 
-	if (dev->bus && dev->bus->suspend && !dev->power.power_state) {
+	if (dev->bus && dev->bus->suspend && !dev->power.power_state.event) {
 		dev_dbg(dev, "suspending\n");
 		error = dev->bus->suspend(dev, state);
 	}
Index: drivers/base/power/sysfs.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/sysfs.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/sysfs.c  (mode:100644)
@@ -26,19 +26,19 @@
 
 static ssize_t state_show(struct device * dev, char * buf)
 {
-	return sprintf(buf, "%u\n", dev->power.power_state);
+	return sprintf(buf, "%u\n", dev->power.power_state.event);
 }
 
 static ssize_t state_store(struct device * dev, const char * buf, size_t n)
 {
-	u32 state;
+	pm_message_t state;
 	char * rest;
 	int error = 0;
 
-	state = simple_strtoul(buf, &rest, 10);
+	state.event = simple_strtoul(buf, &rest, 10);
 	if (*rest)
 		return -EINVAL;
-	if (state)
+	if (state.event)
 		error = dpm_runtime_suspend(dev, state);
 	else
 		dpm_runtime_resume(dev);
Index: drivers/ide/ide.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/ide/ide.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/ide/ide.c  (mode:100644)
@@ -1385,7 +1385,7 @@
 	rq.special = &args;
 	rq.pm = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_suspend;
-	rqpm.pm_state = state;
+	rqpm.pm_state = state.event;
 
 	return ide_do_drive_cmd(drive, &rq, ide_wait);
 }
@@ -1404,7 +1404,7 @@
 	rq.special = &args;
 	rq.pm = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_resume;
-	rqpm.pm_state = 0;
+	rqpm.pm_state = PM_EVENT_ON;
 
 	return ide_do_drive_cmd(drive, &rq, ide_head_wait);
 }
Index: drivers/pci/pci.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/pci/pci.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/pci/pci.c  (mode:100644)
@@ -316,14 +316,14 @@
 
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
 {
-	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
+	switch (state.event) {
+	case PM_EVENT_ON:
 		return PCI_D0;
-
-	switch (state) {
-	case 0: return PCI_D0;
-	case 3: return PCI_D3hot;
-	default:
-		printk("They asked me for state %d\n", state);
+	case PM_EVENT_FREEZE:
+	case PM_EVENT_SUSPEND:
+		return PCI_D3hot;
+	default: 
+		printk("They asked me for state %d\n", state.event);
 		BUG();
 	}
 	return PCI_D0;
Index: drivers/serial/pmac_zilog.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/serial/pmac_zilog.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/serial/pmac_zilog.c  (mode:100644)
@@ -1601,7 +1601,7 @@
 		return 0;
 	}
 
-	if (pm_state == mdev->ofdev.dev.power.power_state || pm_state < 2)
+	if (pm_state.event == mdev->ofdev.dev.power.power_state.event)
 		return 0;
 
 	pmz_debug("suspend, switching to state %d\n", pm_state);
Index: drivers/usb/core/hub.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/core/hub.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/core/hub.c  (mode:100644)
@@ -1564,7 +1564,7 @@
 			struct usb_driver	*driver;
 
 			intf = udev->actconfig->interface[i];
-			if (state <= intf->dev.power.power_state)
+			if (state.event <= intf->dev.power.power_state.event)
 				continue;
 			if (!intf->dev.driver)
 				continue;
@@ -1572,11 +1572,11 @@
 
 			if (driver->suspend) {
 				status = driver->suspend(intf, state);
-				if (intf->dev.power.power_state != state
+				if (intf->dev.power.power_state.event != state.event
 						|| status)
 					dev_err(&intf->dev,
 						"suspend %d fail, code %d\n",
-						state, status);
+						state.event, status);
 			}
 
 			/* only drivers with suspend() can ever resume();
@@ -1589,7 +1589,7 @@
 			 * since we know every driver's probe/disconnect works
 			 * even for drivers that can't suspend.
 			 */
-			if (!driver->suspend || state > PM_SUSPEND_MEM) {
+			if (!driver->suspend || state.event > PM_EVENT_FREEZE) {
 #if 1
 				dev_warn(&intf->dev, "resume is unsafe!\n");
 #else
@@ -1610,7 +1610,7 @@
 	 * policies (when HNP doesn't apply) once we have mechanisms to
 	 * turn power back on!  (Likely not before 2.7...)
 	 */
-	if (state > PM_SUSPEND_MEM) {
+	if (state.event > PM_EVENT_FREEZE) {
 		dev_warn(&udev->dev, "no poweroff yet, suspending instead\n");
 	}
 
@@ -1727,7 +1727,7 @@
 			struct usb_driver	*driver;
 
 			intf = udev->actconfig->interface[i];
-			if (intf->dev.power.power_state == PMSG_SUSPEND)
+			if (intf->dev.power.power_state.event == PM_EVENT_ON)
 				continue;
 			if (!intf->dev.driver) {
 				/* FIXME maybe force to alt 0 */
@@ -1741,11 +1741,11 @@
 
 			/* can we do better than just logging errors? */
 			status = driver->resume(intf);
-			if (intf->dev.power.power_state != PMSG_ON
+			if (intf->dev.power.power_state.event != PM_EVENT_ON
 					|| status)
 				dev_dbg(&intf->dev,
 					"resume fail, state %d code %d\n",
-					intf->dev.power.power_state, status);
+					intf->dev.power.power_state.event, status);
 		}
 		status = 0;
 
@@ -1928,7 +1928,7 @@
 	unsigned		port1;
 	int			status;
 
-	if (intf->dev.power.power_state == PM_SUSPEND_ON)
+	if (intf->dev.power.power_state.event == PM_EVENT_ON)
 		return 0;
 
 	for (port1 = 1; port1 <= hdev->maxchild; port1++) {
Index: drivers/usb/core/usb.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/core/usb.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/core/usb.c  (mode:100644)
@@ -1389,7 +1389,7 @@
 	driver = to_usb_driver(dev->driver);
 
 	/* there's only one USB suspend state */
-	if (intf->dev.power.power_state)
+	if (intf->dev.power.power_state.event)
 		return 0;
 
 	if (driver->suspend)
Index: drivers/usb/host/ehci-dbg.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/ehci-dbg.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/ehci-dbg.c  (mode:100644)
@@ -641,7 +641,7 @@
 
 	spin_lock_irqsave (&ehci->lock, flags);
 
-	if (bus->controller->power.power_state) {
+	if (bus->controller->power.power_state.event) {
 		size = scnprintf (next, size,
 			"bus %s, device %s (driver " DRIVER_VERSION ")\n"
 			"SUSPENDED (no register access)\n",
Index: drivers/usb/host/ohci-dbg.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/ohci-dbg.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/ohci-dbg.c  (mode:100644)
@@ -631,7 +631,7 @@
 		hcd->product_desc,
 		hcd_name);
 
-	if (bus->controller->power.power_state) {
+	if (bus->controller->power.power_state.event) {
 		size -= scnprintf (next, size,
 			"SUSPENDED (no register access)\n");
 		goto done;
Index: drivers/usb/host/sl811-hcd.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/sl811-hcd.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/sl811-hcd.c  (mode:100644)
@@ -1781,9 +1781,9 @@
 	if (phase != SUSPEND_POWER_DOWN)
 		return retval;
 
-	if (state <= PM_SUSPEND_MEM)
+	if (state.event == PM_EVENT_FREEZE)
 		retval = sl811h_hub_suspend(hcd);
-	else
+	else if (state.event == PM_EVENT_SUSPEND)
 		port_power(sl811, 0);
 	if (retval == 0)
 		dev->power.power_state = state;
@@ -1802,7 +1802,7 @@
 	/* with no "check to see if VBUS is still powered" board hook,
 	 * let's assume it'd only be powered to enable remote wakeup.
 	 */
-	if (dev->power.power_state > PM_SUSPEND_MEM
+	if (dev->power.power_state.event == PM_EVENT_SUSPEND
 			|| !hcd->can_wakeup) {
 		sl811->port1 = 0;
 		port_power(sl811, 1);
Index: drivers/video/aty/atyfb_base.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/aty/atyfb_base.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/aty/atyfb_base.c  (mode:100644)
@@ -2071,12 +2071,12 @@
 	struct fb_info *info = pci_get_drvdata(pdev);
 	struct atyfb_par *par = (struct atyfb_par *) info->par;
 
-	if (pdev->dev.power.power_state == 0)
+	if (pdev->dev.power.power_state.event == PM_EVENT_ON)
 		return 0;
 
 	acquire_console_sem();
 
-	if (pdev->dev.power.power_state == 2)
+	if (pdev->dev.power.power_state.event == 2)
 		aty_power_mgmt(0, par);
 	par->asleep = 0;
 
Index: drivers/video/aty/radeon_pm.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/aty/radeon_pm.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/aty/radeon_pm.c  (mode:100644)
@@ -2526,18 +2526,18 @@
         struct radeonfb_info *rinfo = info->par;
 	int i;
 
-	if (state == pdev->dev.power.power_state)
+	if (state.event == pdev->dev.power.power_state.event)
 		return 0;
 
 	printk(KERN_DEBUG "radeonfb (%s): suspending to state: %d...\n",
-	       pci_name(pdev), state);
+	       pci_name(pdev), state.event);
 
 	/* For suspend-to-disk, we cheat here. We don't suspend anything and
 	 * let fbcon continue drawing until we are all set. That shouldn't
 	 * really cause any problem at this point, provided that the wakeup
 	 * code knows that any state in memory may not match the HW
 	 */
-	if (state != PM_SUSPEND_MEM)
+	if (state.event == PM_EVENT_FREEZE)
 		goto done;
 
 	acquire_console_sem();
@@ -2616,7 +2616,7 @@
         struct radeonfb_info *rinfo = info->par;
 	int rc = 0;
 
-	if (pdev->dev.power.power_state == 0)
+	if (pdev->dev.power.power_state.event == PM_EVENT_ON)
 		return 0;
 
 	if (rinfo->no_schedule) {
@@ -2626,7 +2626,7 @@
 		acquire_console_sem();
 
 	printk(KERN_DEBUG "radeonfb (%s): resuming from state: %d...\n",
-	       pci_name(pdev), pdev->dev.power.power_state);
+	       pci_name(pdev), pdev->dev.power.power_state.event);
 
 
 	if (pci_enable_device(pdev)) {
@@ -2637,7 +2637,7 @@
 	}
 	pci_set_master(pdev);
 
-	if (pdev->dev.power.power_state == PM_SUSPEND_MEM) {
+	if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
 		/* Wakeup chip. Check from config space if we were powered off
 		 * (todo: additionally, check CLK_PIN_CNTL too)
 		 */
Index: drivers/video/i810/i810_main.c
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/i810/i810_main.c  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/i810/i810_main.c  (mode:100644)
@@ -1506,12 +1506,12 @@
 	struct i810fb_par *par = (struct i810fb_par *) info->par;
 	int blank = 0, prev_state = par->cur_state;
 
-	if (state == prev_state)
+	if (state.event == prev_state)
 		return 0;
 
-	par->cur_state = state;
+	par->cur_state = state.event;
 
-	switch (state) {
+	switch (state.event) {
 	case 1:
 		blank = VESA_VSYNC_SUSPEND;
 		break;
Index: include/linux/pm.h
===================================================================
--- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/include/linux/pm.h  (mode:100644)
+++ 24a21ff5302734d40e08c400b14c0c1624cceded/include/linux/pm.h  (mode:100644)
@@ -185,7 +185,9 @@
 
 struct device;
 
-typedef u32 __bitwise pm_message_t;
+typedef struct pm_message {
+	int event;
+} pm_message_t;
 
 /*
  * There are 4 important states driver can be in:
@@ -205,9 +207,13 @@
  * or something similar soon.
  */
 
-#define PMSG_FREEZE	((__force pm_message_t) 3)
-#define PMSG_SUSPEND	((__force pm_message_t) 3)
-#define PMSG_ON		((__force pm_message_t) 0)
+#define PM_EVENT_ON 0
+#define PM_EVENT_FREEZE 1
+#define PM_EVENT_SUSPEND 2
+
+#define PMSG_FREEZE	({struct pm_message m; m.event = PM_EVENT_FREEZE; m; })
+#define PMSG_SUSPEND	({struct pm_message m; m.event = PM_EVENT_SUSPEND; m; })
+#define PMSG_ON		({struct pm_message m; m.event = PM_EVENT_ON; m; })
 
 struct dev_pm_info {
 	pm_message_t		power_state;

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Thu, 2005-06-09 at 02:04 +0200, Pavel Machek wrote:
> Hi!
> 
> > > > I think we should also use the pm_message_t defines.  We will need to
> > > > add PMSG_FREEZE eventually.  I decided to default to the current state
> > > > rather than panic.  Does this patch look ok?
> > > 
> > > No.
> > 
> > Hrm... I don't follow you anymore here ...
> > 
> > >         case PM_EVENT_ON:
> > >                 return PCI_D0;
> > >         case PM_EVENT_FREEZE:
> > >         case PM_EVENT_SUSPEND:
> > >                 return PCI_D3hot;
> > 
> > What are these new PM_EVENT_* things ? I though we defined PMSG_* ?
> 
> PMSG_* are for struct; you can't case on struct.
> 
> > > You passed invalid argument; I see no reason why you should paper over
> > > it and risk continuing. This happens during system suspend; it is
> > > quite possible that user will not see your printk when machine powers
> > > off just after that; and remember that it will not be in syslog after
> > > resume.
> > 
> > Crap. I don't think a BUG() makes any useful help neither in this place,
> > and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
> > in there (and I wonder in how many other places).
> 
> At least you can see & report that error... That would not be a case
> for simple printk.

Well not exactly.  The video device may have already been disabled, so
the user wouldn't see it anyway.  The reason I don't like BUG() here is
that it's very unlikely passing an invalid PMSG will have serious
consequences.  The problems are likely less significant than those
caused by calling BUG().  Many suspend routines don't set power at all
yet (e.g. cardbus) and it still works out fine.  Defaulting to the
current state seems like a very safe behavior.  Then, after resume, we
can see the messages.  I'd like to see PM just work, and we have to be
careful during an API change.

> 
> This is the patch I'd like to go in. I hope it makes it
> clear... Please base your development on top of this one...

Yeah, I wasn't sure where those flags were coming from...

Thanks,
Adam


> 									Pavel
> 
> ---
> 
> Turn pm_message_t into struct, so that it is typechecked properly (and
> so that we can add flags field in future). This should not go in
> before 2.6.12.

What do you have in mind for adding to the structure in the future?

Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

Hi!

> > > >         case PM_EVENT_ON:
> > > >                 return PCI_D0;
> > > >         case PM_EVENT_FREEZE:
> > > >         case PM_EVENT_SUSPEND:
> > > >                 return PCI_D3hot;
> > > 
> > > What are these new PM_EVENT_* things ? I though we defined PMSG_* ?
> > 
> > PMSG_* are for struct; you can't case on struct.
> > 
> > > > You passed invalid argument; I see no reason why you should paper over
> > > > it and risk continuing. This happens during system suspend; it is
> > > > quite possible that user will not see your printk when machine powers
> > > > off just after that; and remember that it will not be in syslog after
> > > > resume.
> > > 
> > > Crap. I don't think a BUG() makes any useful help neither in this place,
> > > and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
> > > in there (and I wonder in how many other places).
> > 
> > At least you can see & report that error... That would not be a case
> > for simple printk.
> 
> Well not exactly.  The video device may have already been disabled, so
> the user wouldn't see it anyway.  The reason I don't like BUG() here is
> that it's very unlikely passing an invalid PMSG will have serious
> consequences.  The problems are likely less significant than those
> caused by calling BUG().  Many suspend routines don't set power at all
> yet (e.g. cardbus) and it still works out fine.  Defaulting to the
> current state seems like a very safe behavior.  Then, after resume, we
> can see the messages.  I'd like to see PM just work, and we have to be
> careful during an API change.

Ok, refaulting to current state seems pretty good. But you are not
going to see the messages after the resume; not in swsusp if they
happened after swsusp atomic snapshot. In recent swsusp, I'm carefull
not to blank consoles when I can avoid it, exactly so messages like
this can be seen.

> > Turn pm_message_t into struct, so that it is typechecked properly (and
> > so that we can add flags field in future). This should not go in
> > before 2.6.12.
> 
> What do you have in mind for adding to the structure in the future?

*Maybe* flags telling drivers details of transition will be
needed. And I thought "partial suspend" people will want stuff like
char * "enter state with disk spinning at at most 300 rpm" to be added
there, too.

Now its mostly for typechecking.
								Pavel

Re: [PATCH] fix tulip suspend/resume

[Nigel Cunningham]

Hi.

On Thu, 2005-06-09 at 10:04, Pavel Machek wrote:
> PMSG_* are for struct; you can't case on struct.

switch (state.event)

Regards,

Nigel

Re: [PATCH] fix tulip suspend/resume

[Karsten Keil]

Hi,

On Thu, Jun 09, 2005 at 02:04:02AM +0200, Pavel Machek wrote:
...
> 
> > > You passed invalid argument; I see no reason why you should paper over
> > > it and risk continuing. This happens during system suspend; it is
> > > quite possible that user will not see your printk when machine powers
> > > off just after that; and remember that it will not be in syslog after
> > > resume.
> > 
> > Crap. I don't think a BUG() makes any useful help neither in this place,
> > and when I locally turn PMSG_FREEZE to something sane I suddenly blow up
> > in there (and I wonder in how many other places).
> 
> At least you can see & report that error... That would not be a case
> for simple printk.
> 

Yes, but BUG() should not used for that, IMHO BUG() should be used only for
critical situation, in which you cannot do anything and here is danger to
lost data, I don't think this is the case here, but the BUG() will crash the
machine and so you might be loose data, which is not nice to do, for only to
request a report. Yes printk only is ignored by many people, but here are
always some, who will report it.

-- 
Karsten Keil
SuSE Labs
ISDN development

Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

Hi!

> > pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
> > {
> > 	if (!pci_find_capability(dev, PCI_CAP_ID_PM))
> > 		return PCI_D0;
> > 
> > 	switch (state) {
> > 	case 0: return PCI_D0;
> > 	case 3: return PCI_D3hot;
> > 	default:
> > 		printk("They asked me for state %d\n", state);
> > 		BUG();
> > 	}
> > 	return PCI_D0;
> > }
> 
> Gack ! I need to remember to fix that one before I change PMSG_FREEZE
> definition to be different than PMSG_SUSPEND upstream.
> 
> Pavel, do you know that there are other ways to deal with errors than
> just BUG()'ing all over the place ? :)

And how would you propose to deal this this error? PCI_ERROR probably
needs to be invented (for acpi people), but I do not think I want
callers to check for it.
								Pavel

Re: [PATCH] fix tulip suspend/resume

[Karsten Keil]

On Tue, Jun 07, 2005 at 04:58:00PM -0400, Adam Belay wrote:
> On Tue, Jun 07, 2005 at 12:55:52PM +0200, Karsten Keil wrote:
> > On Mon, Jun 06, 2005 at 10:50:55PM -0400, Adam Belay wrote:
> > > On Mon, Jun 06, 2005 at 05:04:07PM -0700, Linus Torvalds wrote:
> > > > 
> > > > Jeff, 
> > > >  this looks ok, but I'll leave the decision to you. Things like this often 
> > > > break.
> > > > 
> > > > Andrew, maybe at least a few days in -mm to see if there's some outcry?
> > > > 
> > > > 		Linus
> > > 
> > > This patch is an improvement, but there may still be some issues.
> > > Specifically, it looks to me like the the interrupt handler remains
> > > registered.  This could cause some problems when another device is sharing
> > > the interrupt because the tulip driver must read from a hardware register
> > > to determine if it triggered the interrupt. When the hardware has been
> > > physically powered off, things might not go well.
> > 
> > No, I also looked into this, it is not needed for the tulip driver, it
> > detects that it has no access to the hardware (reading 0xffffffff) in the
> > interrupt functions.
> 
> Hmm, doesn't look like it:
> 
> >	/* Let's see whether the interrupt really is for us */
> >	csr5 = ioread32(ioaddr + CSR5);
> >
> >	if (tp->flags & HAS_PHY_IRQ) 
> >	        handled = phy_interrupt (dev);
> >    
> >	if ((csr5 & (NormalIntr|AbnormalIntr)) == 0)
> >		return IRQ_RETVAL(handled);
> 
> Upon reading the value 0xffffffff for csr5, the if statement would be false.
> Maybe it bails out later or something, but really why risk it?  As Ben noted,
> it's easy to have race conditions between power off and interrupt handler
> routines.  Also, there is a transition delay between D0 and D3.  I prefer to
> play it safe and call free_irq().

Yes it test later for 0xffffffff, but you are right, freeing the IRQ would
be safer.
...
> > 
> > I think restoring the PCI config should be done always, not only if the
> > device was in the up state, also powerup should be done.
> > If not you will run into problems to use the device later.
> 
> Yeah, I think you're right.
> 
> Also, after looking at some other netdev suspend routines, it appears that
> "netif_device_present" isn't necessary.  Jeff, is this correct?
> 
> Thanks,
> Adam
> 
> 
> How does this patch look:

Looks OK and also work on my HW.

Andrew, can you please take this updated version, it is much cleaner and
safer, because it handle the IRQ correctly.

> 
> --- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
> +++ b/drivers/net/tulip/tulip_core.c	2005-06-07 16:34:13.641177456 -0400
> @@ -1756,11 +1756,19 @@
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
>  
> -	if (dev && netif_running (dev) && netif_device_present (dev)) {
> -		netif_device_detach (dev);
> -		tulip_down (dev);
> -		/* pci_power_off(pdev, -1); */
> -	}
> +	if (!dev)
> +		return -EINVAL;
> +
> +	if (netif_running(dev))
> +		tulip_down(dev);
> +
> +	netif_device_detach(dev);
> +	pci_save_state(pdev);
> +
> +	free_irq(dev->irq, dev);
> +	pci_disable_device(pdev);
> +	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
>  	return 0;
>  }
>  
> @@ -1768,15 +1776,26 @@
>  static int tulip_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int retval;
>  
> -	if (dev && netif_running (dev) && !netif_device_present (dev)) {
> -#if 1
> -		pci_enable_device (pdev);
> -#endif
> -		/* pci_power_on(pdev); */
> -		tulip_up (dev);
> -		netif_device_attach (dev);
> +	if (!dev)
> +		return -EINVAL;
> +
> +	pci_set_power_state(pdev, PCI_D0);
> +	pci_restore_state(pdev);
> +
> +	pci_enable_device(pdev);
> +
> +	if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
> +		printk (KERN_ERR "tulip: request_irq failed in resume\n");
> +		return retval;
>  	}
> +
> +	netif_device_attach(dev);
> +
> +	if (netif_running(dev))
> +		tulip_up(dev);
> +
>  	return 0;
>  }
>  

-- 
Karsten Keil
SuSE Labs
ISDN development

Re: [PATCH] fix tulip suspend/resume

[Davide Rossetti]

>Looks OK and also work on my HW.
>
>Andrew, can you please take this updated version, it is much cleaner and
>safer, because it handle the IRQ correctly.
>
>  
>
>>--- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
>>+++ b/drivers/net/tulip/tulip_core.c	2005-06-07 16:34:13.641177456 -0400
>>@@ -1756,11 +1756,19 @@
>> {
>> 	struct net_device *dev = pci_get_drvdata(pdev);
>> 
>>-	if (dev && netif_running (dev) && netif_device_present (dev)) {
>>-		netif_device_detach (dev);
>>-		tulip_down (dev);
>>-		/* pci_power_off(pdev, -1); */
>>-	}
>>+	if (!dev)
>>+		return -EINVAL;
>>+
>>+	if (netif_running(dev))
>>+		tulip_down(dev);
>>+
>>+	netif_device_detach(dev);
>>+	pci_save_state(pdev);
>>+
>>+	free_irq(dev->irq, dev);
>>    
>>
isn't it safer to swap these last two guys ?? in theory there could be 
an IRQ betwen pci_save_state() and free_irq()...

>>+	pci_disable_device(pdev);
>>+	pci_set_power_state(pdev, pci_choose_state(pdev, state));
>>+
>> 	return 0;
>> }
>> 
>>    
>>
regards

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Wed, Jun 08, 2005 at 08:11:36PM +0200, Davide Rossetti wrote:
> 
> >Looks OK and also work on my HW.
> >
> >Andrew, can you please take this updated version, it is much cleaner and
> >safer, because it handle the IRQ correctly.
> >
> > 
> >
> >>--- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
> >>+++ b/drivers/net/tulip/tulip_core.c	2005-06-07 16:34:13.641177456 -0400
> >>@@ -1756,11 +1756,19 @@
> >>{
> >>	struct net_device *dev = pci_get_drvdata(pdev);
> >>
> >>-	if (dev && netif_running (dev) && netif_device_present (dev)) {
> >>-		netif_device_detach (dev);
> >>-		tulip_down (dev);
> >>-		/* pci_power_off(pdev, -1); */
> >>-	}
> >>+	if (!dev)
> >>+		return -EINVAL;
> >>+
> >>+	if (netif_running(dev))
> >>+		tulip_down(dev);
> >>+
> >>+	netif_device_detach(dev);
> >>+	pci_save_state(pdev);
> >>+
> >>+	free_irq(dev->irq, dev);
> >>   
> >>
> isn't it safer to swap these last two guys ?? in theory there could be 
> an IRQ betwen pci_save_state() and free_irq()...

Well, so not exactly.  The issue here was that we might access the hardware
while it's:
a.) physically powered down
b.) making a power transition

In this case, the device is still powered on, so there shouldn't be a problem.
However, changing the order shouldn't break anything.  If you think it would
be cleaner this way and someone is willing to confirm that it still works,
then we can change it to the patch below.

Thanks,
Adam

Here's an updated patch:


[PM] fix suspend/resume for tulip based cards

This patch allows the tulip driver to suspend and resume properly.  It was
originally written by Karsten Keil and then modified by Adam Belay.

Signed-off-by: Karsten Keil <kkeil@suse.de>
Signed-off-by: Adam Belay <abelay@novell.com>

--- a/drivers/net/tulip/tulip_core.c	2005-05-27 22:06:00.000000000 -0400
+++ b/drivers/net/tulip/tulip_core.c	2005-06-08 21:28:26.111797528 -0400
@@ -1756,11 +1756,19 @@
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
 
-	if (dev && netif_running (dev) && netif_device_present (dev)) {
-		netif_device_detach (dev);
-		tulip_down (dev);
-		/* pci_power_off(pdev, -1); */
-	}
+	if (!dev)
+		return -EINVAL;
+
+	if (netif_running(dev))
+		tulip_down(dev);
+
+	netif_device_detach(dev);
+	free_irq(dev->irq, dev);
+
+	pci_save_state(pdev);
+	pci_disable_device(pdev);
+	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+
 	return 0;
 }
 
@@ -1768,15 +1776,26 @@
 static int tulip_resume(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	int retval;
 
-	if (dev && netif_running (dev) && !netif_device_present (dev)) {
-#if 1
-		pci_enable_device (pdev);
-#endif
-		/* pci_power_on(pdev); */
-		tulip_up (dev);
-		netif_device_attach (dev);
+	if (!dev)
+		return -EINVAL;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+
+	pci_enable_device(pdev);
+
+	if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
+		printk (KERN_ERR "tulip: request_irq failed in resume\n");
+		return retval;
 	}
+
+	netif_device_attach(dev);
+
+	if (netif_running(dev))
+		tulip_up(dev);
+
 	return 0;
 }
 

Re: [PATCH] fix tulip suspend/resume

[Stefan Seyfried]

Linus Torvalds wrote:
> Jeff, 
>  this looks ok, but I'll leave the decision to you. Things like this often 
> break.
> 
> Andrew, maybe at least a few days in -mm to see if there's some outcry?

This is a good idea.
This one was broken for a real long time, so some more days cannot hurt ;-)
IIRC it only happened on cardbus cards (but i may have been just lucky)
which you can just pull out and put back in again after resume.
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

On Tue, 2005-06-07 at 00:46 +0200, Karsten Keil wrote:
> Hi,
> 
> following patch fix the suspend/resume for tulip based
> cards, so suspend on disk work now for me and tulip based
> cardbus cards.
> 
> 
> Signed-off-by: Karsten Keil <kkeil@suse.de>
> 
> --- linux/drivers/net/tulip/tulip_core.c.orig	2005-03-23 23:54:43.000000000 +0100
> +++ linux/drivers/net/tulip/tulip_core.c	2005-05-26 17:29:14.000000000 +0200
> @@ -1755,12 +1755,16 @@
>  static int tulip_suspend (struct pci_dev *pdev, pm_message_t state)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int err;
>  
> +	pci_save_state(pdev);
>  	if (dev && netif_running (dev) && netif_device_present (dev)) {
>  		netif_device_detach (dev);
>  		tulip_down (dev);
>  		/* pci_power_off(pdev, -1); */
>  	}
> +	if ((err = pci_set_power_state(pdev, PCI_D3hot)))
> +		printk(KERN_ERR "%s: pci_set_power_state D3hot return %d\n", dev->name, err);
>  	return 0;
>  }

It should probably test for message state, it's not worth doing
pci_set_power_state(D3) if PMSG_FREEZE is passed... (just slows down
suspend to disk)

> @@ -1768,7 +1772,11 @@
>  static int tulip_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	int err;
>  
> +	if ((err = pci_set_power_state(pdev, PCI_D0)))
> +		printk(KERN_ERR "%s: pci_set_power_state D0 return %d\n", dev->name, err);
> +	pci_restore_state(pdev);
>  	if (dev && netif_running (dev) && !netif_device_present (dev)) {
>  #if 1
>  		pci_enable_device (pdev);
> 

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, Jun 07, 2005 at 12:15:44PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2005-06-07 at 00:46 +0200, Karsten Keil wrote:
> > Hi,
> > 
> > following patch fix the suspend/resume for tulip based
> > cards, so suspend on disk work now for me and tulip based
> > cardbus cards.
> > 
> > 
> > Signed-off-by: Karsten Keil <kkeil@suse.de>
> > 
> > --- linux/drivers/net/tulip/tulip_core.c.orig	2005-03-23 23:54:43.000000000 +0100
> > +++ linux/drivers/net/tulip/tulip_core.c	2005-05-26 17:29:14.000000000 +0200
> > @@ -1755,12 +1755,16 @@
> >  static int tulip_suspend (struct pci_dev *pdev, pm_message_t state)
> >  {
> >  	struct net_device *dev = pci_get_drvdata(pdev);
> > +	int err;
> >  
> > +	pci_save_state(pdev);
> >  	if (dev && netif_running (dev) && netif_device_present (dev)) {
> >  		netif_device_detach (dev);
> >  		tulip_down (dev);
> >  		/* pci_power_off(pdev, -1); */
> >  	}
> > +	if ((err = pci_set_power_state(pdev, PCI_D3hot)))
> > +		printk(KERN_ERR "%s: pci_set_power_state D3hot return %d\n", dev->name, err);
> >  	return 0;
> >  }
> 
> It should probably test for message state, it's not worth doing
> pci_set_power_state(D3) if PMSG_FREEZE is passed... (just slows down
> suspend to disk)

Yeah, I added pci_choose_state in my last email.  This will at least help
avoid powering off.  Still, I agree this needs to be handled specifically.
Currently, I don't think many drivers support PMSG_FREEZE.

Thanks,
Adam

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> > It should probably test for message state, it's not worth doing
> > pci_set_power_state(D3) if PMSG_FREEZE is passed... (just slows down
> > suspend to disk)
> 
> Yeah, I added pci_choose_state in my last email.  This will at least help
> avoid powering off.  Still, I agree this needs to be handled specifically.
> Currently, I don't think many drivers support PMSG_FREEZE.

Nope, but I've been improving swsusp support on macs lately and have
already a bunch of driver fixes waiting.

Now I need to get Pavel, Patrick and I to agree about the PM toplevel
core changes before I can send all that stuff to Andrew :)

Ben.

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, 2005-06-07 at 13:32 +1000, Benjamin Herrenschmidt wrote:
> > > It should probably test for message state, it's not worth doing
> > > pci_set_power_state(D3) if PMSG_FREEZE is passed... (just slows down
> > > suspend to disk)
> > 
> > Yeah, I added pci_choose_state in my last email.  This will at least help
> > avoid powering off.  Still, I agree this needs to be handled specifically.
> > Currently, I don't think many drivers support PMSG_FREEZE.
> 
> Nope, but I've been improving swsusp support on macs lately and have
> already a bunch of driver fixes waiting.
> 
> Now I need to get Pavel, Patrick and I to agree about the PM toplevel
> core changes before I can send all that stuff to Andrew :)
> 
> Ben.

What PM toplevel core changes are you referring to?  I've look over the
changes to pm_ops and they seem to make sense.  Still I almost wonder if
we should make the entire thing arch specific code, and then have this
code call things like device_suspend().  If mac hardware required that
many new hooks, then other platforms might require even more.

Thanks,
Adam

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

> What PM toplevel core changes are you referring to?  I've look over the
> changes to pm_ops and they seem to make sense.  Still I almost wonder if
> we should make the entire thing arch specific code, and then have this
> code call things like device_suspend().  If mac hardware required that
> many new hooks, then other platforms might require even more.

That is exactly the debate. Patrick thinks the whole thing should be
arch code and kernel/power/* just provices "library" routines to call
(like the freezer, swsusp stuff, etc...), Pavel wants to share as much
code as possible in a single place.

I have no real strong preference, I tend to be a bit more on Patrick's
side here. I can do either way, but we need to decide. On one case, I
would do a patch removing most of kernel/power/main.c and disk.c (they
are mostly redundant anyway) and replacing with a simple mecanism where
the arch provides a table of state names + function to call for sysfs.
On the other case, just merge my patch adding all the new hooks.

Ben.

Re: [PATCH] fix tulip suspend/resume

[Adam Belay]

On Tue, 2005-06-07 at 14:29 +1000, Benjamin Herrenschmidt wrote:
> > What PM toplevel core changes are you referring to?  I've look over the
> > changes to pm_ops and they seem to make sense.  Still I almost wonder if
> > we should make the entire thing arch specific code, and then have this
> > code call things like device_suspend().  If mac hardware required that
> > many new hooks, then other platforms might require even more.
> 
> That is exactly the debate. Patrick thinks the whole thing should be
> arch code and kernel/power/* just provices "library" routines to call
> (like the freezer, swsusp stuff, etc...), Pavel wants to share as much
> code as possible in a single place.
> 
> I have no real strong preference, I tend to be a bit more on Patrick's
> side here. I can do either way, but we need to decide. On one case, I
> would do a patch removing most of kernel/power/main.c and disk.c (they
> are mostly redundant anyway) and replacing with a simple mecanism where
> the arch provides a table of state names + function to call for sysfs.
> On the other case, just merge my patch adding all the new hooks.
> 
> Ben.

I'd tend to agree with Pat then.  The original pm_ops seem to be
designed around ACPI and after looking at the spec I don't think they're
correct. (e.g. it looks like _PTS and _GTS should be run after
device_suspend() instead of before, so *prepare may not be needed).  In
short, this tends to be tricky.  It's probably best to have platforms
handle it on their own with kernel/power as a library.

Thanks,
Adam

Re: [PATCH] fix tulip suspend/resume

[Nigel Cunningham]

Hi.

On Tue, 2005-06-07 at 15:03, Adam Belay wrote:
> I'd tend to agree with Pat then.  The original pm_ops seem to be
> designed around ACPI and after looking at the spec I don't think they're
> correct. (e.g. it looks like _PTS and _GTS should be run after
> device_suspend() instead of before, so *prepare may not be needed).  In
> short, this tends to be tricky.  It's probably best to have platforms
> handle it on their own with kernel/power as a library.

Hmm....

- We need a lot of arch dependent hooks so the right things are done at
the right time
- We need a lot of arch independent code run between the arch dependent
code so that the right things are done at the right time.

Therefore it doesn't matter whether the centre of the universe is arch
dependent or independent - either way we have to have hooks to get the
other stuff done.

Swsusp takes a minimalist design, so it doesn't look like much to worry
about if some of that code gets duplicated in arch specific places (taht
said, I'm not sure what whoever-it-was was thinking of duplicating).
Suspend2 takes a feature complete/user friendly/etc design and therefore
has a lot more arch independent code. We definitely want to minimise
duplication of code there.

So I'd suggest leaving the arch independent code in the drivers seat,
but simultaneously replacing/redesigning pm_ops & swsusp so that all the
ops that are or might be needed are there and called at the appropriate
time.

Regards,

Nigel

Re: [PATCH] fix tulip suspend/resume

[Benjamin Herrenschmidt]

On Tue, 2005-06-07 at 15:51 +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Tue, 2005-06-07 at 15:03, Adam Belay wrote:
> > I'd tend to agree with Pat then.  The original pm_ops seem to be
> > designed around ACPI and after looking at the spec I don't think they're
> > correct. (e.g. it looks like _PTS and _GTS should be run after
> > device_suspend() instead of before, so *prepare may not be needed).  In
> > short, this tends to be tricky.  It's probably best to have platforms
> > handle it on their own with kernel/power as a library.
> 
> Hmm....
> 
> - We need a lot of arch dependent hooks so the right things are done at
> the right time
> - We need a lot of arch independent code run between the arch dependent
> code so that the right things are done at the right time.
> 
> Therefore it doesn't matter whether the centre of the universe is arch
> dependent or independent - either way we have to have hooks to get the
> other stuff done.

Except that one direction is hooks, the other is normal function calls,
that is, one is up layer with hook to low level the other is up layer
calling library to low level, I prefer the later.
 
> Swsusp takes a minimalist design, so it doesn't look like much to worry
> about if some of that code gets duplicated in arch specific places (taht
> said, I'm not sure what whoever-it-was was thinking of duplicating).
> Suspend2 takes a feature complete/user friendly/etc design and therefore
> has a lot more arch independent code. We definitely want to minimise
> duplication of code there.

There may not be much differences, but I'll have to look at your code to
see.

> So I'd suggest leaving the arch independent code in the drivers seat,
> but simultaneously replacing/redesigning pm_ops & swsusp so that all the
> ops that are or might be needed are there and called at the appropriate
> time.

Re: [PATCH] fix tulip suspend/resume

[Pavel Machek]

Hi!

> > following patch fix the suspend/resume for tulip based
> > cards, so suspend on disk work now for me and tulip based
> > cardbus cards.
> > 
> > 
> > Signed-off-by: Karsten Keil <kkeil@suse.de>
> > 
> >  static int tulip_suspend (struct pci_dev *pdev, pm_message_t state)
> >  {
> >  	struct net_device *dev = pci_get_drvdata(pdev);
> > +	int err;
> >  
> > +	pci_save_state(pdev);
> >  	if (dev && netif_running (dev) && netif_device_present (dev)) {
> >  		netif_device_detach (dev);
> >  		tulip_down (dev);
> >  		/* pci_power_off(pdev, -1); */
> >  	}
> > +	if ((err = pci_set_power_state(pdev, PCI_D3hot)))
> > +		printk(KERN_ERR "%s: pci_set_power_state D3hot return %d\n", dev->name, err);
> >  	return 0;
> >  }
> 
> It should probably test for message state, it's not worth doing
> pci_set_power_state(D3) if PMSG_FREEZE is passed... (just slows down
> suspend to disk)

How long does powering down netcard take? I am not sure speedup is worth added complexity.

				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms