Re: [PATCH] don't create regular files in devfs (fwd)

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

Hi Christoph,

I don't know about mtrr (probably does it for the same reason) but the
reason why microcode driver uses a regular file is because it needs
something that only regular files can provide --- the file size.

This is an easy external "signifier" as to whether a successfull microcode
update has occurred or not.

Regards
Tigran

On Mon, 13 Jan 2003, Hugh Dickins wrote:

> FYI...
>
> ---------- Forwarded message ----------
> Date: Sat, 11 Jan 2003 20:56:00 +0100
> From: Christoph Hellwig <hch@lst.de>
> To: torvalds@transmeta.com
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH] don't create regular files in devfs
>
> Even more devfs creptomancy :)
>
> When devfs is enabled the i386 microcode and mtrr drivers try to create
> regular files in devfs in addition to their regular interfaces
> (miscdevice and procfs).  The tools work with the normal interfaces
> anyway in non-devfs systems and regular files in _dev_fs are a rather
> strange concept..
>
> Get rid of it.
>
>
> --- 1.15/arch/i386/kernel/microcode.c	Thu Dec  5 21:56:34 2002
> +++ edited/arch/i386/kernel/microcode.c	Sat Jan 11 16:50:00 2003
> @@ -65,7 +65,6 @@
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include <linux/miscdevice.h>
> -#include <linux/devfs_fs_kernel.h>
>  #include <linux/spinlock.h>
>  #include <linux/mm.h>
>
> @@ -122,39 +121,14 @@
>  	.fops	= &microcode_fops,
>  };
>
> -static devfs_handle_t devfs_handle;
> -
>  static int __init microcode_init(void)
>  {
> -	int error;
> -
> -	error = misc_register(&microcode_dev);
> -	if (error)
> -		printk(KERN_WARNING
> -			"microcode: can't misc_register on minor=%d\n",
> -			MICROCODE_MINOR);
> -
> -	devfs_handle = devfs_register(NULL, "cpu/microcode",
> -			DEVFS_FL_DEFAULT, 0, 0, S_IFREG | S_IRUSR | S_IWUSR,
> -			&microcode_fops, NULL);
> -	if (devfs_handle == NULL && error) {
> -		printk(KERN_ERR "microcode: failed to devfs_register()\n");
> -		misc_deregister(&microcode_dev);
> -		goto out;
> -	}
> -	error = 0;
> -	printk(KERN_INFO
> -		"IA-32 Microcode Update Driver: v%s <tigran@veritas.com>\n",
> -		MICROCODE_VERSION);
> -
> -out:
> -	return error;
> +	return misc_register(&microcode_dev);
>  }
>
>  static void __exit microcode_exit(void)
>  {
>  	misc_deregister(&microcode_dev);
> -	devfs_unregister(devfs_handle);
>  	if (mc_applied)
>  		kfree(mc_applied);
>  	printk(KERN_INFO "IA-32 Microcode Update Driver v%s unregistered\n",
> @@ -373,7 +347,6 @@
>  		ret = (ssize_t)len;
>  	}
>  out_fsize:
> -	devfs_set_file_size(devfs_handle, mc_fsize);
>  	vfree(microcode);
>  out_unlock:
>  	up_write(&microcode_rwsem);
> @@ -389,7 +362,6 @@
>  			if (mc_applied) {
>  				int bytes = NR_CPUS * sizeof(struct microcode);
>
> -				devfs_set_file_size(devfs_handle, 0);
>  				kfree(mc_applied);
>  				mc_applied = NULL;
>  				printk(KERN_INFO "microcode: freed %d bytes\n", bytes);
> --- 1.4/arch/i386/kernel/cpu/mtrr/if.c	Sat Dec 21 22:17:44 2002
> +++ edited/arch/i386/kernel/cpu/mtrr/if.c	Sat Jan 11 16:50:35 2003
> @@ -1,6 +1,5 @@
>  #include <linux/init.h>
>  #include <linux/proc_fs.h>
> -#include <linux/devfs_fs_kernel.h>
>  #include <linux/ctype.h>
>  #include <linux/module.h>
>  #include <linux/seq_file.h>
> @@ -300,8 +299,6 @@
>
>  #  endif			/*  CONFIG_PROC_FS  */
>
> -static devfs_handle_t devfs_handle;
> -
>  char * attrib_to_str(int x)
>  {
>  	return (x <= 6) ? mtrr_strings[x] : "?";
> @@ -337,7 +334,6 @@
>  			     attrib_to_str(type), usage_table[i]);
>  		}
>  	}
> -	devfs_set_file_size(devfs_handle, len);
>  	return 0;
>  }
>
> @@ -350,11 +346,6 @@
>  		proc_root_mtrr->owner = THIS_MODULE;
>  		proc_root_mtrr->proc_fops = &mtrr_fops;
>  	}
> -#endif
> -#ifdef USERSPACE_INTERFACE
> -	devfs_handle = devfs_register(NULL, "cpu/mtrr", DEVFS_FL_DEFAULT, 0, 0,
> -				      S_IFREG | S_IRUGO | S_IWUSR,
> -				      &mtrr_fops, NULL);
>  #endif
>  	return 0;
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH] don't create regular files in devfs (fwd)

[Christoph Hellwig]

On Tue, Jan 14, 2003 at 09:34:52AM +0000, Tigran Aivazian wrote:
> Hi Christoph,
> 
> I don't know about mtrr (probably does it for the same reason) but the
> reason why microcode driver uses a regular file is because it needs
> something that only regular files can provide --- the file size.
> 
> This is an easy external "signifier" as to whether a successfull microcode
> update has occurred or not.

It seems to work without that feature on systems that don't have devfs..

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

On Tue, 14 Jan 2003, Christoph Hellwig wrote:

> On Tue, Jan 14, 2003 at 09:34:52AM +0000, Tigran Aivazian wrote:
> > Hi Christoph,
> >
> > I don't know about mtrr (probably does it for the same reason) but the
> > reason why microcode driver uses a regular file is because it needs
> > something that only regular files can provide --- the file size.
> >
> > This is an easy external "signifier" as to whether a successfull microcode
> > update has occurred or not.
>
> It seems to work without that feature on systems that don't have devfs..

Of course it works without it. I never said it is a required feature, only
a very nice to have. Namely:

a) if devfs is available it provides a regular file whose size can be
examined by applications and content read/written without much "fuss". In
particular it is very convenient to say "vi microcode" and examine the
content directly. If it was a device node then this would have been
impossible.

b) if devfs is not available then a limited basic functionality is
provided --- i.e. just update the microcode on CPU(s) and write the log
messages in the kernel log.

Btw, having a size of mtrr is also useful for a similar reason.

Regards
Tigran

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

btw, I assumed that by "It" you meant the actual act of updating microcode
on cpus. If you were referring to the ability to manipulate microcode file
that I described, then "it" certainly does _not_ work if no devfs is
present.

Unless the world has been turned upside down and device nodes now have a
filesize stored somewhere? :)

Regards
Tigran

On Tue, 14 Jan 2003, Tigran Aivazian wrote:

> On Tue, 14 Jan 2003, Christoph Hellwig wrote:
>
> > On Tue, Jan 14, 2003 at 09:34:52AM +0000, Tigran Aivazian wrote:
> > > Hi Christoph,
> > >
> > > I don't know about mtrr (probably does it for the same reason) but the
> > > reason why microcode driver uses a regular file is because it needs
> > > something that only regular files can provide --- the file size.
> > >
> > > This is an easy external "signifier" as to whether a successfull microcode
> > > update has occurred or not.
> >
> > It seems to work without that feature on systems that don't have devfs..
>
> Of course it works without it. I never said it is a required feature, only
> a very nice to have. Namely:
>
> a) if devfs is available it provides a regular file whose size can be
> examined by applications and content read/written without much "fuss". In
> particular it is very convenient to say "vi microcode" and examine the
> content directly. If it was a device node then this would have been
> impossible.
>
> b) if devfs is not available then a limited basic functionality is
> provided --- i.e. just update the microcode on CPU(s) and write the log
> messages in the kernel log.
>
> Btw, having a size of mtrr is also useful for a similar reason.
>
> Regards
> Tigran
>
>

Re: [PATCH] don't create regular files in devfs (fwd)

[Christoph Hellwig]

On Tue, Jan 14, 2003 at 11:48:58AM +0000, Tigran Aivazian wrote:
> a) if devfs is available it provides a regular file whose size can be
> examined by applications and content read/written without much "fuss". In
> particular it is very convenient to say "vi microcode" and examine the
> content directly. If it was a device node then this would have been
> impossible.

What do you think about adding a sysvfs attribute for that instead in
2.5?  This seems to be the much more logical interface to me..

Re: [PATCH] don't create regular files in devfs (fwd)

[Patrick Mochel]

On 14 Jan 2003, Arjan van de Ven wrote:

> On Tue, 2003-01-14 at 17:32, Tigran Aivazian wrote:
> 
> > If you move it all the way to sysfs (i.e. no device node in /dev) then it
> > seems a bit odd that a device driver entry point is found somewhere other
> > than the usual /dev.
> 
> well
> 
> cat firmware > /sys/processor/0/microcode

Out of curiosity, how large are the microcode updates? Or rather, what is 
the range of sizes that you've seen?


	-pat

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

On Tue, 14 Jan 2003, Christoph Hellwig wrote:
> On Tue, Jan 14, 2003 at 11:48:58AM +0000, Tigran Aivazian wrote:
> > a) if devfs is available it provides a regular file whose size can be
> > examined by applications and content read/written without much "fuss". In
> > particular it is very convenient to say "vi microcode" and examine the
> > content directly. If it was a device node then this would have been
> > impossible.
>
> What do you think about adding a sysvfs attribute for that instead in
> 2.5?  This seems to be the much more logical interface to me..

Ok, that is a reasonable constructive suggestion. The only disadvantage I
see is:

sysfs is mounted under /sys and having two distinct paths (a device node
/dev/cpu/microcode and a regular file somewhere in /sys) does not seem
worthwhile. Not necessarily illogical, just not worth the hassle, imho.

I think one filename is sufficient in this case. The fact that the same
filename may sometimes refer to a device node and other times (if devfs is
present) to a regular file may seem a little odd but it is not harmful and
since it is useful then why remove it or change it?

If you move it all the way to sysfs (i.e. no device node in /dev) then it
seems a bit odd that a device driver entry point is found somewhere other
than the usual /dev.

Regards
Tigran

Re: [PATCH] don't create regular files in devfs (fwd)

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

On Tue, 2003-01-14 at 17:32, Tigran Aivazian wrote:

> If you move it all the way to sysfs (i.e. no device node in /dev) then it
> seems a bit odd that a device driver entry point is found somewhere other
> than the usual /dev.

well

cat firmware > /sys/processor/0/microcode

is obviously the ultimate interface for this ;)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

On 14 Jan 2003, Arjan van de Ven wrote:

> On Tue, 2003-01-14 at 17:32, Tigran Aivazian wrote:
>
> > If you move it all the way to sysfs (i.e. no device node in /dev) then it
> > seems a bit odd that a device driver entry point is found somewhere other
> > than the usual /dev.
>
> well
>
> cat firmware > /sys/processor/0/microcode
>
> is obviously the ultimate interface for this ;)

No, because cat is using 4K chunks and the data has to be written in one
large chunk, like this:

# dd if=microcode of=/dev/cpu/microcode bs=141312 count=1

This actually works fine but you need to convert microcode data from human
readable (what Intel distribute) to binary format first. Easily done with
microcode_ctl utility.

Regards
Tigran

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

On Tue, 14 Jan 2003, Patrick Mochel wrote:
> Out of curiosity, how large are the microcode updates? Or rather, what is
> the range of sizes that you've seen?

Each chunk is 2048 bytes long of which only 2000 bytes are the data bits
sent to the cpus. See struct microcode in <asm/processor.h>

Regards
Tigran

Re: [PATCH] don't create regular files in devfs (fwd)

[Dave Jones]

On Tue, Jan 14, 2003 at 05:29:58PM +0000, Tigran Aivazian wrote:

 > No, because cat is using 4K chunks and the data has to be written in one
 > large chunk, like this:
 > 
 > # dd if=microcode of=/dev/cpu/microcode bs=141312 count=1
 > 
 > This actually works fine but you need to convert microcode data from human
 > readable (what Intel distribute) to binary format first. Easily done with
 > microcode_ctl utility.

What about the dumps Christian Ludhoff put at ftp.sandpile.org/mcupdate ?
These are binary data, but are they in the right format to be used ?
I'm curious if these are newer than the ones described in microcode.dat,
but haven't had time to dig through the dates on them.

		Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] don't create regular files in devfs (fwd)

[Tigran Aivazian]

On Tue, 14 Jan 2003, Dave Jones wrote:

> On Tue, Jan 14, 2003 at 05:29:58PM +0000, Tigran Aivazian wrote:
>
>  > No, because cat is using 4K chunks and the data has to be written in one
>  > large chunk, like this:
>  >
>  > # dd if=microcode of=/dev/cpu/microcode bs=141312 count=1
>  >
>  > This actually works fine but you need to convert microcode data from human
>  > readable (what Intel distribute) to binary format first. Easily done with
>  > microcode_ctl utility.
>
> What about the dumps Christian Ludhoff put at ftp.sandpile.org/mcupdate ?
> These are binary data, but are they in the right format to be used ?
> I'm curious if these are newer than the ones described in microcode.dat,
> but haven't had time to dig through the dates on them.
>

Most likely they are NOT newer than the ones I receive from Intel (because
I assume Intel distribute their updates more or less simultaneously to all
OS/BIOS vendors). As soon as I get updates from Intel they will be
uploaded for general consumption, of course.

Regards
Tigran