Re: Fw: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)

Re: Fw: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)

[Jeff Garzik]

Andrew Morton wrote:
> 
> Begin forwarded message:
> 
> Date: Thu, 28 Oct 2004 17:34:19 +0800 (CST)
> From: "Zhu, Yi" <yi.zhu@intel.com>
> To: Andrew Morton <akpm@osdl.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)
> 
> 
> 
> Hi,
> 
> Recent 2.6.10-rc1 merged the pci_save_state change. This prevents some
> drivers from working with suspend/resume. The reason is the
> pci_save_state() called in driver's ->suspend doesn't take effect any more,
> since pci bus ->suspend will override it. And the two states might be
> different in some drivers, i.e. e100. I don't know if there are other
> drivers also suffer from it.
> 
> Thanks,
> -yi
> 
> 
> Signed-off-by: Zhu Yi <yi.zhu@intel.com>
> 
> --- /tmp/e100.c	2004-10-28 16:31:41.000000000 +0800
> +++ drivers/net/e100.c	2004-10-28 16:33:14.000000000 +0800
> @@ -2309,9 +2309,7 @@ static int e100_suspend(struct pci_dev *
>  
> -	pci_save_state(pdev);
>  	pci_enable_wake(pdev, state, nic->flags & (wol_magic | e100_asf(nic)));
> -	pci_disable_device(pdev);
>  	pci_set_power_state(pdev, state);


I'm waiting for Greg to respond to the serious concerns raised by 
BenH[1] and me[2].

AFAICS, Greg has broken the standard Linux "driver knows it, driver does 
it" model.

	Jeff


[1] http://marc.theaimsgroup.com/?l=linux-kernel&m=109867742404637&w=2
[2] http://marc.theaimsgroup.com/?l=linux-kernel&m=109868495426108&w=2

Re: Fw: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)

[Greg KH]

On Thu, Oct 28, 2004 at 10:21:26AM -0400, Jeff Garzik wrote:
> Andrew Morton wrote:
> >
> >Begin forwarded message:
> >
> >Date: Thu, 28 Oct 2004 17:34:19 +0800 (CST)
> >From: "Zhu, Yi" <yi.zhu@intel.com>
> >To: Andrew Morton <akpm@osdl.org>, Linux Kernel Mailing List 
> ><linux-kernel@vger.kernel.org>
> >Subject: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to 
> >pci_save_state change)
> >
> >
> >
> >Hi,
> >
> >Recent 2.6.10-rc1 merged the pci_save_state change. This prevents some
> >drivers from working with suspend/resume. The reason is the
> >pci_save_state() called in driver's ->suspend doesn't take effect any more,
> >since pci bus ->suspend will override it. And the two states might be
> >different in some drivers, i.e. e100. I don't know if there are other
> >drivers also suffer from it.
> >
> >Thanks,
> >-yi
> >
> >
> >Signed-off-by: Zhu Yi <yi.zhu@intel.com>
> >
> >--- /tmp/e100.c	2004-10-28 16:31:41.000000000 +0800
> >+++ drivers/net/e100.c	2004-10-28 16:33:14.000000000 +0800
> >@@ -2309,9 +2309,7 @@ static int e100_suspend(struct pci_dev *
> > 
> >-	pci_save_state(pdev);
> > 	pci_enable_wake(pdev, state, nic->flags & (wol_magic | 
> > 	e100_asf(nic)));
> >-	pci_disable_device(pdev);
> > 	pci_set_power_state(pdev, state);
> 
> 
> I'm waiting for Greg to respond to the serious concerns raised by 
> BenH[1] and me[2].
> 
> AFAICS, Greg has broken the standard Linux "driver knows it, driver does 
> it" model.

Huh?  How did changing the pci_save_state() api change anything?  I
didn't change the pci core any, just made it easier to not have to
specify the storage location of the memory to save everything off on.

> [1] http://marc.theaimsgroup.com/?l=linux-kernel&m=109867742404637&w=2
> [2] http://marc.theaimsgroup.com/?l=linux-kernel&m=109868495426108&w=2

I'm still on the road (all week long) and will try to get to the above
messages soon...

thanks,

greg k-h

Re: Fw: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)

[Jeff Garzik]

Greg KH wrote:
> Huh?  How did changing the pci_save_state() api change anything?  I
> didn't change the pci core any, just made it easier to not have to
> specify the storage location of the memory to save everything off on.

That could have been accomplished by moving the saved-config data 
storage into struct pci_dev, but not changing pci_save_state() prototype...

	Jeff

Re: Fw: [PATCH] Fix e100 suspend/resume w/ 2.6.10-rc1 and above (due to pci_save_state change)

[Greg KH]

On Thu, Oct 28, 2004 at 10:59:25AM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> >Huh?  How did changing the pci_save_state() api change anything?  I
> >didn't change the pci core any, just made it easier to not have to
> >specify the storage location of the memory to save everything off on.
> 
> That could have been accomplished by moving the saved-config data 
> storage into struct pci_dev

We did that a while ago.

> but not changing pci_save_state() prototype...

That was the last piece.  It also fixed up some odd bugs when it was
done.

Do we really want drivers to go back to have to declare the pci config
space themselves if they want to?  What does that buy us?

I agree that Ben's comments are valid, but the recent patch doesn't
change anything that wasn't already being done.

thanks,

greg k-h