Hi Sarah,
Thanks for your suggestion. My comments below:
> What if you set hcd->driver->irq to NULL once you've set up MSI or
> MSI-X, here:
>
> /*
> * sucessfully setup msi/msi-x,
> * need to disable the legacy INTx
> */
> if (!ret) {
> if (hcd->irq)
> free_irq(hcd->irq, hcd);
> hcd->irq = -1;
> hcd->driver->irq = NULL;
> }
>
> Then you can check for that NULL pointer in your clean up function, and
> only free the IRQ if you have setup MSI:
>
> else if (xhci_to_hcd(xhci)->driver->irq == NULL) {
> free_irq(pdev->irq, xhci_to_hcd(xhci));
> pdev->irq = -1;
> }
>
The hcd->driver is read only memory and I don't think we can set to NULL
once xhci initialization completed. That's why I introduced the
msi/msi-x enable flag in first patch so I only call free_irq for
msi/msi-x case and leave the legacy interrupt for core hcd to free irq.
Adding one msi/msi-x flag probably easiest solution when clean up
msi/msi-x.
> I think you have two choices if pdev->irq isn't the same as hcd->irq
> after a call to pci_disable_msi(). You can just straight up fail, and
> make xhci_run() return an error (although users aren't going to be happy
> with that). The second option is to call free_irq before trying to
> enable MSI or MSI-X, and try to reallocate the interrupt if those fail.
>
> Either way, you should fix this error case. Have you tried adding code
> to make the MSI-X allocation fail and fall back to MSI? Or make the
> request_irq() fail after calling either pci_msi_enable() or
> pci_msix_enable()? The code must work in those cases. This review
> would go much faster if you throughly tested your code.
To make the msi/msi-x patch works with legacy interrupt, I change the
code to free the legacy int before enable msi/msi-x and try to
reallocate the legacy interrupt if those fail.
Thanks,
Dong
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
