On 19/03/15 15:14, Greg Kroah-Hartman wrote:
> 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.
>
I've separated out the irq bits into a separate patch, which depends
on patching on top of the first patch. I'm a bit unsure how to go about
sending the update now the patches are broken out and only one featured
in the previous versions. Does this seek ok?
3 separate mails:
[PATCH v8 0/2] uio: Fix uio hotplug issues
summary of patches
[PATCH v8 1/2] uio: Fix uio driver to refcount device
original patch minus irq and include version changes
[PATCH v8 2/2] uio: Request/free irq separate from dev lifecycle
broken out irq bits, no other version info
... or should the v8 only go on the first patch?
Thanks,
Brian
>>>> 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/
