[PATCH] lp: implement proper detach function for parport_driver lp

[PATCH] lp: implement proper detach function for parport_driver lp

[Hannes Weisbach]

From: Hannes Weisbach <hannes_weisbach@gmx.net>

The lp pardevice driver does not have a proper detach function. Consequently, parport_unregister_device() is not called when the underlying parport driver calls parport_remove_port(). As a result, the ref count of the parport driver's module does not go to zero and the driver module cannot be unloaded.
The attached patch unregisters all lp pardevices which are on the to-be-detached parport.

Signed-off-by: Hannes Weisbach <hannes_weisbach@gmx.net>
---
Granted, for normal parport drivers this is usually not an issue, because the device does not go away. However, I am currently writing a Linux device driver for a USB to parallel port converter [0] and therefore need proper detaching. Additionally, the wrong ref count keeps me from simply rmmod my driver and insmod a new version while developing and testing.

 drivers/char/lp.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 0913d79..57e6941 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -930,7 +930,17 @@ static void lp_attach (struct parport *port)
 
 static void lp_detach (struct parport *port)
 {
-	/* Write this some day. */
+	int offset;
+
+	for (offset = 0; offset < LP_NO; offset++) {
+		if (lp_table[offset].dev == NULL)
+			continue;
+		if (lp_table[offset].dev->port == port) {
+			device_destroy(lp_class, MKDEV(LP_MAJOR, offset));
+			parport_unregister_device(lp_table[offset].dev);
+		}
+	}
+
 #ifdef CONFIG_LP_CONSOLE
 	if (console_registered == port) {
 		unregister_console(&lpcons);

Re: [PATCH] lp: implement proper detach function for parport_driver lp

[Greg Kroah-Hartman]

On Sat, Jun 01, 2013 at 10:02:21PM +0200, Hannes Weisbach wrote:
> From: Hannes Weisbach <hannes_weisbach@gmx.net>
> 
> The lp pardevice driver does not have a proper detach function. Consequently, parport_unregister_device() is not called when the underlying parport driver calls parport_remove_port(). As a result, the ref count of the parport driver's module does not go to zero and the driver module cannot be unloaded.
> The attached patch unregisters all lp pardevices which are on the to-be-detached parport.

Please wrap your changelog comments at 72 columns or so.

> Signed-off-by: Hannes Weisbach <hannes_weisbach@gmx.net>
> ---
> Granted, for normal parport drivers this is usually not an issue,
> because the device does not go away. However, I am currently writing a
> Linux device driver for a USB to parallel port converter [0] and
> therefore need proper detaching. Additionally, the wrong ref count
> keeps me from simply rmmod my driver and insmod a new version while
> developing and testing.

Really?  We already have a usb to parallel port driver in the kernel
tree that seems to work just fine.  It's been there since the 2.3 kernel
days, so either it has the same problem, or your driver is doing
something odd.

Can you test the in-kernel driver and let me know if it has the same
problem?

thanks,

greg k-h

Re: [PATCH] lp: implement proper detach function for parport_driver lp

[Hannes Weisbach]

>> Signed-off-by: Hannes Weisbach <hannes_weisbach@gmx.net>
>> ---
>> Granted, for normal parport drivers this is usually not an issue,
>> because the device does not go away. However, I am currently writing a
>> Linux device driver for a USB to parallel port converter [0] and
>> therefore need proper detaching. Additionally, the wrong ref count
>> keeps me from simply rmmod my driver and insmod a new version while
>> developing and testing.
> 
> Really?  We already have a usb to parallel port driver in the kernel
> tree that seems to work just fine.

>  It's been there since the 2.3 kernel
> days, so either it has the same problem, or your driver is doing
> something odd.

Do you mean the USB Printer class driver for pseudo parallel port
adapters?  They don't use the char/lp.c printer driver.  (Or I didn't
see it).  I'm writing a driver for USB2LPT [0], which gives you a real
/dev/parportN-device in user space, with which you can do all the
bit-twiddling like with a real parallel port.

Or do you mean USS720 (misc/uss720.c) and MOS7715 (serial/mos7720.c)
drivers.  They are doing what I am doing: translating whatever the
user does on a /dev/parportN-node and sending device-specific commands
over USB.  When they do parport_announce_port(), lp.c should also be
initialized and they should have the same problem.

On second thought, my patch might not be optimal.  lp.c stores
instance-structs in an array of size 8.  So after 8 re-plugs, lp.c
will not instantiate any more printer devices.  I think I better go
all the way and replace that array with a list, to have a proper
solution.

[0] http://www-user.tu-chemnitz.de/~heha/bastelecke/Rund%20um%20den%20PC/USB2LPT/index.html.en

Re: [PATCH] lp: implement proper detach function for parport_driver lp

[Greg Kroah-Hartman]

On Wed, Jun 19, 2013 at 07:04:51PM +0200, Hannes Weisbach wrote:
> >> Signed-off-by: Hannes Weisbach <hannes_weisbach@gmx.net>
> >> ---
> >> Granted, for normal parport drivers this is usually not an issue,
> >> because the device does not go away. However, I am currently writing a
> >> Linux device driver for a USB to parallel port converter [0] and
> >> therefore need proper detaching. Additionally, the wrong ref count
> >> keeps me from simply rmmod my driver and insmod a new version while
> >> developing and testing.
> > 
> > Really?  We already have a usb to parallel port driver in the kernel
> > tree that seems to work just fine.
> 
> >  It's been there since the 2.3 kernel
> > days, so either it has the same problem, or your driver is doing
> > something odd.
> 
> Do you mean the USB Printer class driver for pseudo parallel port
> adapters?  They don't use the char/lp.c printer driver.  (Or I didn't
> see it).  I'm writing a driver for USB2LPT [0], which gives you a real
> /dev/parportN-device in user space, with which you can do all the
> bit-twiddling like with a real parallel port.
> 
> Or do you mean USS720 (misc/uss720.c) and MOS7715 (serial/mos7720.c)
> drivers.

Yes, I mean these.

> They are doing what I am doing: translating whatever the
> user does on a /dev/parportN-node and sending device-specific commands
> over USB.  When they do parport_announce_port(), lp.c should also be
> initialized and they should have the same problem.

Why hasn't anyone reported that problem then?  Surely someone must use
these, they've been in the kernel tree for over a decade now.

> On second thought, my patch might not be optimal.  lp.c stores
> instance-structs in an array of size 8.  So after 8 re-plugs, lp.c
> will not instantiate any more printer devices.  I think I better go
> all the way and replace that array with a list, to have a proper
> solution.

Proper solutions are always good :)

thanks,

greg k-h

Re: [PATCH] lp: implement proper detach function for parport_driver lp

[Hannes Weisbach]

Am 19.06.2013 um 20:32 schrieb Greg Kroah-Hartman:
> 
>> They are doing what I am doing: translating whatever the
>> user does on a /dev/parportN-node and sending device-specific commands
>> over USB.  When they do parport_announce_port(), lp.c should also be
>> initialized and they should have the same problem.
> 
> Why hasn't anyone reported that problem then?  Surely someone must use
> these, they've been in the kernel tree for over a decade now.
I don't know.  Maybe nobody uses them (anymore/ever)?  Nobody tried to
rmmod the kernel module or compiled the driver into the kernel?

The solution to the problem (if someone encountered it) was probably
policy-based: just don't unload the module.  The comment in lp.c is
clear: /* Write this some day.  */ ;)