RFC: add udev support to parport_pc

RFC: add udev support to parport_pc

[Jason Dravet]

Here is a patch to parport_pc.c that adds udev support.  Without it sysfs 
does not have enough information to give to udev so the lp and parport nodes 
can be created.  The only problem I have is the kernel_oops when I do the 
following: insmod parport, insmod parport_pc, rmmod parport_pc, rmmod 
parport, insmod parport, insmod parport_pc, rmmod parport_pc, kernel oops.  
>From the part of the traceback I can read the problem is in parport_pc_exit. 
  Any help in tracking down the problem would be appreciated.  If this is 
acceptable how can I get this added to the mainline?  This is my first patch 
so please be gentle.

Thanks,
Jason Dravet

Signed-off-by: Jason Dravet <dravet@hotmail.com>
--- /usr/src/linux-2.6.14/drivers/parport/parport_pc.c.orig	2005-12-30 
13:52:48.000000000 -0600
+++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2005-12-30 
19:56:20.000000000 -0600
@@ -55,6 +55,7 @@
#include <linux/pci.h>
#include <linux/pnp.h>
#include <linux/sysctl.h>
+#include <linux/sysfs.h>

#include <asm/io.h>
#include <asm/dma.h>
@@ -99,6 +100,8 @@ static struct superio_struct {	/* For Su
	int dma;
} superios[NR_SUPERIOS] __devinitdata = { {0,},};

+static struct class *parallel_class;
+
static int user_specified;
#if defined(CONFIG_PARPORT_PC_SUPERIO) || \
        (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
@@ -2232,6 +2235,25 @@ struct parport *parport_pc_probe_port (u
                                            is mandatory (see above) */
		p->dma = PARPORT_DMA_NONE;

+	parallel_class = class_create(THIS_MODULE, "lp");
+	if (p->base == 888) /* 888 is dec for 378h */
+	{
+		class_device_create(parallel_class, NULL, MKDEV(6, 0), NULL, "lp0");
+		class_device_create(parallel_class, NULL, MKDEV(99, 0), NULL, 
"parport0");
+	}
+
+	if (p->base == 632) /* 632 is dec for 278h */
+	{
+		class_device_create(parallel_class, NULL, MKDEV(6, 1), NULL, "lp1");
+		class_device_create(parallel_class, NULL, MKDEV(99, 1), NULL, 
"parport1");
+	}
+
+	if (p->base == 956) /* 956 is dec for 3BCh */
+	{
+		class_device_create(parallel_class, NULL, MKDEV(6, 2), NULL, "lp2");
+		class_device_create(parallel_class, NULL, MKDEV(99, 2), NULL, 
"parport2");
+	}
+
#ifdef CONFIG_PARPORT_PC_FIFO
	if (parport_ECP_supported(p) &&
	    p->dma != PARPORT_DMA_NOFIFO &&
@@ -3406,6 +3428,13 @@ static void __exit parport_pc_exit(void)
	if (pnp_registered_parport)
		pnp_unregister_driver (&parport_pc_pnp_driver);

+	class_device_destroy(parallel_class, MKDEV(99, 2));
+	class_device_destroy(parallel_class, MKDEV(6, 2));
+	class_device_destroy(parallel_class, MKDEV(99, 1));
+	class_device_destroy(parallel_class, MKDEV(6, 1));
+	class_device_destroy(parallel_class, MKDEV(99, 0));
+	class_device_destroy(parallel_class, MKDEV(6, 0));
+
	spin_lock(&ports_lock);
	while (!list_empty(&ports_list)) {
		struct parport_pc_private *priv;

Re: RFC: add udev support to parport_pc

[Jan Engelhardt]

> +	parallel_class = class_create(THIS_MODULE, "lp");
> +	if (p->base == 888) /* 888 is dec for 378h */

I would prefer to actually see == 0x378 in the code, because the 
hexademical number is what you see everywhere else, such as the BIOS POST 
and /proc/ioports. This also applies to 0x278 and 0x3BC below.

> +	{
> +		class_device_create(parallel_class, NULL, MKDEV(6, 0), NULL,
> "lp0");
> +		class_device_create(parallel_class, NULL, MKDEV(99, 0), NULL,
> "parport0");
> +	}

Background info before: Because I burnt my on-board LPT port (applying too 
much volts or milliamps), I bought a dual-slot PCI add-in card. This card 
provides "parport1" and "parport2" at ports at 0xC800 and 0xC00 
(/proc/ioports).

There are a number of problems in your code:

1- testing just for 0x378/0x278/0x3BC is not enough

2- parport0 could be 0xC800 (address may vary) if you do not
   have any onboard LPT ports.
    2=> that is why I think you should not reserver "lp0"/"parport0"
        for 0x378.

All this applies to the other chunks too of course...:

> +	if (p->base == 632) /* 632 is dec for 278h */
> +	{
> +		class_device_create(parallel_class, NULL, MKDEV(6, 1), NULL,
> "lp1");
> +		class_device_create(parallel_class, NULL, MKDEV(99, 1), NULL,
> "parport1");
> +	}
> +
> +	if (p->base == 956) /* 956 is dec for 3BCh */
> +	{
> +		class_device_create(parallel_class, NULL, MKDEV(6, 2), NULL,
> "lp2");
> +		class_device_create(parallel_class, NULL, MKDEV(99, 2), NULL,
> "parport2");
> +	}
> +


> +	class_device_destroy(parallel_class, MKDEV(99, 2));
> +	class_device_destroy(parallel_class, MKDEV(6, 2));
> +	class_device_destroy(parallel_class, MKDEV(99, 1));
> +	class_device_destroy(parallel_class, MKDEV(6, 1));
> +	class_device_destroy(parallel_class, MKDEV(99, 0));
> +	class_device_destroy(parallel_class, MKDEV(6, 0));
> +


Jan Engelhardt
-- 
| Alphagate Systems, http://alphagate.hopto.org/
| jengelh's site, http://jengelh.hopto.org/

Re: RFC: add udev support to parport_pc

[Jiri Slaby]

Jason Dravet napsal(a):
> Here is a patch to parport_pc.c that adds udev support.  Without it
> sysfs does not have enough information to give to udev so the lp and
> parport nodes can be created.  The only problem I have is the
> kernel_oops when I do the following: insmod parport, insmod parport_pc,
> rmmod parport_pc, rmmod parport, insmod parport, insmod parport_pc,
> rmmod parport_pc, kernel oops. 
[snip]
> +    if (p->base == 888) /* 888 is dec for 378h */
> +    {
> +        class_device_create(parallel_class, NULL, MKDEV(6, 0), NULL,
> "lp0");
> +        class_device_create(parallel_class, NULL, MKDEV(99, 0), NULL,
> "parport0");
> +    }
use please
if () {
}
instead of
if ()
{
}
like the surrounding code

thanks,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
\_.-^-._   jirislaby@gmail.com   _.-^-._/
B67499670407CE62ACC8 22A032CC55C339D47A7E

Re: RFC: add udev support to parport_pc

[Jason Dravet]

Thank you for the reply.  Comments inline.

>I would prefer to actually see == 0x378 in the code, because the
>hexademical number is what you see everywhere else, such as the BIOS POST
>and /proc/ioports. This also applies to 0x278 and 0x3BC below.
>
This is what I wanted but I could not figure out how do it.  If you tell me 
how I will be happy to change it.  I tried  if (p->base == 0x378) but then 
class_device_create does not get executed.

> > +	{
> > +		class_device_create(parallel_class, NULL, MKDEV(6, 0), NULL,
> > "lp0");
> > +		class_device_create(parallel_class, NULL, MKDEV(99, 0), NULL,
> > "parport0");
> > +	}
>
>Background info before: Because I burnt my on-board LPT port (applying too
>much volts or milliamps), I bought a dual-slot PCI add-in card. This card
>provides "parport1" and "parport2" at ports at 0xC800 and 0xC00
>(/proc/ioports).
>
The last experience I have with off board cards was about 5 years ago.  The 
choices for the two parallel ports were 378, 278, or 3BC.  I was not aware 
that you had flexibility now.
>
>There are a number of problems in your code:
>
>1- testing just for 0x378/0x278/0x3BC is not enough
>
>2- parport0 could be 0xC800 (address may vary) if you do not
>    have any onboard LPT ports.
>     2=> that is why I think you should not reserver "lp0"/"parport0"
>         for 0x378.
As I said above I was not aware todays off board parallel ports had more 
choices.  I will see what I can do to fix this.  Do you have any 
suggestions?

Thanks,
Jason

Re: RFC: add udev support to parport_pc

[Jason Dravet]

>From: Jiri Slaby <slaby@liberouter.org>
>To: Jason Dravet <dravet@hotmail.com>
>CC: linux-kernel@vger.kernel.org, linux-parport@lists.infradead.org
>Subject: Re: RFC: add udev support to parport_pc
>Date: Sat, 31 Dec 2005 18:59:15 +0100
>
>Jason Dravet napsal(a):
> > Here is a patch to parport_pc.c that adds udev support.  Without it
> > sysfs does not have enough information to give to udev so the lp and
> > parport nodes can be created.  The only problem I have is the
> > kernel_oops when I do the following: insmod parport, insmod parport_pc,
> > rmmod parport_pc, rmmod parport, insmod parport, insmod parport_pc,
> > rmmod parport_pc, kernel oops.
>[snip]
> > +    if (p->base == 888) /* 888 is dec for 378h */
> > +    {
> > +        class_device_create(parallel_class, NULL, MKDEV(6, 0), NULL,
> > "lp0");
> > +        class_device_create(parallel_class, NULL, MKDEV(99, 0), NULL,
> > "parport0");
> > +    }
>use please
>if () {
>}
>instead of
>if ()
>{
>}
>like the surrounding code
>
>thanks,
>--
>Jiri Slaby         www.fi.muni.cz/~xslaby

Will do.  Thanks,
Jason

Re: RFC: add udev support to parport_pc

[Jason Dravet]

Here is version 2 of the patch.  It should address the issues pointed out so 
far.

Thanks,
Jason

--- /usr/src/linux-2.6.14/drivers/parport/parport_pc.c.orig	2005-12-30 
13:52:48.000000000 -0600
+++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c	2005-12-31 
13:17:42.000000000 -0600
@@ -14,6 +14,7 @@
  * More PCI support now conditional on CONFIG_PCI, 03/2001, Paul G.
  * Various hacks, Fred Barnes, 04/2001
  * Updated probing logic - Adam Belay <ambx1@neo.rr.com>
+ * Added sysfs and udev - Jason Dravet <dravet@hotmail.com>
  */

/* This driver should work with any hardware that is broadly compatible
@@ -55,6 +56,7 @@
#include <linux/pci.h>
#include <linux/pnp.h>
#include <linux/sysctl.h>
+#include <linux/sysfs.h>

#include <asm/io.h>
#include <asm/dma.h>
@@ -99,6 +101,9 @@ static struct superio_struct {	/* For Su
	int dma;
} superios[NR_SUPERIOS] __devinitdata = { {0,},};

+static struct class *parallel_class;
+int countports = 0;
+
static int user_specified;
#if defined(CONFIG_PARPORT_PC_SUPERIO) || \
        (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
@@ -2232,6 +2237,11 @@ struct parport *parport_pc_probe_port (u
                                            is mandatory (see above) */
		p->dma = PARPORT_DMA_NONE;

+	parallel_class = class_create(THIS_MODULE, "lp");
+	class_device_create(parallel_class, NULL, MKDEV(6, countports), NULL, 
"lp%i", countports);
+	class_device_create(parallel_class, NULL, MKDEV(99, countports), NULL, 
"parport%i", countports);
+	countports++;
+
#ifdef CONFIG_PARPORT_PC_FIFO
	if (parport_ECP_supported(p) &&
	    p->dma != PARPORT_DMA_NOFIFO &&
@@ -3406,6 +3416,13 @@ static void __exit parport_pc_exit(void)
	if (pnp_registered_parport)
		pnp_unregister_driver (&parport_pc_pnp_driver);

+	printk (KERN_WARNING "countports = %i \n", countports);
+	for (countports--; countports >=0; countports--) {
+		printk (KERN_WARNING "countports loop = %i \n", countports);
+		class_device_destroy(parallel_class, MKDEV(99, countports));
+		class_device_destroy(parallel_class, MKDEV(6, countports));
+	}
+
	spin_lock(&ports_lock);
	while (!list_empty(&ports_list)) {
		struct parport_pc_private *priv;

Re: RFC: add udev support to parport_pc

[Jan Engelhardt]

>> I would prefer to actually see == 0x378 in the code, because the
>> hexademical number is what you see everywhere else, such as the BIOS POST
>> and /proc/ioports. This also applies to 0x278 and 0x3BC below.
>> 
> This is what I wanted but I could not figure out how do it.  If you tell me how
> I will be happy to change it.  I tried  if (p->base == 0x378) but then
> class_device_create does not get executed.
>
Sounds very strange to me that by changing a base-10 integer into base-16 
does not execute the branch anymore...

>> Background info before: Because I burnt my on-board LPT port (applying too
>> much volts or milliamps), I bought a dual-slot PCI add-in card. This card
>> provides "parport1" and "parport2" at ports at 0xC800 and 0xC00
>> (/proc/ioports).
>> 
> The last experience I have with off board cards was about 5 years ago.  The
> choices for the two parallel ports were 378, 278, or 3BC.  I was not aware that
> you had flexibility now.
>
Well, I can tell you even more: I also have an ISA LPT expansion card in an 
ol 386 that gets assigned 278 and 3BC respectively.


> As I said above I was not aware todays off board parallel ports had more
> choices.  I will see what I can do to fix this.  Do you have any suggestions?

I have not yet checked the BIOS POST message of the i686 where
the PCI LPT card is in... maybe it's just Linux that remaps
it to high ports. Gotta test it in Windows. BBL.

Anyway, as for the patch I think you should walk down the list of 
already-collected ioports (in userspace that would equal to `grep parport 
/proc/ioports`) and assign each of them consecutive device names.




Jan Engelhardt
--