[PATCH] Don't touch USB controllers with MMIO disabled in quirks

[PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Paul Mackerras]

Recently a quirk was added which attempts to do usb-handoff on all USB
host controllers on all platforms.  This is causing machine checks on
various machines such as my G4 powerbook, because the quirk code
attempts to do MMIO to the device without calling pci_enable_device,
or even checking that MMIO is enabled.

I still think that a FIXUP_HEADER header is the wrong place to be
doing this sort of thing, and that code that touches a device without
doing pci_enable_device is just asking for trouble; however, in order
to get my machine to be able to boot, this patch adds a check that
MMIO is enabled for the device, and if it isn't, leaves the device
alone.  With this patch my powerbook will boot.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---

diff -urN powerpc-merge/drivers/usb/host/pci-quirks.c merge-hack/drivers/usb/host/pci-quirks.c
--- powerpc-merge/drivers/usb/host/pci-quirks.c	2005-10-31 13:15:27.000000000 +1100
+++ merge-hack/drivers/usb/host/pci-quirks.c	2005-11-01 14:47:47.000000000 +1100
@@ -286,6 +286,11 @@
 
 static void __devinit quirk_usb_early_handoff(struct pci_dev *pdev)
 {
+	u16 cmd;
+
+	if (pci_read_config_word(pdev, PCI_COMMAND, &cmd) ||
+	    (cmd & PCI_COMMAND_MEMORY) == 0)
+		return;
 	if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
 		quirk_usb_handoff_uhci(pdev);
 	else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Linus Torvalds]

On Tue, 1 Nov 2005, Paul Mackerras wrote:
> 
> I still think that a FIXUP_HEADER header is the wrong place to be
> doing this sort of thing, and that code that touches a device without
> doing pci_enable_device is just asking for trouble; however, in order
> to get my machine to be able to boot, this patch adds a check that
> MMIO is enabled for the device, and if it isn't, leaves the device
> alone.  With this patch my powerbook will boot.

Well, this can't be right, because depending on which controller type it 
is, the handoff code uses PIO, not MMIO. In fact, a uhci controller 
wouldn't necessarily ever have PCI_COMMAND_MEMORY set afaik, since it 
doesn't even _have_ MMIO.

Would something like the appended work instead?

		Linus

---
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index b7fd3f6..b1aa350 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -138,11 +138,23 @@ reset_needed:
 }
 EXPORT_SYMBOL_GPL(uhci_check_and_reset_hc);
 
+static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
+{
+	u16 cmd;
+	return !pci_read_config_word(pdev, PCI_COMMAND, &cmd) && (cmd & mask);
+}
+
+#define pio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_IO)
+#define mmio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_MEMORY)
+
 static void __devinit quirk_usb_handoff_uhci(struct pci_dev *pdev)
 {
 	unsigned long base = 0;
 	int i;
 
+	if (!pio_enabled(pdev))
+		return;
+
 	for (i = 0; i < PCI_ROM_RESOURCE; i++)
 		if ((pci_resource_flags(pdev, i) & IORESOURCE_IO)) {
 			base = pci_resource_start(pdev, i);
@@ -153,12 +165,20 @@ static void __devinit quirk_usb_handoff_
 		uhci_check_and_reset_hc(pdev, base);
 }
 
+static int __devinit mmio_resource_enabled(struct pci_dev *pdev, int idx)
+{
+	return pci_resource_start(pdev, idx) && mmio_enabled(pdev);
+}
+
 static void __devinit quirk_usb_handoff_ohci(struct pci_dev *pdev)
 {
 	void __iomem *base;
 	int wait_time;
 	u32 control;
 
+	if (!mmio_resource_enabled(pdev, 0))
+		return;
+
 	base = ioremap_nocache(pci_resource_start(pdev, 0),
 				     pci_resource_len(pdev, 0));
 	if (base == NULL) return;
@@ -201,6 +221,9 @@ static void __devinit quirk_usb_disable_
 	u32 hcc_params, val, temp;
 	u8 cap_length;
 
+	if (!mmio_resource_enabled(pdev, 0))
+		return;
+
 	base = ioremap_nocache(pci_resource_start(pdev, 0),
 				pci_resource_len(pdev, 0));
 	if (base == NULL) return;

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Paul Mackerras]

Linus Torvalds writes:

> Would something like the appended work instead?

Looks fine to me, and it fixes the problem on my powerbook.

Paul.

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[David Brownell]

On Monday 31 October 2005 8:03 pm, Paul Mackerras wrote:

>  static void __devinit quirk_usb_early_handoff(struct pci_dev *pdev)
>  {
> +	u16 cmd;
> +
> +	if (pci_read_config_word(pdev, PCI_COMMAND, &cmd) ||
> +	    (cmd & PCI_COMMAND_MEMORY) == 0)

I suspect that should be

	(tabs)	|| (cmd & (PCI_COMMAND_MEMORY|PCI_COMMAND_IO)) == 0

Admittedly that'll matter only for UHCI, which isn't much used out of
x86 and ia64 ... but testing for both is more correct.  Other than that,
this looks good to me.

- Dave


> +		return;
>  	if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
>  		quirk_usb_handoff_uhci(pdev);
>  	else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)
> 

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Alan Stern]

On Mon, 31 Oct 2005, Linus Torvalds wrote:

> Well, this can't be right, because depending on which controller type it 
> is, the handoff code uses PIO, not MMIO. In fact, a uhci controller 
> wouldn't necessarily ever have PCI_COMMAND_MEMORY set afaik, since it 
> doesn't even _have_ MMIO.
> 
> Would something like the appended work instead?
> 
> 		Linus
> 
> ---
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index b7fd3f6..b1aa350 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -138,11 +138,23 @@ reset_needed:
>  }
>  EXPORT_SYMBOL_GPL(uhci_check_and_reset_hc);
>  
> +static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
> +{
> +	u16 cmd;
> +	return !pci_read_config_word(pdev, PCI_COMMAND, &cmd) && (cmd & mask);
> +}
> +
> +#define pio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_IO)
> +#define mmio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_MEMORY)
> +
>  static void __devinit quirk_usb_handoff_uhci(struct pci_dev *pdev)
>  {
>  	unsigned long base = 0;
>  	int i;
>  
> +	if (!pio_enabled(pdev))
> +		return;
> +

In theory, is it possible for a UHCI controller still to be running, doing 
DMA and/or generating interrupts, even if PCI_COMMAND_IO isn't set?  If it 
is, is there anything wrong with enabling the device fully in order to 
shut it off?

Or is this scenario not worth worrying about?

Alan Stern

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Linus Torvalds]

On Tue, 1 Nov 2005, Alan Stern wrote:
> 
> In theory, is it possible for a UHCI controller still to be running, doing 
> DMA and/or generating interrupts, even if PCI_COMMAND_IO isn't set?

Yes, it's possible in theory. I guess we could check whether BUS_MASTER is 
enabled (which _does_ need to be enabled, otherwise it couldn't function), 
and then enable it.

> Or is this scenario not worth worrying about?

It's probably not worth worrying about. After all, this is really just for 
when something else (firmware) has enabled the USB controller for its own 
nefarious purposes (ie it also wanted keyboard input), and left it 
running. If something has left it running by mistake, it won't have 
disabled IO access either.

And if it _has_ disabled IO access, we wouldn't know how to enable it at 
this point. Sure, we could enable the command bit, but this is too early 
for us to know where in the IO address space it would be safe to enable 
it.

But an alternative strategy (which might be very sensible) is to forget 
about the handoff entirely, and just shut down the bus master flag 
unconditionally. Just make sure that the eventual driver will reset the 
controller before it re-enables bus mastering.

That would seem to be the simplest possible "handoff". The only danger is 
that I could imagine that there would be controllers out there that get 
really confused (ie "I'm not going to play nice any more") if we shut them 
up that way.

		Linus

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Alan Stern]

On Tue, 1 Nov 2005, Linus Torvalds wrote:

> On Tue, 1 Nov 2005, Alan Stern wrote:
> > 
> > In theory, is it possible for a UHCI controller still to be running, doing 
> > DMA and/or generating interrupts, even if PCI_COMMAND_IO isn't set?
> 
> Yes, it's possible in theory. I guess we could check whether BUS_MASTER is 
> enabled (which _does_ need to be enabled, otherwise it couldn't function), 
> and then enable it.
> 
> > Or is this scenario not worth worrying about?
> 
> It's probably not worth worrying about. After all, this is really just for 
> when something else (firmware) has enabled the USB controller for its own 
> nefarious purposes (ie it also wanted keyboard input), and left it 
> running. If something has left it running by mistake, it won't have 
> disabled IO access either.
> 
> And if it _has_ disabled IO access, we wouldn't know how to enable it at 
> this point. Sure, we could enable the command bit, but this is too early 
> for us to know where in the IO address space it would be safe to enable 
> it.
> 
> But an alternative strategy (which might be very sensible) is to forget 
> about the handoff entirely, and just shut down the bus master flag 
> unconditionally. Just make sure that the eventual driver will reset the 
> controller before it re-enables bus mastering.
> 
> That would seem to be the simplest possible "handoff". The only danger is 
> that I could imagine that there would be controllers out there that get 
> really confused (ie "I'm not going to play nice any more") if we shut them 
> up that way.

Then here's a patch to do that.

Unfortunately, the current setup _does_ enable bus mastering before 
resetting the controller.  That deserves to be fixed in a separate patch.

Alan Stern


Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/usb/host/pci-quirks.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/pci-quirks.c
+++ usb-2.6/drivers/usb/host/pci-quirks.c
@@ -138,11 +138,47 @@ reset_needed:
 }
 EXPORT_SYMBOL_GPL(uhci_check_and_reset_hc);
 
+static int __devinit io_type_enabled(struct pci_dev *pdev, unsigned int mask)
+{
+	u16 cmd = 0;
+
+	if (pci_read_config_word(pdev, PCI_COMMAND, &cmd) || !(cmd & mask)) {
+
+		/* The controller is at least partially disabled.
+		 * To be on the safe side, we'll turn off the COMMAND_MASTER
+		 * bit so that it can't do DMA.
+		 */
+		if (cmd & PCI_COMMAND_MASTER)
+			pci_write_config_word(pdev, PCI_COMMAND,
+					cmd & ~PCI_COMMAND_MASTER);
+		cmd = 0;
+	}
+	return cmd;
+}
+
+#define pio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_IO)
+#define mmio_enabled(dev) io_type_enabled(dev, PCI_COMMAND_MEMORY)
+
+static int __devinit mmio_resource_enabled(struct pci_dev *pdev, int idx)
+{
+	return mmio_enabled(pdev) && pci_resource_start(pdev, idx);
+}
+
 static void __devinit quirk_usb_handoff_uhci(struct pci_dev *pdev)
 {
 	unsigned long base = 0;
 	int i;
 
+	if (!pio_enabled(pdev)) {
+
+		/* The controller is at least partially disabled.
+		 * To be safe, we'll disable PIRQD and SMI so that
+		 * it can't generate interrupts.
+		 */
+		pci_write_config_word(pdev, UHCI_USBLEGSUP, 0);
+		return;
+	}
+
 	for (i = 0; i < PCI_ROM_RESOURCE; i++)
 		if ((pci_resource_flags(pdev, i) & IORESOURCE_IO)) {
 			base = pci_resource_start(pdev, i);
@@ -159,6 +195,9 @@ static void __devinit quirk_usb_handoff_
 	int wait_time;
 	u32 control;
 
+	if (!mmio_resource_enabled(pdev, 0))
+		return;
+
 	base = ioremap_nocache(pci_resource_start(pdev, 0),
 				     pci_resource_len(pdev, 0));
 	if (base == NULL) return;
@@ -201,6 +240,9 @@ static void __devinit quirk_usb_disable_
 	u32 hcc_params, val, temp;
 	u8 cap_length;
 
+	if (!mmio_resource_enabled(pdev, 0))
+		return;
+
 	base = ioremap_nocache(pci_resource_start(pdev, 0),
 				pci_resource_len(pdev, 0));
 	if (base == NULL) return;

Re: [PATCH] Don't touch USB controllers with MMIO disabled in quirks

[Benjamin Herrenschmidt]

> But an alternative strategy (which might be very sensible) is to forget 
> about the handoff entirely, and just shut down the bus master flag 
> unconditionally. Just make sure that the eventual driver will reset the 
> controller before it re-enables bus mastering.

Unfortunately, I know quite a few devices (including USB controllers)
that will react badly to the bus master flag being just cleared like
that. By badly, I mean it ranges from simply ignoring it and happily
continuing whatever bus master was being done, to completely screwing up
and crapping all over your memory/bus.

> That would seem to be the simplest possible "handoff". The only danger is 
> that I could imagine that there would be controllers out there that get 
> really confused (ie "I'm not going to play nice any more") if we shut them 
> up that way.

I suspect with the IO/MEM enable test fix we did, it shoul work fine in
practice for all cases. Let's address the "potential" issues if they
happen to show up in real life, which I doubt.

Ben.