| Message ID | 1376384922-8519-3-git-send-email-b.spranger@linutronix.de |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Aug 13, 2013 at 11:08:36AM +0200, Benedikt Spranger wrote: > If an UIO device is created by another driver (MFD for instance) it has to be > ensured that the MFD driver isn't removed while the UIO is still in use. Shouldn't if the MFD driver is removed, the UIO device will be cleaned up and also removed? You shouldn't need a module reference for this type of thing. And why did I only get 2 of the 7 patches here? greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Tue, 13 Aug 2013 10:48:14 -0700 schrieb Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > On Tue, Aug 13, 2013 at 11:08:36AM +0200, Benedikt Spranger wrote: > > If an UIO device is created by another driver (MFD for instance) it has to be > > ensured that the MFD driver isn't removed while the UIO is still in use. > > Shouldn't if the MFD driver is removed, the UIO device will be cleaned > up and also removed? That is part of the problem: 1) MFD driver creates platform device "uio_pdrv" 2) uio_pdrv creates "UIOX" 3) userspace opens "UIOX" 4) MFD driver unload (remove platform device "uio_pdrv") 5) userspace reads from "UIOX" --> crash > You shouldn't need a module reference for this type of thing. The driver uio_pdrv has no chance to recognize that the underlaying platform device has gone. Regards Benedikt Spranger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 14, 2013 at 09:19:46AM +0200, Benedikt Spranger wrote: > Am Tue, 13 Aug 2013 10:48:14 -0700 > schrieb Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > > On Tue, Aug 13, 2013 at 11:08:36AM +0200, Benedikt Spranger wrote: > > > If an UIO device is created by another driver (MFD for instance) it has to be > > > ensured that the MFD driver isn't removed while the UIO is still in use. > > > > Shouldn't if the MFD driver is removed, the UIO device will be cleaned > > up and also removed? > That is part of the problem: > > 1) MFD driver creates platform device "uio_pdrv" > 2) uio_pdrv creates "UIOX" > 3) userspace opens "UIOX" > 4) MFD driver unload (remove platform device "uio_pdrv") How can this happen in a normal situation? Modules do not simply unload themselves :) > 5) userspace reads from "UIOX" --> crash Step 4 should have told UIO that it was gone and had it shut everything down properly, so that there would not be a crash. > > You shouldn't need a module reference for this type of thing. > The driver uio_pdrv has no chance to recognize that the underlaying platform > device has gone. The mfd driver could tell it that it is gone, right? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 14 Aug 2013 09:33:11 -0700 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > Step 4 should have told UIO that it was gone and had it shut everything > down properly, so that there would not be a crash. The MFD driver only knows about a specific MFD cell. Through enable/disable callbacks the driver could tell UIO ...hm... whom? what? Neither the MFD driver nor the MFD core knows something about a specific UIO driver. But only that specific UIO driver knows about the device node activities. > > > You shouldn't need a module reference for this type of thing. > > The driver uio_pdrv has no chance to recognize that the underlaying platform > > device has gone. > The mfd driver could tell it that it is gone, right? It could tell, but whom and how? Regards Benedikt Spranger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 08:42:21AM +0200, Benedikt Spranger wrote: > On Wed, 14 Aug 2013 09:33:11 -0700 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > Step 4 should have told UIO that it was gone and had it shut everything > > down properly, so that there would not be a crash. > The MFD driver only knows about a specific MFD cell. Through > enable/disable callbacks the driver could tell UIO ...hm... whom? what? > > Neither the MFD driver nor the MFD core knows something about a specific > UIO driver. But only that specific UIO driver knows about the device > node activities. > > > > > You shouldn't need a module reference for this type of thing. > > > The driver uio_pdrv has no chance to recognize that the underlaying platform > > > device has gone. > > The mfd driver could tell it that it is gone, right? > It could tell, but whom and how? Hm. Ah, doesn't this work like PCI, when a PCI device is removed from the system, reads just start returning all 0xFF, so the userspace UIO driver now knows the device is gone from the system. Doesn't MFD hardware work the same way? Why would removing the MFD driver affect UIO at all, as it's just an interrupt and memory, both of which are controlled by UIO, not MFD at all. confused, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 14 Aug 2013 23:59:36 -0700 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > Hm. Ah, doesn't this work like PCI, when a PCI device is removed from > the system, reads just start returning all 0xFF, so the userspace UIO > driver now knows the device is gone from the system. Doesn't MFD > hardware work the same way? Why would removing the MFD driver affect > UIO at all, as it's just an interrupt and memory, both of which are > controlled by UIO, not MFD at all. > > confused, Sorry for the confusion. MFD core allocates the platform device structure and platform data dynamicaly and fill it up with appropriate data from the MFD cell data. And MFD core frees this memory on unregistering. Therefore the UIO driver needs to know that the underlaying platform device struct and platform device data are invalid and should not be used any more. The whole point are dynamicaly allocated devices and UIO. Regards Benedikt Spranger -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 09:27:53AM +0200, Benedikt Spranger wrote: > On Wed, 14 Aug 2013 23:59:36 -0700 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > Hm. Ah, doesn't this work like PCI, when a PCI device is removed from > > the system, reads just start returning all 0xFF, so the userspace UIO > > driver now knows the device is gone from the system. Doesn't MFD > > hardware work the same way? Why would removing the MFD driver affect > > UIO at all, as it's just an interrupt and memory, both of which are > > controlled by UIO, not MFD at all. > > > > confused, > Sorry for the confusion. > > MFD core allocates the platform device structure and platform data > dynamicaly and fill it up with appropriate data from the MFD cell data. > And MFD core frees this memory on unregistering. > > Therefore the UIO driver needs to know that the underlaying platform device > struct and platform device data are invalid and should not be used any more. > The whole point are dynamicaly allocated devices and UIO. Given that a UIO driver does need to specificly bind to a device (be it a PCI, OF, or something else), it should be the one that is notified when a device is removed. Again, just like PCI is handled today, why is MFD so "special" that it can't tell the drivers bound to its devices that it is going away? Do you have a specific example of an in-tree UIO driver that has this problem that I can look at to try to understand this better? Still confused, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/15/2013 10:09 AM, Greg Kroah-Hartman wrote: > Do you have a specific example of an in-tree UIO driver that has this > problem that I can look at to try to understand this better? grep for "uio_pdrv" and you find a few devices in arm and sh tree. Each one is created once at boot time and never removed. With mfd the device can be removed. If you look now at uio_write() then you will notice that it will deference idev->info->irqcontrol but once the device is gone the memory starting at info is gone, not to mention the code behind irqcontrol. > Still confused, > > greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 10:18:17AM +0200, Sebastian Andrzej Siewior wrote: > On 08/15/2013 10:09 AM, Greg Kroah-Hartman wrote: > > Do you have a specific example of an in-tree UIO driver that has this > > problem that I can look at to try to understand this better? > > grep for "uio_pdrv" and you find a few devices in arm and sh tree. Each > one is created once at boot time and never removed. With mfd the device > can be removed. But that's a "platform" device, for a resource that is described as not going away. If this is really a mfd device, then make your uio driver a mfd driver, not a platform driver for a resource that isn't under your control. > If you look now at uio_write() then you will notice that it will > deference idev->info->irqcontrol but once the device is gone the memory > starting at info is gone, not to mention the code behind irqcontrol. It sounds like the wrong uio driver is binding to this device, fix the uio driver and you should be fine, right? A module reference count will not "save" you from a device going away, only a code chunk going away. That is why no other subsystem has this type of thing. If you dynamically remove the mfd device, but not remove the module (i.e. through the sysfs files to do that), then you would still have this same problem, right? There's a reason the driver core doesn't deal with module reference counts, it's not the proper thing for devices. So I'm not willing to add it to the UIO code either, as it's not the correct thing for it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/15/2013 05:55 PM, Greg Kroah-Hartman wrote: > But that's a "platform" device, for a resource that is described as not > going away. > > If this is really a mfd device, then make your uio driver a mfd driver, > not a platform driver for a resource that isn't under your control. As you described it later yourself: You have the same problem if you manually unbind the platform_device from the driver while the device node is open. >> If you look now at uio_write() then you will notice that it will >> deference idev->info->irqcontrol but once the device is gone the memory >> starting at info is gone, not to mention the code behind irqcontrol. > > It sounds like the wrong uio driver is binding to this device, fix the > uio driver and you should be fine, right? For this to happen you would need a refcount in uio-core which learns that the device is gone and does not invoke any callbacks because the device is gone. Something like you have in USB where you return 0 on reads from ttyUSB after someone pulled the cable. > A module reference count will not "save" you from a device going away, > only a code chunk going away. That is why no other subsystem has this > type of thing. If you dynamically remove the mfd device, but not remove > the module (i.e. through the sysfs files to do that), then you would > still have this same problem, right? Yes, I think so. > There's a reason the driver core doesn't deal with module reference > counts, it's not the proper thing for devices. So I'm not willing to > add it to the UIO code either, as it's not the correct thing for it. okay. > > thanks, > > greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 06:03:44PM +0200, Sebastian Andrzej Siewior wrote: > On 08/15/2013 05:55 PM, Greg Kroah-Hartman wrote: > > But that's a "platform" device, for a resource that is described as not > > going away. > > > > If this is really a mfd device, then make your uio driver a mfd driver, > > not a platform driver for a resource that isn't under your control. > > As you described it later yourself: You have the same problem if you > manually unbind the platform_device from the driver while the device > node is open. No, at that point in time the remove function of the uio driver should be called, and you can invalidate everything then. Or the driver should be doing that, odds are, it needs to be fixed because no one checks for that :) > >> If you look now at uio_write() then you will notice that it will > >> deference idev->info->irqcontrol but once the device is gone the memory > >> starting at info is gone, not to mention the code behind irqcontrol. > > > > It sounds like the wrong uio driver is binding to this device, fix the > > uio driver and you should be fine, right? > > For this to happen you would need a refcount in uio-core which learns > that the device is gone and does not invoke any callbacks because the > device is gone. Something like you have in USB where you return 0 on > reads from ttyUSB after someone pulled the cable. That happens because we invalidate the filehandle in the tty layer by tearing everything down in the usb serial driver. And yes, uio also needs to do the same thing, if it doesn't already. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/15/2013 06:42 PM, Greg Kroah-Hartman wrote: >> For this to happen you would need a refcount in uio-core which learns >> that the device is gone and does not invoke any callbacks because the >> device is gone. Something like you have in USB where you return 0 on >> reads from ttyUSB after someone pulled the cable. > > That happens because we invalidate the filehandle in the tty layer by > tearing everything down in the usb serial driver. And yes, uio also > needs to do the same thing, if it doesn't already. Ah. Do you have a handy pointer where / how you do this? > > thanks, > > greg k-h Sebastian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 06:54:46PM +0200, Sebastian Andrzej Siewior wrote: > On 08/15/2013 06:42 PM, Greg Kroah-Hartman wrote: > >> For this to happen you would need a refcount in uio-core which learns > >> that the device is gone and does not invoke any callbacks because the > >> device is gone. Something like you have in USB where you return 0 on > >> reads from ttyUSB after someone pulled the cable. > > > > That happens because we invalidate the filehandle in the tty layer by > > tearing everything down in the usb serial driver. And yes, uio also > > needs to do the same thing, if it doesn't already. > > Ah. Do you have a handy pointer where / how you do this? The tty layer isn't exactly "handy" :) Somewhere we send a HANGUP signal to userspace, and I think we invalidate the file handle somehow, it's been years since I last looked at that code, sorry. greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index 3b96f18..9a95220 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -444,6 +444,11 @@ static int uio_open(struct inode *inode, struct file *filep) goto out; } + if (!try_module_get(idev->info->owner)) { + ret = -ENODEV; + goto err_get_module; + } + listener = kmalloc(sizeof(*listener), GFP_KERNEL); if (!listener) { ret = -ENOMEM; @@ -465,6 +470,9 @@ err_infoopen: kfree(listener); err_alloc_listener: + module_put(idev->info->owner); + +err_get_module: module_put(idev->owner); out: @@ -488,6 +496,7 @@ static int uio_release(struct inode *inode, struct file *filep) if (idev->info->release) ret = idev->info->release(idev->info, inode); + module_put(idev->info->owner); module_put(idev->owner); kfree(listener); return ret; diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h index 1ad4724..99f1696 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -80,6 +80,7 @@ struct uio_device; * @open: open operation for this uio device * @release: release operation for this uio device * @irqcontrol: disable/enable irqs when 0/1 is written to /dev/uioX + * @owner: module which created the uio device incase */ struct uio_info { struct uio_device *uio_dev; @@ -95,6 +96,7 @@ struct uio_info { int (*open)(struct uio_info *info, struct inode *inode); int (*release)(struct uio_info *info, struct inode *inode); int (*irqcontrol)(struct uio_info *info, s32 irq_on); + struct module *owner; }; extern int __must_check