usb, PCI: split quirk for usb host controller to three

RFC PATCH In review Yinghai Lu 4 participants 8 messages Mar 1, 2012

Subsystems (3)

USB PCI USB xHCI driver

lore

Share & tools

lore

Yinghai Lu

author

2012-03-01 17:17 UTC #0 lore raw

RFC PATCH 1/1 usb, PCI: split quirk for usb host controller to three

so we avoid checking class again and again in that quirk.

need to be applied after pci/linux-next and usb/usb-next

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/usb/host/pci-quirks.c |   42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

drivers/usb/host/pci-quirks.c +28 ~~-14~~

Index: linux-2.6/drivers/usb/host/pci-quirks.c
===================================================================
@@ -884,33 +884,47 @@ static void __devinit quirk_usb_handoff_
 	iounmap(base);
 }
 
-static void __devinit quirk_usb_early_handoff(struct pci_dev *pdev)
+static void __devinit quirk_usb_early_handoff_uhci(struct pci_dev *pdev)
+{
+	quirk_usb_handoff_uhci(pdev);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_SERIAL_USB_UHCI, 0, quirk_usb_early_handoff_uhci);
+
+static void __devinit quirk_usb_early_handoff_ohci(struct pci_dev *pdev)
 {
 	/* Skip Netlogic mips SoC's internal PCI USB controller.
 	 * This device does not need/support EHCI/OHCI handoff
 	 */
 	if (pdev->vendor == 0x184e)	/* vendor Netlogic */
 		return;
-	if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
-			pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
-			pdev->class != PCI_CLASS_SERIAL_USB_EHCI &&
-			pdev->class != PCI_CLASS_SERIAL_USB_XHCI)
+
+	quirk_usb_handoff_ohci(pdev);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_SERIAL_USB_OHCI, 0, quirk_usb_early_handoff_ohci);
+
+static void __devinit quirk_usb_early_handoff_ehci(struct pci_dev *pdev)
+{
+	if (pdev->vendor == 0x184e)	/* vendor Netlogic */
 		return;
 
+	quirk_usb_handoff_ehci(pdev);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_SERIAL_USB_EHCI, 0, quirk_usb_early_handoff_ehci);
+
+static void __devinit quirk_usb_early_handoff_xhci(struct pci_dev *pdev)
+{
 	if (pci_enable_device(pdev) < 0) {
 		dev_warn(&pdev->dev, "Can't enable PCI device, "
 				"BIOS handoff failed.\n");
 		return;
 	}
-	if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
-		quirk_usb_handoff_uhci(pdev);
-	else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)
-		quirk_usb_handoff_ohci(pdev);
-	else if (pdev->class == PCI_CLASS_SERIAL_USB_EHCI)
-		quirk_usb_handoff_ehci(pdev);
-	else if (pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
-		quirk_usb_handoff_xhci(pdev);
+
+	quirk_usb_handoff_xhci(pdev);
+
 	pci_disable_device(pdev);
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-			PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
+		PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_usb_early_handoff_xhci);

Greg Kroah-Hartman

maintainer

2012-03-01 17:24 UTC #1 lore raw

Quoted 3 lines · Yinghai Lu

so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next

As those are two independant trees, this needs to wait until after 3.4-rc1 is out.

Quoted 5 lines

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/usb/host/pci-quirks.c |   42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Is this really helping anything here? You added code overall :(

greg k-h

Yinghai Lu

author

2012-03-01 17:45 UTC #2 lore raw

Quoted 7 lines · Greg Kroah-Hartman

Quoted 3 lines · Yinghai Lu
so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next
As those are two independant trees, this needs to wait until after 3.4-rc1 is out.

ok.

Quoted 8 lines

Quoted 6 lines
Signed-off-by Yinghai Lu <[email protected]>
---
drivers/usb/host/pci-quirks.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)
Is this really helping anything here? You added code overall :(

just make the code looks simple by removing

        if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
                        pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
                        pdev->class != PCI_CLASS_SERIAL_USB_EHCI &&
                        pdev->class != PCI_CLASS_SERIAL_USB_XHCI)
                return;
...
        if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
   ...
        else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)
   ...
        else if (pdev->class == PCI_CLASS_SERIAL_USB_EHCI)
    ...
        else if (pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
    ...

also skip disable/enable device on non xhci controller.

Thanks

Yinghai

Bjorn Helgaas

maintainer

2012-03-01 18:20 UTC #3 lore raw

Quoted 39 lines · Yinghai Lu

Quoted 7 lines · Greg Kroah-Hartman
Quoted 3 lines · Yinghai Lu
so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next
As those are two independant trees, this needs to wait until after 3.4-rc1 is out.
ok.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Yinghai Lu <[email protected]>
---
drivers/usb/host/pci-quirks.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)
Is this really helping anything here? You added code overall :(
just make the code looks simple by removing
if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI && pdev->class != PCI_CLASS_SERIAL_USB_OHCI && pdev->class != PCI_CLASS_SERIAL_USB_EHCI && pdev->class != PCI_CLASS_SERIAL_USB_XHCI) return;
...
if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
...
else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)
...
else if (pdev->class == PCI_CLASS_SERIAL_USB_EHCI)
...
else if (pdev->class == PCI_CLASS_SERIAL_USB_XHCI)
...

Quoted 1 line

also skip disable/enable device on non xhci controller.

Hmm... From the description, I thought this patch only took advantage of DECLARE_PCI_FIXUP_CLASS_FINAL to remove the "if (pdev->class == XXX)" tests.

If it *also* skips a disable/enable sequence on non-xhci controllers, please make that a separate patch so it's clear what's happening.

It's very important to be clear about these things for people who are deciding whether to backport patches into distros. All the zillions of PCI changes we're doing make their lives hard enough without having subtle behavior changes hidden in patches that seem to be "code restructure only, no behavior change."

Bjorn

Yinghai Lu

author

2012-03-01 19:20 UTC #4 lore raw

Quoted 4 lines · Bjorn Helgaas

Hmm... From the description, I thought this patch only took advantage of DECLARE_PCI_FIXUP_CLASS_FINAL to remove the "if (pdev->class == XXX)" tests.

Quoted 8 lines

If it *also* skips a disable/enable sequence on non-xhci controllers, please make that a separate patch so it's clear what's happening.
It's very important to be clear about these things for people who are deciding whether to backport patches into distros. All the zillions of PCI changes we're doing make their lives hard enough without having subtle behavior changes hidden in patches that seem to be "code restructure only, no behavior change."

sure, will separate the patch to small ones.

Thanks

Yinghai

Sarah Sharp

2012-03-01 19:03 UTC #5 lore raw

In reply to Greg Kroah-Hartman

Quoted 8 lines · Greg Kroah-Hartman

Quoted 3 lines · Yinghai Lu
so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next
As those are two independant trees, this needs to wait until after 3.4-rc1 is out.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/usb/host/pci-quirks.c |   42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)
Is this really helping anything here? You added code overall :(

I agree with Greg. Why change this? Does it shave off any boot time? Please show hard numbers for what improvements this makes. I really don't want to change PCI init and break people's systems.

Sarah Sharp

Yinghai Lu

author

2012-03-01 19:19 UTC #6 lore raw

Quoted 15 lines · Sarah Sharp

Quoted 14 lines · Greg Kroah-Hartman
Quoted 3 lines · Yinghai Lu
so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next
As those are two independant trees, this needs to wait until after 3.4-rc1 is out.
Quoted 5 lines
Signed-off-by Yinghai Lu <[email protected]>
---
drivers/usb/host/pci-quirks.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Quoted 5 lines

Quoted 1 line
Is this really helping anything here? You added code overall :(
I agree with Greg. Why change this? Does it shave off any boot time? Please show hard numbers for what improvements this makes. I really don't want to change PCI init and break people's systems.

but your code make pci init change to call disable/enable for non-xhci for fixing xhci problem.

| commit cab928ee1f221c9cc48d6615070fefe2e444384a
| Author: Sarah Sharp <[email protected]>
| Date:   Tue Feb 7 15:11:46 2012 -0800
|
|    USB: Fix handoff when BIOS disables host PCI device.

Yinghai

Sarah Sharp

2012-03-01 20:07 UTC #7 lore raw

Quoted 17 lines · Yinghai Lu

Quoted 15 lines · Sarah Sharp
Quoted 14 lines · Greg Kroah-Hartman
Quoted 3 lines · Yinghai Lu
so we avoid checking class again and again in that quirk.
need to be applied after pci/linux-next and usb/usb-next
As those are two independant trees, this needs to wait until after 3.4-rc1 is out.
Quoted 5 lines
Signed-off-by Yinghai Lu <[email protected]>
---
drivers/usb/host/pci-quirks.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Is this really helping anything here? You added code overall :(
I agree with Greg. Why change this? Does it shave off any boot time? Please show hard numbers for what improvements this makes. I really don't want to change PCI init and break people's systems.
but your code make pci init change to call disable/enable for non-xhci for fixing xhci problem.

We discussed this, and agreed it was safe to do so for all USB host controllers. It's possible some other BIOS will disable the PCI host controller before handing it off to the OS, so we have to handle that case. So it's fine that we enable/disable the device for all USB host controllers. I tried to make sure we weren't enabling/disabling for non-USB PCI devices.

Sarah Sharp