[PATCH] PPC64 iSeries viodasd proc file

[PATCH] PPC64 iSeries viodasd proc file

[Stephen Rothwell]

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

Hi Andrew,

This patch adds a proc file for viodasd so to make it
easier to enumerate the available disks.  It is in a
(somewhat) strange format to try for a simple level of
compatability with the old viodasd code (that was in a
couple of vendor's kernels).

Please apply (and for Linus' tree as well).

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

diff -ruN 2.6.7/drivers/block/viodasd.c 2.6.7.viodasd.1/drivers/block/viodasd.c
--- 2.6.7/drivers/block/viodasd.c	2004-06-16 22:15:21.000000000 +1000
+++ 2.6.7.viodasd.1/drivers/block/viodasd.c	2004-06-18 16:00:33.000000000 +1000
@@ -42,6 +42,8 @@
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
 
 #include <asm/uaccess.h>
 #include <asm/vio.h>
@@ -171,6 +173,34 @@
 } viodasd_devices[MAX_DISKNO];
 
 /*
+ * Proc file so that disks may be identified.  It looks like
+ * this in order to be (somewhat) compatible with the old code
+ * which just dumped statistics for each disk.
+ */
+static int viodasd_proc_show(struct seq_file *m, void *v)
+{
+	int i;
+
+	seq_printf(m, "viod %d possible devices\n", MAX_DISKNO);
+	for (i = 0; i < MAX_DISKNO; i++)
+		if (viodasd_devices[i].disk != NULL)
+			seq_printf(m, "DISK %2.2d:\n", i);
+	return 0;
+}
+
+static int viodasd_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, viodasd_proc_show, NULL);
+}
+
+static struct file_operations viodasd_proc_operations = {
+	.open		= viodasd_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+/*
  * External open entry point.
  */
 static int viodasd_open(struct inode *ino, struct file *fil)
@@ -755,6 +785,7 @@
 static int __init viodasd_init(void)
 {
 	int i;
+	struct proc_dir_entry *e;
 
 	/* Try to open to our host lp */
 	if (viopath_hostLp == HvLpIndexInvalid)
@@ -794,6 +825,12 @@
 	vio_register_driver(&viodasd_driver);	/* FIX ME - error checking */
 	driver_create_file(&viodasd_driver.driver, &driver_attr_probe);
 
+	e = create_proc_entry("iSeries/viodasd", S_IFREG|S_IRUGO, NULL);
+	if (e) {
+		e->owner = THIS_MODULE;
+		e->proc_fops = &viodasd_proc_operations;
+	}
+
 	return 0;
 }
 module_init(viodasd_init);
@@ -806,6 +843,7 @@
 	driver_remove_file(&viodasd_driver.driver, &driver_attr_probe);
 	vio_unregister_driver(&viodasd_driver);
 
+	remove_proc_entry("iSeries/viodasd", NULL);
         for (i = 0; i < MAX_DISKNO; i++) {
 		d = &viodasd_devices[i];
 		if (d->disk) {

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeff Garzik]

Stephen Rothwell wrote:
> Hi Andrew,
> 
> This patch adds a proc file for viodasd so to make it
> easier to enumerate the available disks.  It is in a
> (somewhat) strange format to try for a simple level of
> compatability with the old viodasd code (that was in a
> couple of vendor's kernels).

Exporting redundant information from procfs is a step backwards, since 
we have sysfs.

I would prefer not to apply this.  Upstream is for 'getting it right', 
not for dragging every little vendor kernel hack along.

	Jeff

Re: [PATCH] PPC64 iSeries viodasd proc file

[Christoph Hellwig]

On Fri, Jun 18, 2004 at 11:09:40AM -0400, Jeff Garzik wrote:
> Stephen Rothwell wrote:
> >Hi Andrew,
> >
> >This patch adds a proc file for viodasd so to make it
> >easier to enumerate the available disks.  It is in a
> >(somewhat) strange format to try for a simple level of
> >compatability with the old viodasd code (that was in a
> >couple of vendor's kernels).
> 
> Exporting redundant information from procfs is a step backwards, since 
> we have sysfs.
> 
> I would prefer not to apply this.  Upstream is for 'getting it right', 
> not for dragging every little vendor kernel hack along.

Agreed.  And the old viodasd reason was rejected exactly because it was
such a f***ing mess.

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeremy]

On Fri, 18 Jun 2004 16:17:53 +0100, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Jun 18, 2004 at 11:09:40AM -0400, Jeff Garzik wrote:
> > Stephen Rothwell wrote:
> > >This patch adds a proc file for viodasd so to make it
> > >easier to enumerate the available disks.  It is in a
> > >(somewhat) strange format to try for a simple level of
> > >compatability with the old viodasd code (that was in a
> > >couple of vendor's kernels).
> > 
> > Exporting redundant information from procfs is a step backwards, since
> > we have sysfs.
> >
> > I would prefer not to apply this.  Upstream is for 'getting it right',
> > not for dragging every little vendor kernel hack along.

It was in the tree for the platform, not just vendor trees.  ie,
anyone who wanted to use the platform with Linux would have had this
functionality.  If you'd argue that people shouldn't do that, then how
are platforms supposed to get to a point where they can be included in
the mainline tree?

Also, it's exactly the sort of thing that would have been accepted in
2.4 if the platform had tried to get included there.  So this is a bit
of bogus reasoning.  eg, if there was an attempt to include iSeries in
the 2.4 series now (or a year ago, when it might have been more
reasonable), this would have gone in.  It's an interface that users of
the platform have come to depend on, for better or for worse.

And the information is hardly redundant when the same information
isn't really available in /sys at present.  And before it's mentioned,
/sys/block isn't the same information.

> Agreed.  And the old viodasd reason was rejected exactly because it was
> such a f***ing mess.

The argument could be made that sysfs is similarly a f***ing mess and
that instead of solving problems, it creates more.  The mess of
symlinks present there is a disaster and disgusting for anyone who
wants to actually write clean probing code.    Also, things in sysfs
aren't exactly stable enough to count on as a dependable interface,
but that's something the kernel has never reliably exported to
userspace.

Jeremy

Re: [PATCH] PPC64 iSeries viodasd proc file

[Christoph Hellwig]

On Sun, Jun 20, 2004 at 03:52:33PM -0400, Jeremy wrote:
> It was in the tree for the platform, not just vendor trees.  ie,
> anyone who wanted to use the platform with Linux would have had this
> functionality.  If you'd argue that people shouldn't do that, then how
> are platforms supposed to get to a point where they can be included in
> the mainline tree?

Doesn't matter.  There was a really crappy 2.4 driver that IBM didn't
even bother to submit.  So for us it simply doesn't matter it existed.

Stop whining and ensure you're employer doesn't apply broken IBM patches
instead.

Re: [PATCH] PPC64 iSeries viodasd proc file

[Greg KH]

On Sun, Jun 20, 2004 at 03:52:33PM -0400, Jeremy wrote:
> > Agreed.  And the old viodasd reason was rejected exactly because it was
> > such a f***ing mess.
> 
> The argument could be made that sysfs is similarly a f***ing mess and
> that instead of solving problems, it creates more.

It does?  Have you brought this up to the sysfs / kobject / driver model
authors?  I think they would be open to any critiques of the current
code, especially if such critique contains patches.

> The mess of symlinks present there is a disaster and disgusting for
> anyone who wants to actually write clean probing code.

What do you mean by this.  Any examples?

> Also, things in sysfs aren't exactly stable enough to count on as a
> dependable interface, but that's something the kernel has never
> reliably exported to userspace.

Why isn't sysfs stable enough?  You can find any driver instantly.  And
any device bound to that driver in a stable and repeatable manner.

So, give me specific examples, or stop ranting for no reason.

greg k-h

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeremy Katz]

Oops, I can't use gmail and sent this just to Greg originally...  Greg
-- I've added a little bit more from my original mail

On Sun, 20 Jun 2004 23:04:35 -0700, Greg KH <greg@kroah.com> wrote:
> On Sun, Jun 20, 2004 at 03:52:33PM -0400, Jeremy wrote:
> > > Agreed.  And the old viodasd reason was rejected exactly because it was
> > > such a f***ing mess.
> >
> > The argument could be made that sysfs is similarly a f***ing mess and
> > that instead of solving problems, it creates more.
>
> It does?  Have you brought this up to the sysfs / kobject / driver model
> authors?  I think they would be open to any critiques of the current
> code, especially if such critique contains patches.

My argument isn't around the code... I haven't looked at it enough to
say one way or another.  Although it does seem to duplicate plenty of
things that were available before and then drivers start to drop
random bits that other people depended on being in /proc with the
"well, it should be in sysfs now" argument.  Unfortunately, it wasn't
a clean break and instead it's a very meandering migration.  Which
makes things a bit more difficult to deal with.

> > The mess of symlinks present there is a disaster and disgusting for
> > anyone who wants to actually write clean probing code.
>
> What do you mean by this.  Any examples?

For example, /sys/bus/ide/devices/* and then symlinks forever... lots
of readdir, readlink, etc makes probing far slower and more complex
than the simple /proc/ide/ide?/*/ that could be used before.

> > Also, things in sysfs aren't exactly stable enough to count on as a
> > dependable interface, but that's something the kernel has never
> > reliably exported to userspace.
>
> Why isn't sysfs stable enough?  You can find any driver instantly.  And
> any device bound to that driver in a stable and repeatable manner.

Again, not sysfs itself.  How information is exported via sysfs.  I'm
not saying that things exported via /proc are always the picture of
stability here (cf the recent change from /proc/scsi/usb-storage-$host
to /proc/scsi/usb-storage/$host), but at the same time, things in
/proc have tended to settle down in the general case.  This just isn't
true yet with sysfs and is only the sort of thing that can happen with
time.

There are also other things; I guess consistency is a better word. 
People like to say use /sys/block to show block devices, but that
shows a lot of "useless" block devices from the point of view of
trying to show disks.  Block devices also ends up including things
like loop devs (which are all always there), ramdisks, nbd and
probably other things I'm not thinking of.  There's also some
variation in what drivers show -- eg, iseries!vda vs carmel_0 for
device names...  the directory separator used being different points
at how there's a bit of ad-hocness to it.

> So, give me specific examples, or stop ranting for no reason.

And to be more constructive (after a discussion with Jeff this
afternoon which is when I realized the reply didn't go out), what
would be _very_ useful to have from a "probing disks" perspective
would be a way to enumerate easily and simply from within sysfs the
disks that are associated with a specific controller.  Not entirely
sure where under sysfs this would go, but to be able to easily see
that for block device type foo, I have disks disk0, disk1 and disk2. 
The vio sysfs stuff actually works kind of nicely like this, but it
would be more useful as a generic thing rather than not being able to
depend on it.

Note: this should not mean that we then go and remove currently
existing stuff in /proc.  Deprecate it and then it can go away in time
as people switch.  Having to have a flag day is very painful.  It's
far easier to deprecate in one stable series with a new interface
available and then start removing the old ones as things start to
switch over.  If it really is an improvement, then getting people to
change won't be difficult.

Jeremy

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeff Garzik]

Jeremy Katz wrote:
> And to be more constructive (after a discussion with Jeff this
> afternoon which is when I realized the reply didn't go out), what
> would be _very_ useful to have from a "probing disks" perspective
> would be a way to enumerate easily and simply from within sysfs the
> disks that are associated with a specific controller.  Not entirely
> sure where under sysfs this would go, but to be able to easily see
> that for block device type foo, I have disks disk0, disk1 and disk2. 
> The vio sysfs stuff actually works kind of nicely like this, but it
> would be more useful as a generic thing rather than not being able to
> depend on it.

And here's what I said to Jeremy over IRC as well...

I think it's perfectly reasonable to want a "driver -> registered 
devices" mapping.  That's what the installer appears to want, if I 
understand Jeremy correctly.

That will probably take some thought by the sysfs wizards, though, since 
drivers are registered on a per-bus basis...

	Jeff

Re: [PATCH] PPC64 iSeries viodasd proc file

[Greg KH]

On Wed, Jun 23, 2004 at 05:15:54PM -0400, Jeremy Katz wrote:
> For example, /sys/bus/ide/devices/* and then symlinks forever... lots
> of readdir, readlink, etc makes probing far slower and more complex
> than the simple /proc/ide/ide?/*/ that could be used before.

Yes, ide never got completly moved over to sysfs like scsi did.  We
never had the time to do this work, sorry.

But now your parsing should be easier with the one-value to one-file
rule, right?  And libsysfs should help out here with all of the symlinks
and readdir, etc calls.

> > > Also, things in sysfs aren't exactly stable enough to count on as a
> > > dependable interface, but that's something the kernel has never
> > > reliably exported to userspace.
> >
> > Why isn't sysfs stable enough?  You can find any driver instantly.  And
> > any device bound to that driver in a stable and repeatable manner.
> 
> Again, not sysfs itself.  How information is exported via sysfs.  I'm
> not saying that things exported via /proc are always the picture of
> stability here (cf the recent change from /proc/scsi/usb-storage-$host
> to /proc/scsi/usb-storage/$host), but at the same time, things in
> /proc have tended to settle down in the general case.  This just isn't
> true yet with sysfs and is only the sort of thing that can happen with
> time.
> 
> There are also other things; I guess consistency is a better word. 
> People like to say use /sys/block to show block devices, but that
> shows a lot of "useless" block devices from the point of view of
> trying to show disks.

But all of those devices are block devices.  You want a hardware
picture, right?  sysfs never said it would show you just that, but it
makes it easier to determine.

For this specific instance, just look for block devices that have a
device symlink that points to a real device.

> > So, give me specific examples, or stop ranting for no reason.
> 
> And to be more constructive (after a discussion with Jeff this
> afternoon which is when I realized the reply didn't go out), what
> would be _very_ useful to have from a "probing disks" perspective
> would be a way to enumerate easily and simply from within sysfs the
> disks that are associated with a specific controller.

Hm, I think libsysfs can give you this, if you ask for the block devices
that are associated with each individual device associated with a
driver.

The whole "what driver controls what devices" is not a simple one to one
mapping all the time, with drivers that work on multiple types of
busses, and drivers that control devices that contain multiple class
devices, etc.  It's not a simple thing to solve, sorry.

But what you can use is the MODULE_DEVICE_TABLE() information in the
modules to try to help you out here.  That details a mapping of what
kind of devices that specific driver supports.

> Note: this should not mean that we then go and remove currently
> existing stuff in /proc.  Deprecate it and then it can go away in time
> as people switch.  Having to have a flag day is very painful.  It's
> far easier to deprecate in one stable series with a new interface
> available and then start removing the old ones as things start to
> switch over.  If it really is an improvement, then getting people to
> change won't be difficult.

I agree, I don't think that many things have disappeared from /proc just
yet, right?  You should just have more information than what you
previously did, right?  Or did scsi drop their /proc support fully?

thanks,

greg k-h

Re: [PATCH] PPC64 iSeries viodasd proc file

[Stephen Rothwell]

From: Greg KH <greg@kroah.com>
> 
> I agree, I don't think that many things have disappeared from /proc just
> yet, right?  You should just have more information than what you
> previously did, right?  Or did scsi drop their /proc support fully?

What started this discussion is that I had to drop all the proc support
from the iSeries virtual devices while attempting to get the drivers
into the mainline kernel.

Cheers,
Stephen

Re: [PATCH] PPC64 iSeries viodasd proc file

[Greg KH]

On Thu, Jun 24, 2004 at 09:54:49AM +1000, Stephen Rothwell wrote:
> From: Greg KH <greg@kroah.com>
> > 
> > I agree, I don't think that many things have disappeared from /proc just
> > yet, right?  You should just have more information than what you
> > previously did, right?  Or did scsi drop their /proc support fully?
> 
> What started this discussion is that I had to drop all the proc support
> from the iSeries virtual devices while attempting to get the drivers
> into the mainline kernel.

Ok, but that support was never in mainline, right?  :)

Yeah I know, distro trees...

greg k-h

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeff Garzik]

Greg KH wrote:
> On Wed, Jun 23, 2004 at 05:15:54PM -0400, Jeremy Katz wrote:
> 
>>For example, /sys/bus/ide/devices/* and then symlinks forever... lots
>>of readdir, readlink, etc makes probing far slower and more complex
>>than the simple /proc/ide/ide?/*/ that could be used before.
> 
> 
> Yes, ide never got completly moved over to sysfs like scsi did.  We
> never had the time to do this work, sorry.
> 
> But now your parsing should be easier with the one-value to one-file
> rule, right?  And libsysfs should help out here with all of the symlinks
> and readdir, etc calls.
> 
> 
>>>>Also, things in sysfs aren't exactly stable enough to count on as a
>>>>dependable interface, but that's something the kernel has never
>>>>reliably exported to userspace.
>>>
>>>Why isn't sysfs stable enough?  You can find any driver instantly.  And
>>>any device bound to that driver in a stable and repeatable manner.
>>
>>Again, not sysfs itself.  How information is exported via sysfs.  I'm
>>not saying that things exported via /proc are always the picture of
>>stability here (cf the recent change from /proc/scsi/usb-storage-$host
>>to /proc/scsi/usb-storage/$host), but at the same time, things in
>>/proc have tended to settle down in the general case.  This just isn't
>>true yet with sysfs and is only the sort of thing that can happen with
>>time.
>>
>>There are also other things; I guess consistency is a better word. 
>>People like to say use /sys/block to show block devices, but that
>>shows a lot of "useless" block devices from the point of view of
>>trying to show disks.
> 
> 
> But all of those devices are block devices.  You want a hardware
> picture, right?  sysfs never said it would show you just that, but it
> makes it easier to determine.
> 
> For this specific instance, just look for block devices that have a
> device symlink that points to a real device.
> 
> 
>>>So, give me specific examples, or stop ranting for no reason.
>>
>>And to be more constructive (after a discussion with Jeff this
>>afternoon which is when I realized the reply didn't go out), what
>>would be _very_ useful to have from a "probing disks" perspective
>>would be a way to enumerate easily and simply from within sysfs the
>>disks that are associated with a specific controller.
> 
> 
> Hm, I think libsysfs can give you this, if you ask for the block devices
> that are associated with each individual device associated with a
> driver.
> 
> The whole "what driver controls what devices" is not a simple one to one
> mapping all the time, with drivers that work on multiple types of
> busses, and drivers that control devices that contain multiple class
> devices, etc.  It's not a simple thing to solve, sorry.

SET_BLKDEV_DRIVER(), SET_NETDEV_DRIVER(), ...

We need a struct driver just like we have a struct device.

Then binding registered interfaces, of any type, to the driver.


> But what you can use is the MODULE_DEVICE_TABLE() information in the
> modules to try to help you out here.  That details a mapping of what
> kind of devices that specific driver supports.

No, it details what devices a driver supports, not what _type_ of 
devices the driver supports.


>>Note: this should not mean that we then go and remove currently
>>existing stuff in /proc.  Deprecate it and then it can go away in time
>>as people switch.  Having to have a flag day is very painful.  It's
>>far easier to deprecate in one stable series with a new interface
>>available and then start removing the old ones as things start to
>>switch over.  If it really is an improvement, then getting people to
>>change won't be difficult.
> 
> 
> I agree, I don't think that many things have disappeared from /proc just
> yet, right?  You should just have more information than what you
> previously did, right?  Or did scsi drop their /proc support fully?

Concrete example:  modprobe sx8.  Now, what block devices did it detect?

	Jeff

Re: [PATCH] PPC64 iSeries viodasd proc file

[Greg KH]

On Wed, Jun 23, 2004 at 08:38:10PM -0400, Jeff Garzik wrote:
> >But what you can use is the MODULE_DEVICE_TABLE() information in the
> >modules to try to help you out here.  That details a mapping of what
> >kind of devices that specific driver supports.
> 
> No, it details what devices a driver supports, not what _type_ of 
> devices the driver supports.

Yes, you are correct. 

> >>Note: this should not mean that we then go and remove currently
> >>existing stuff in /proc.  Deprecate it and then it can go away in time
> >>as people switch.  Having to have a flag day is very painful.  It's
> >>far easier to deprecate in one stable series with a new interface
> >>available and then start removing the old ones as things start to
> >>switch over.  If it really is an improvement, then getting people to
> >>change won't be difficult.
> >
> >
> >I agree, I don't think that many things have disappeared from /proc just
> >yet, right?  You should just have more information than what you
> >previously did, right?  Or did scsi drop their /proc support fully?
> 
> Concrete example:  modprobe sx8.  Now, what block devices did it detect?

Could we determine this in 2.4?

Anyway, how about this assuming sx8 is a pci driver:
	- look in /sys/bus/pci/drivers/sx8/
	- for every device listed in that directory do:
		- `tree | grep block` or however you want to search the
		  tree for the block symlink, find is probably nicer
		  here.
		- that gives you the base block device, then go into the
		  /sys/block/FOUND_BLOCK_DEVICE to find the individual
		  partitions if needed.

Or work backwards if you want to:
	- tally up every /sys/block/*/device symlink, and see if they
	  point to a device owned by the sx8 driver.

Does that work for you?

thanks,

greg k-h

Re: [PATCH] PPC64 iSeries viodasd proc file

[Jeff Garzik]

On Thu, Jun 24, 2004 at 01:59:37PM -0700, Greg KH wrote:
> Could we determine this in 2.4?

Yes -- by each driver exporting its own procfs node, with its own
private format different from all the others :)


> Anyway, how about this assuming sx8 is a pci driver:
> 	- look in /sys/bus/pci/drivers/sx8/
> 	- for every device listed in that directory do:
> 		- `tree | grep block` or however you want to search the
> 		  tree for the block symlink, find is probably nicer
> 		  here.
> 		- that gives you the base block device, then go into the
> 		  /sys/block/FOUND_BLOCK_DEVICE to find the individual
> 		  partitions if needed.
> 
> Or work backwards if you want to:
> 	- tally up every /sys/block/*/device symlink, and see if they
> 	  point to a device owned by the sx8 driver.
> 
> Does that work for you?

Jeremy?

	Jeff