On Thu, May 20, 2010 at 06:20:28PM -0500, Nguyen, Dong wrote:
> Hi Sarah,
>
> The else block is for both MSI and Legacy interrupt. I probably need to set the value of pdev->irq after free_irq:
>
> else {
> if (pdev->irq >= 0) {
> free_irq(pdev->irq, xhci_to_hcd(xhci));
> pdev->irq = -1;
> }
> }
Yes, that might fix the symptom, but I don't think you *should* free the
legacy interrupt. The USB core set it up, and it's the core's job to
tear it down. You should only free that interrupt if you've setup MSI.
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;
}
Another reason not to free the legacy interrupt in xhci_free_irq() is
that xhci_setup_msi() calls xhci_free_irq() if one of the request_irq()
calls fails. I don't think you want to disable the legacy interrupt in
that case.
One more monkey wrench: Say MSI-X allocation fails. You fall back to
MSI and call pci_enable_msi(), which changes the irq number of the pci
device to something different. Now say you call request_irq(), and it
fails. You then call pci_disable_msi().
The issue is the original PCI pin-based irq number could have been given
to a different device (see Documentation/PCI/MSI-HOWTO.txt, section
4.2.3). Then the USB core thinks it's still using hcd->irq, but that's
been assigned to a different device. Then you'll never get interrupts
for the host. Worse, when the host controller driver is unloaded, the
USB core will call free_irq() on an IRQ that doesn't belong to it.
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.
Good news is that straight MSI-X works fine for me.
Sarah Sharp
> -----Original Message-----
> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Sarah Sharp
> Sent: Thursday, May 20, 2010 4:01 PM
> To: Nguyen, Dong
> Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Yang, Libin; Xu, Andiry
> Subject: Re: [PATCH v4 1/1] xHCI: Supporting MSI/MSI-X - Resend
>
> Hi Dong,
>
> I tested this with CONFIG_PCI_MSI=n, and I no longer see the crashes
> when I plug in devices. However, I do see a WARNING when I unload the
> driver. The issue is you're freeing the IRQ when xhci_stop() is called,
> but the USB core will take care of that in usb_remove_hcd(). The
> warning is triggered when the double free of the IRQ is detected in
> usb_remove_hcd().
>
> On Thu, May 20, 2010 at 11:27:00AM -0700, Dong Nguyen wrote:
> > -#if 0
> > -/* Set up MSI-X table for entry 0 (may claim other entries later) */
> > +/*
> > + * Free IRQs
> > + * free all IRQs request
> > + */
> > +static void xhci_free_irq(struct xhci_hcd *xhci)
> > +{
> > + int i;
> > + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> > +
> > + if (xhci->msix_entries) {
> > + for (i = 0; i < xhci->msix_count; i++)
> > + if (xhci->msix_entries[i].vector)
> > + free_irq(xhci->msix_entries[i].vector,
> > + xhci_to_hcd(xhci));
> > + } else {
> > + if (pdev->irq)
> > + free_irq(pdev->irq, xhci_to_hcd(xhci));
> > + }
>
> Remove this else block here, and I think the warning will go away.
>
> Here's the output from the warning if you're curious:
>
> [105361.669858] xhci_hcd 0000:05:00.0: xhci_stop completed - status = 11
> [105361.669879] ------------[ cut here ]------------
> [105361.669903] WARNING: at kernel/irq/manage.c:890 __free_irq+0x9f/0x1a0()
> [105361.669924] Hardware name: 74663HU
> [105361.669945] Trying to free already-free IRQ 19
> [105361.669964] Modules linked in: xhci_hcd(-) netconsole configfs joydev usbhid nls_iso8859_1 nls_cp437 vfat fat usb_storage tun aes_x86_64 aes_generic binfmt_misc fbcon tileblit font bitblit softcursor ppdev lp parport snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_pcm_oss snd_mixer_oss thinkpad_acpi snd_pcm snd_seq_dummy snd_seq_oss arc4 snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq iwlagn iwlcore snd_timer snd_seq_device i915 psmouse mac80211 snd drm_kms_helper pcspkr serio_raw cfg80211 iTCO_wdt iTCO_vendor_support led_class drm tpm_tis i2c_algo_bit soundcore intel_agp nvram tpm tpm_bios video snd_page_alloc output ehci_hcd uhci_hcd usbcore [last unloaded: xhci_hcd]
> [105361.671343] Pid: 22959, comm: modprobe Not tainted 2.6.34-rc7 #476
> [105361.671365] Call Trace:
> [105361.671390] [<ffffffff8104dee8>] warn_slowpath_common+0x78/0xb0
> [105361.671415] [<ffffffff8104df7c>] warn_slowpath_fmt+0x3c/0x40
> [105361.671444] [<ffffffff8153bc5c>] ? _raw_spin_lock_irqsave+0x4c/0x60
> [105361.671468] [<ffffffff810b308e>] ? __free_irq+0x4e/0x1a0
> [105361.671491] [<ffffffff810b30df>] __free_irq+0x9f/0x1a0
> [105361.671517] [<ffffffff8105e080>] ? del_timer_sync+0x0/0x90
> [105361.671540] [<ffffffff810b362f>] free_irq+0x3f/0x80
> [105361.671574] [<ffffffffa00085de>] usb_remove_hcd+0x10e/0x130 [usbcore]
> [105361.671610] [<ffffffffa0018cf6>] usb_hcd_pci_remove+0x26/0xa0 [usbcore]
> [105361.671639] [<ffffffff81291bd2>] pci_device_remove+0x32/0x60
> [105361.671665] [<ffffffff81324ab0>] __device_release_driver+0x70/0xe0
> [105361.671690] [<ffffffff81324fc0>] driver_detach+0xc0/0xd0
> [105361.671715] [<ffffffff8132398f>] bus_remove_driver+0x8f/0xf0
> [105361.671739] [<ffffffff813251ea>] driver_unregister+0x5a/0x90
> [105361.671763] [<ffffffff81291d4f>] pci_unregister_driver+0x3f/0xc0
> [105361.671790] [<ffffffffa0343600>] xhci_unregister_pci+0x10/0x20 [xhci_hcd]
> [105361.671815] [<ffffffffa0349bf9>] xhci_hcd_cleanup+0x9/0xb [xhci_hcd]
> [105361.671841] [<ffffffff8108b408>] sys_delete_module+0x188/0x260
> [105361.671868] [<ffffffff81072dae>] ? up_read+0x1e/0x40
> [105361.671893] [<ffffffff8153ba1a>] ? lockdep_sys_exit_thunk+0x35/0x67
> [105361.671919] [<ffffffff81009f02>] system_call_fastpath+0x16/0x1b
> [105361.671943] ---[ end trace 0f01d8b0b9f7139f ]---
> [105361.672911] xhci_hcd 0000:05:00.0: USB bus 9 deregistered
> [105361.673311] xhci_hcd 0000:05:00.0: PCI INT A disabled
>
> I'm recompiling with MSI turned back on, and I'll let you know if I run
> into any issues.
>
> Sarah Sharp
> --
> 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
>
>
--
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
