On Thu, Mar 19, 2015 at 02:49:21PM +0000, Brian Russell wrote:
> On 19/03/15 14:23, Greg Kroah-Hartman wrote:
> > On Thu, Mar 19, 2015 at 02:11:19PM +0000, Brian Russell wrote:
> >> if (info->irq && (info->irq != UIO_IRQ_CUSTOM)) {
> >> - ret = devm_request_irq(idev->dev, info->irq, uio_interrupt,
> >> + ret = request_irq(info->irq, uio_interrupt,
> >> info->irq_flags, info->name, idev);
> >
> > Why move this away from the device lifecycle?
> >
>
> I ran into a problem here: after device unregister the parent module (in my
> case igb_uio in DPDK) can call pci_disable_msi. From the PCI MSI how to:
>
> "Before calling this function, a device driver must always call free_irq()
> on any interrupt for which it previously called request_irq().
> Failure to do so results in a BUG_ON(), leaving the device with
> MSI enabled and thus leaking its vector."
>
> So I need the free_irq, but the device may still have open fds and therefore
> be around a bit longer waiting for them to be released.
Note, please wrap your email lines...
Anyway, ok, that makes sense. But put in a big comment in here about
this so that someone doesn't try to convert it to devm_* in the future.
Can you do this in a separate patch first as well? Burrying it in a
pointer conversion patch isn't good.
> >> err_device_create:
> >> uio_free_minor(idev);
> >> return ret;
> >> @@ -871,8 +892,10 @@ void uio_unregister_device(struct uio_info *info)
> >>
> >> uio_dev_del_attributes(idev);
> >>
> >> - device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
> >> + device_unregister(&idev->dev);
> >> + free_irq(idev->info->irq, idev);
> >>
> >> + idev->info = NULL;
> >
> > Why? If this was the last reference count, isn't idev now gone? You
> > shouldn't be referencing it anymore.
> >
>
> No, the device can be unregistered here but still have outstanding refcount
> because of open fds. The count can get to zero in the release function, so
> idev could still be around after this but the calling module could free info.
Yes, but if it was the last reference, idev is now a stale pointer and
you just derefrenced it. Twice. :(
Once you do a put, and don't have a reference on the pointer, you can't
touch that pointer again.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
