[PATCH] Added devfs support for i386 msr/cpuid driver

[PATCH] Added devfs support for i386 msr/cpuid driver

[Per Niva]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3244 bytes --]


Hi all,

I added devfs support to the i386 msr/cpuid driver,
along the lines in the microcode/mtrr drivers.

Please CC me any replies personally, as I don't
regularly follow the list.


Regards,


	Per Niva


----------------------------------------------------
pna@mendosus.org                     +46 705 509 654
"Beware - the world will never be the same again..."



--- arch/i386/kernel/msr.c.orig	Wed Aug 22 18:24:39 2001
+++ arch/i386/kernel/msr.c	Wed Aug 22 17:35:24 2001
@@ -22,6 +22,11 @@
  *
  * This driver uses /dev/cpu/%d/msr where %d is the minor number, and on
  * an SMP box will direct the access to CPU %d.
+ *
+ * ChangeLog
+ *
+ * 2001-08-22   Per Niva <pna@mendosus.org>
+ *              Added support for devfs
  */

 #include <linux/module.h>
@@ -34,12 +39,17 @@
 #include <linux/poll.h>
 #include <linux/smp.h>
 #include <linux/major.h>
+#include <linux/devfs_fs_kernel.h>

 #include <asm/processor.h>
 #include <asm/msr.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>

+
+static devfs_handle_t devfs_handle;
+
+
 /* Note: "err" is handled in a funny way below.  Otherwise one version
    of gcc or another breaks. */

@@ -250,18 +260,28 @@

 int __init msr_init(void)
 {
+#ifdef CONFIG_DEVFS_FS
+    devfs_handle = devfs_register(NULL, "cpu/msr", DEVFS_FL_DEFAULT, 0, 0,
+                                  S_IFREG | S_IRUGO | S_IWUSR,
+                                  &msr_fops, NULL);
+#else
   if (register_chrdev(MSR_MAJOR, "cpu/msr", &msr_fops)) {
     printk(KERN_ERR "msr: unable to get major %d for msr\n",
 	   MSR_MAJOR);
     return -EBUSY;
   }
+#endif

   return 0;
 }

 void __exit msr_exit(void)
 {
+#ifdef CONFIG_DEVFS_FS
+    devfs_unregister(devfs_handle);
+#else
   unregister_chrdev(MSR_MAJOR, "cpu/msr");
+#endif
 }

 module_init(msr_init);



--- arch/i386/kernel/cpuid.c.orig	Wed Aug 22 18:24:31 2001
+++ arch/i386/kernel/cpuid.c	Wed Aug 22 18:18:03 2001
@@ -23,6 +23,11 @@
  *
  * This driver uses /dev/cpu/%d/cpuid where %d is the minor number, and on
  * an SMP box will direct the access to CPU %d.
+ *
+ * ChangeLog
+ *
+ * 2001-08-22   Per Niva <pna@mendosus.org>
+ *              Added support for devfs
  */

 #include <linux/module.h>
@@ -35,12 +40,17 @@
 #include <linux/poll.h>
 #include <linux/smp.h>
 #include <linux/major.h>
+#include <linux/devfs_fs_kernel.h>

 #include <asm/processor.h>
 #include <asm/msr.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>

+
+static devfs_handle_t devfs_handle;
+
+
 #ifdef CONFIG_SMP

 struct cpuid_command {
@@ -142,18 +152,27 @@

 int __init cpuid_init(void)
 {
+#ifdef CONFIG_DEVFS_FS
+    devfs_handle = devfs_register(NULL, "cpu/cpuid", DEVFS_FL_DEFAULT, 0, 0,
+                                  S_IFREG | S_IRUGO | S_IWUSR,
+                                  &cpuid_fops, NULL);
+#else
   if (register_chrdev(CPUID_MAJOR, "cpu/cpuid", &cpuid_fops)) {
     printk(KERN_ERR "cpuid: unable to get major %d for cpuid\n",
 	   CPUID_MAJOR);
     return -EBUSY;
   }
-
+#endif
   return 0;
 }

 void __exit cpuid_exit(void)
 {
-  unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
+#ifdef CONFIG_DEVFS_FS
+    devfs_unregister(devfs_handle);
+#else
+    unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
+#endif
 }

 module_init(cpuid_init);

[-- Attachment #2: Type: APPLICATION/x-gzip, Size: 763 bytes --]

[-- Attachment #3: Type: APPLICATION/x-gzip, Size: 725 bytes --]

Re: [PATCH] Added devfs support for i386 msr/cpuid driver

[Arjan van de Ven]

> 
>  int __init msr_init(void)
>  {
> +#ifdef CONFIG_DEVFS_FS
> +    devfs_handle = devfs_register(NULL, "cpu/msr", DEVFS_FL_DEFAULT, 0, 0,
> +                                  S_IFREG | S_IRUGO | S_IWUSR,
> +                                  &msr_fops, NULL);
> +#else
>    if (register_chrdev(MSR_MAJOR, "cpu/msr", &msr_fops)) {
>      printk(KERN_ERR "msr: unable to get major %d for msr\n",
>            MSR_MAJOR);
>      return -EBUSY;
>    }
> +#endif

this must be wrong as you don't check for devfs_register failures...



Also why devfs can't just use register_chrdev and not touch ALL drivers 
is beyond me, but it has been like that for a while now...

Re: [PATCH] Added devfs support for i386 msr/cpuid driver

[Richard Gooch]

Arjan van de Ven writes:
> > 
> >  int __init msr_init(void)
> >  {
> > +#ifdef CONFIG_DEVFS_FS
> > +    devfs_handle = devfs_register(NULL, "cpu/msr", DEVFS_FL_DEFAULT, 0, 0,
> > +                                  S_IFREG | S_IRUGO | S_IWUSR,
> > +                                  &msr_fops, NULL);
> > +#else
> >    if (register_chrdev(MSR_MAJOR, "cpu/msr", &msr_fops)) {
> >      printk(KERN_ERR "msr: unable to get major %d for msr\n",
> >            MSR_MAJOR);
> >      return -EBUSY;
> >    }
> > +#endif
> 
> this must be wrong as you don't check for devfs_register failures...

The reason it's wrong is because he put #ifdef's in there. The
functions should just be called unconditionally. The #ifdef's are in
the header.

> Also why devfs can't just use register_chrdev and not touch ALL
> drivers is beyond me, but it has been like that for a while now...

Huh?!? I thought that should be obvious. Think about it,
register_chrdev() registers a *major*, whereas devfs_register()
registers an individual device node. The former has no way of
representing which devices are detected, the latter sets up a 1:1
relationship between devices and device nodes (which is what people
actually want).

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] Added devfs support for i386 msr/cpuid driver

[Per Niva]

On Mon, 27 Aug 2001, Richard Gooch wrote:

> Arjan van de Ven writes:
> > >
> > >  int __init msr_init(void)
> > >  {
> > > +#ifdef CONFIG_DEVFS_FS
> > > +    devfs_handle = devfs_register(NULL, "cpu/msr", DEVFS_FL_DEFAULT, 0, 0,
> > > +                                  S_IFREG | S_IRUGO | S_IWUSR,
> > > +                                  &msr_fops, NULL);
> > > +#else
> > >    if (register_chrdev(MSR_MAJOR, "cpu/msr", &msr_fops)) {
> > >      printk(KERN_ERR "msr: unable to get major %d for msr\n",
> > >            MSR_MAJOR);
> > >      return -EBUSY;
> > >    }
> > > +#endif
> >
> > this must be wrong as you don't check for devfs_register failures...
>
> The reason it's wrong is because he put #ifdef's in there. The
> functions should just be called unconditionally. The #ifdef's are in
> the header.

I actually pondered a while on this, and settled on
the cut'n'paste-from-mtrr.c version. There is no error
check there, and I just overlooked it.

The defence for the #ifdefs is that I didn't see
register_chrdev() being aware of devfs, and I thought
we'd be better off just not calling register_chrdev()
at all if we have devfs.

It's not like I personally like #ifdefs, but it seemed
justified to my inexperienced eyes at that point. And
there's a #ifdef CONFIG_DEVFS_FS around the call in
mtrr.c too, and I thought it safe to do like what's
already in the official tree.

In microcode.c however, is the new-style without #ifdef
(or rather with the #ifdef in the headers instead)
and with error checking, but microcode_init() doesn't
use register_chrdev() anyway, even if devfs is not
supported.

Please enlighten me!

> 				Regards,
>
> 					Richard....


Grateful for comments,


	Per


----------------------------------------------------
pna@mendosus.org                     +46 705 509 654
"Beware - the world will never be the same again..."

Re: [PATCH] Added devfs support for i386 msr/cpuid driver

[Richard Gooch]

Per Niva writes:
> On Mon, 27 Aug 2001, Richard Gooch wrote:
> > The reason it's wrong is because he put #ifdef's in there. The
> > functions should just be called unconditionally. The #ifdef's are in
> > the header.
> 
> I actually pondered a while on this, and settled on
> the cut'n'paste-from-mtrr.c version. There is no error
> check there, and I just overlooked it.
> 
> The defence for the #ifdefs is that I didn't see
> register_chrdev() being aware of devfs, and I thought
> we'd be better off just not calling register_chrdev()
> at all if we have devfs.

No, even if CONFIG_DEVFS_FS=y, doesn't mean the user wants to
mount/use devfs, so you shouldn't disable register_chrdev().

> It's not like I personally like #ifdefs, but it seemed
> justified to my inexperienced eyes at that point. And
> there's a #ifdef CONFIG_DEVFS_FS around the call in
> mtrr.c too, and I thought it safe to do like what's
> already in the official tree.

The #ifdef in mtrr.c is there for historical reasons (at one point,
neither the mtrr or devfs patches were in the kernel, but I wanted
mtrr to use devfs if available, hence the #ifdef). The #ifdef can be
safely taken out (and should be).

> In microcode.c however, is the new-style without #ifdef
> (or rather with the #ifdef in the headers instead)
> and with error checking, but microcode_init() doesn't
> use register_chrdev() anyway, even if devfs is not
> supported.
> 
> Please enlighten me!

The microcode patch went in after devfs was in the kernel, IIRC, and
thus could unconditionally reference devfs_register().

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca