Re: [RFC PATCH] usb, PCI: split quirk for usb host controller to three

[Bjorn Helgaas <bhelgaas@google.com>]

On Thu, Mar 1, 2012 at 10:45 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Mar 1, 2012 at 9:24 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Thu, Mar 01, 2012 at 09:17:07AM -0800, Yinghai Lu wrote:
>>> 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.
>
>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>
>>> ---
>>>  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.

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