PCI changes kill USB PM

PCI changes kill USB PM

[Benjamin Herrenschmidt]

Hi !

The recent PCI changes are killing USB Power Management, and maybe more.
The problem is that the common PCI code will now always call
pci_save_state() after calling the pci_driver->suspend().

The USB suspend() code does pci_save_state() then pci_disable_device(),
which clears PCI_COMMAND_MASTER in the PCI command register.

So when later on, the PCI core calls pci_save_state() again, it saves a
state that corresponds to a suspended device with the command register
"disabled".

On wakeup, USB does pci_set_mater() then pci_restore_state(). Now, what
happen is that the later "undos" the work done by pci_set_master(),
restoring a command register with PCI_COMMAND_MASTER clear. Bad bad bad.

This triggers a very funny behaviour on pmac laptops btw, for some
reason, the Apple IO ASIC that contains the 2 OHCI's along with a bunch
of other devices like IDE gets crazy of this situation (the controller
is enabled, tries to bus master, but isn't allowed to do so by it's
command reg) and the whole ASIC seem to lose all sense of PCI
arbitration, IDE throughput goes down to about 40Kb/sec for example...

There are 2 issues here. One is that USB should definitely call
pci_set_master() _after_ pci_resume_state(), though it shouldn't need to
call it at all ...

The other one is that I don't like the asymetry in the PCI core of one
side always calling pci_save_state() after the driver suspend() while
the other side only calls pci_restore_state() when there is no driver
resume()... I really prefer having save_state & restore_state 100% under
driver control when the driver has suspend & resume routines.

This patch takes cares of both, Andrew, do not apply before Ack from
Greg since the change in the PCI code may affect some other things (I
hope you didn't start removing calls to pci_save_state() from
drivers ? :)

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/drivers/usb/core/hcd-pci.c
===================================================================
--- linux-work.orig/drivers/usb/core/hcd-pci.c	2004-10-20 13:01:02.000000000 +1000
+++ linux-work/drivers/usb/core/hcd-pci.c	2004-10-29 17:57:52.027929064 +1000
@@ -366,7 +366,6 @@
 			"can't restore IRQ after resume!\n");
 		return retval;
 	}
-	pci_set_master (dev);
 	pci_restore_state (dev);
 #ifdef	CONFIG_USB_SUSPEND
 	pci_enable_wake (dev, dev->current_state, 0);
Index: linux-work/drivers/pci/pci-driver.c
===================================================================
--- linux-work.orig/drivers/pci/pci-driver.c	2004-10-20 13:01:02.000000000 +1000
+++ linux-work/drivers/pci/pci-driver.c	2004-10-29 17:57:44.202118768 +1000
@@ -308,8 +308,8 @@
 	dev_state = state_conversion[state];
 	if (drv && drv->suspend)
 		i = drv->suspend(pci_dev, dev_state);
-		
-	pci_save_state(pci_dev);
+	else
+		pci_save_state(pci_dev);
 	return i;
 }
 

Re: PCI changes kill USB PM

[David Brownell]

Thanks, you just saved me more headscratching ... :)


On Friday 29 October 2004 17:14, Benjamin Herrenschmidt wrote:
> 
> The recent PCI changes are killing USB Power Management, and maybe more.
> The problem is that the common PCI code will now always call
> pci_save_state() after calling the pci_driver->suspend().

Where "now" is "after a change from Arjan in May";
recent changes made it a more significant problem.

I agree with the guts of your patch -- adding the
missing "else" in:

	if (pci_driver && pci_driver->suspend)
		pci_driver->suspend(pdev, state);
	else
		pci_save_state(pdev);

That makes Arjan's patch only affect PCI drivers
which don't know how to suspend/resume themselves;
that's all his patch was supposed to affect.



> On wakeup, USB does pci_set_mater() then pci_restore_state().
> happen is that the later "undos" the work done by pci_set_master(),
> restoring a command register with PCI_COMMAND_MASTER clear. Bad bad bad.

That explains a problem I've been seeing with root hub lossage
after resume.  I was about to start digging deeper into this,
now that I've addressed EHCI and OHCI problems that show up
earlier for me (and some other PCI suspend/resume issues that
complicated them on different systems!), but this solves a
"DMA not working" symptom that I noticed after merging those
updates with RC1.

The relevant PCI PM code has changed semantics at least four
times since that bit of usbcore was written, but I'd only
noticed two of the changes ... :(


> There are 2 issues here. One is that USB should definitely call
> pci_set_master() _after_ pci_resume_state(), though it shouldn't need to
> call it at all ...

It shouldn't actually have mattered ... which
means it's safe to delete.  (I forget where that
sequence was copied from; too bad.)


> This patch takes cares of both, Andrew, do not apply before Ack from
> Greg since the change in the PCI code may affect some other things (I
> hope you didn't start removing calls to pci_save_state() from
> drivers ? :)

The PCI part looks essential to me, and just restores
previous (and long-documented!!) behavior ... if it
affects anything, it should be to make it work again.

The USB part is OK, but shouldn't be merged before
some more essential patches(*) to that file.

What I've done with your patch is updated it to apply
against a conflicting USB patch, and forwarded the
result to Greg (CC you and linux-usb-devel).  Or,
this could be handled as two separate one-liners.

- Dave

(*) http://marc.theaimsgroup.com/?l=linux-usb-devel&m=109881500807667&w=2


> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> --- linux-work.orig/drivers/pci/pci-driver.c	2004-10-20 13:01:02.000000000 
+1000
> +++ linux-work/drivers/pci/pci-driver.c	2004-10-29 17:57:44.202118768 +1000
> @@ -308,8 +308,8 @@
>  	dev_state = state_conversion[state];
>  	if (drv && drv->suspend)
>  		i = drv->suspend(pci_dev, dev_state);
> -		
> -	pci_save_state(pci_dev);
> +	else
> +		pci_save_state(pci_dev);
>  	return i;
>  }
>  
> 

Re: PCI changes kill USB PM

[Arjan van de Ven]

On Sat, 2004-10-30 at 10:44 -0700, David Brownell wrote:

> Where "now" is "after a change from Arjan in May";
> recent changes made it a more significant problem.
> 
> I agree with the guts of your patch -- adding the
> missing "else" in:
> 
> 	if (pci_driver && pci_driver->suspend)
> 		pci_driver->suspend(pdev, state);
> 	else
> 		pci_save_state(pdev);
> 
> That makes Arjan's patch only affect PCI drivers
> which don't know how to suspend/resume themselves;
> that's all his patch was supposed to affect.

well the idea back then was to just always save so that drivers could be lazy and only provide a specialized restore. However I agree with this change now that the API changed where regular state saves use the same storage area.