2006-01-02 15:39:51

by Jason Dravet

[permalink] [raw]
Subject: [RFC]: add sysfs support to parport_pc, v3

Here is a new patch to parport_pc that adds sysfs and thus udev support to
parport_pc. I fixed my earilier problem of the kernel oops (I forgot the
class_destory) and I can insmod and rmmod this module all day long with no
side effects. I do have one question why do both lp and parport nodes have
to be created?

What do you think of this patch? What would be the next step to get this
into the kernel?

Thanks,
Jason Dravet

signed-off-by: Jason Dravet <[email protected]>

--- /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 2006-01-01
11:29:05.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 <[email protected]>
+ * Added sysfs and udev - Jason Dravet <[email protected]>
*/

/* 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,12 @@ static void __exit parport_pc_exit(void)
if (pnp_registered_parport)
pnp_unregister_driver (&parport_pc_pnp_driver);

+ for (countports--; countports >=0; countports--) {
+ class_device_destroy(parallel_class, MKDEV(99, countports));
+ class_device_destroy(parallel_class, MKDEV(6, countports));
+ }
+ class_destroy(parallel_class);
+
spin_lock(&ports_lock);
while (!list_empty(&ports_list)) {
struct parport_pc_private *priv;



2006-01-02 19:50:12

by Marko Kohtala

[permalink] [raw]
Subject: Re: [Linux-parport] [RFC]: add sysfs support to parport_pc, v3

2006/1/2, Jason Dravet <[email protected]>:
> Here is a new patch to parport_pc that adds sysfs and thus udev support to
> parport_pc. I fixed my earilier problem of the kernel oops (I forgot the
> class_destory) and I can insmod and rmmod this module all day long with no
> side effects. I do have one question why do both lp and parport nodes have
> to be created?
>
> What do you think of this patch? What would be the next step to get this
> into the kernel?

If your problem is that you do not get /dev/lp0 created automatically,
I think a better way to get that solved is to find out a way that the
module lp is loaded automatically when a printer is found on the
parport.

Maybe you should look into drivers/parport/probe.c and have it do
request_module("lp") when it finds a printer attached to the parport.

Some paride (or SCSI) devices could also be probed for using a
pre-IEEE1284 daisy chain command that gives a 16 bit device ID, and
proper drivers loaded with some module aliases including the device
ID. If something unidentifiable is found on the parport then maybe the
ppdev could be loaded so that userspace drivers will have an interface
to work with.

I fear that some people would find this too smart to be useful. I'd
expect them to request at least a module option to parport to disable
it.

Most people are happy with just "lp" in /etc/modules.

2006-01-04 01:08:51

by Greg KH

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

On Mon, Jan 02, 2006 at 09:39:50AM -0600, Jason Dravet wrote:
> Here is a new patch to parport_pc that adds sysfs and thus udev support to
> parport_pc. I fixed my earilier problem of the kernel oops (I forgot the
> class_destory) and I can insmod and rmmod this module all day long with no
> side effects. I do have one question why do both lp and parport nodes have
> to be created?
>
> What do you think of this patch? What would be the next step to get this
> into the kernel?
>
> Thanks,
> Jason Dravet
>
> signed-off-by: Jason Dravet <[email protected]>

"Signed-off-by:"

>
> --- /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 2006-01-01
> 11:29:05.000000000 -0600

Line wrapped so it can't be applied :(

> @@ -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 <[email protected]>
> + * Added sysfs and udev - Jason Dravet <[email protected]>
> */

Doesn't belong here, this goes in the change log.

>
> /* 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>

Your email client also ate the leading spaces :(

>
> #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++;

What does the 6 and 99 come from? Aren't these #defined in a header
file somewhere?

thanks,

greg k-h

2006-01-04 03:24:40

by Jason Dravet

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

>From: Greg KH <[email protected]>
> > +++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c 2006-01-01
> > 11:29:05.000000000 -0600
>
>Line wrapped so it can't be applied :(
>
> > + * Added sysfs and udev - Jason Dravet <[email protected]>
> > */
>
>Doesn't belong here, this goes in the change log.
My mistake.

> > +#include <linux/sysfs.h>
>
>Your email client also ate the leading spaces :(
My email client is hotmail. I have come to realize how much hotmail sucks
for email. It eats the leading spaces, converts tabs to spaces, and line
wraps. I will work on getting a new mail account.

> > + 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++;
>
>What does the 6 and 99 come from? Aren't these #defined in a header file
>somewhere?
Good question. The answer is I have no idea. I booted Fedora Core and I
did a ls -l /dev and wrote down the specs for lp0 and parport0. lp0 looked
like crw-rw---- root lp 6, 0 date lp0 so that is what I used. After
reading Linux Device Drivers I found out that 6 is the major number and 0 is
the minor number. The goal was to make sure that the nodes generated by my
patch were the same as the nodes generated before my patch so I used those
numbers. Should I have used different major numbers?

Thanks,
Jason


2006-01-04 09:14:55

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

>> somewhere?
> Good question. The answer is I have no idea. I booted Fedora Core and I did a
> ls -l /dev and wrote down the specs for lp0 and parport0. lp0 looked like
> crw-rw---- root lp 6, 0 date lp0 so that is what I used. After reading
> Linux Device Drivers I found out that 6 is the major number and 0 is the minor
> number. The goal was to make sure that the nodes generated by my patch were
> the same as the nodes generated before my patch so I used those numbers.
> Should I have used different major numbers?

You should have at least used the constants from <linux/major.h>


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

2006-01-04 22:33:39

by Andrew Morton

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

"Jason Dravet" <[email protected]> wrote:
>
> >From: Greg KH <[email protected]>
> > > +++ /usr/src/linux-2.6.14/drivers/parport/parport_pc.c 2006-01-01
> > > 11:29:05.000000000 -0600
> >
> >Line wrapped so it can't be applied :(
> >
> > > + * Added sysfs and udev - Jason Dravet <[email protected]>
> > > */
> >
> >Doesn't belong here, this goes in the change log.
> My mistake.
>
> > > +#include <linux/sysfs.h>
> >
> >Your email client also ate the leading spaces :(
> My email client is hotmail. I have come to realize how much hotmail sucks
> for email. It eats the leading spaces, converts tabs to spaces, and line
> wraps. I will work on getting a new mail account.
>
> > > + 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++;
> >
> >What does the 6 and 99 come from? Aren't these #defined in a header file
> >somewhere?
> Good question. The answer is I have no idea. I booted Fedora Core and I
> did a ls -l /dev and wrote down the specs for lp0 and parport0. lp0 looked
> like crw-rw---- root lp 6, 0 date lp0 so that is what I used. After
> reading Linux Device Drivers I found out that 6 is the major number and 0 is
> the minor number. The goal was to make sure that the nodes generated by my
> patch were the same as the nodes generated before my patch so I used those
> numbers. Should I have used different major numbers?
>

6 is OK - it's LP_MAJOR.

However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c. And yet my
/dev/parport0 is also 99:0 (RH 7.3 and RH FC1). I've no idea how that came
about??

bix:/home/akpm> grep parport /etc/makedev.d/*
/etc/makedev.d/generic:a generic parport
/etc/makedev.d/linux-2.4.x:c $PRINTER 99 0 1 8 parport%d

2006-01-05 16:17:56

by Marko Kohtala

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

2006/1/5, Andrew Morton <[email protected]>:
> "Jason Dravet" <[email protected]> wrote:
> > >From: Greg KH <[email protected]>
> > > > + * Added sysfs and udev - Jason Dravet <[email protected]>
> > > > */
>
> 6 is OK - it's LP_MAJOR.
>
> However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c. And yet my
> /dev/parport0 is also 99:0 (RH 7.3 and RH FC1). I've no idea how that came
> about??
>
> bix:/home/akpm> grep parport /etc/makedev.d/*
> /etc/makedev.d/generic:a generic parport
> /etc/makedev.d/linux-2.4.x:c $PRINTER 99 0 1 8 parport%d

JSFD is a block device so tha majors are ok. I'm not just sure if the
PP_MAJOR from linux/ppdev.h should be moved to major.h.

The patch by Jason however is not ok. He had another problem and this
is not the fix. What he tries to do is already in lp and ppdev, where
I think they belong.

There is also something weird: why does RedHat create these nodes in
/dev when udev already does that.

2006-01-05 21:25:20

by Kay Sievers

[permalink] [raw]
Subject: Re: [RFC]: add sysfs support to parport_pc, v3

On Thu, Jan 05, 2006 at 06:17:51PM +0200, Marko Kohtala wrote:
> 2006/1/5, Andrew Morton <[email protected]>:
> > "Jason Dravet" <[email protected]> wrote:
> > > >From: Greg KH <[email protected]>
> > > > > + * Added sysfs and udev - Jason Dravet <[email protected]>
> > > > > */
> >
> > 6 is OK - it's LP_MAJOR.
> >
> > However 99 is JSFD_MAJOR, used by drivers/sbus/char/jsflash.c. And yet my
> > /dev/parport0 is also 99:0 (RH 7.3 and RH FC1). I've no idea how that came
> > about??
> >
> > bix:/home/akpm> grep parport /etc/makedev.d/*
> > /etc/makedev.d/generic:a generic parport
> > /etc/makedev.d/linux-2.4.x:c $PRINTER 99 0 1 8 parport%d
>
> JSFD is a block device so tha majors are ok. I'm not just sure if the
> PP_MAJOR from linux/ppdev.h should be moved to major.h.
>
> The patch by Jason however is not ok. He had another problem and this
> is not the fix. What he tries to do is already in lp and ppdev, where
> I think they belong.

Yeah, I was wondering about it too.

> There is also something weird: why does RedHat create these nodes in
> /dev when udev already does that.

Probably cause they want to be safe if nothing from their sysconfig loads
the module. Opening the node will autoload it then.

Kay