[ppc patch] support SCSI disks

[ppc patch] support SCSI disks

[Hollis Blanchard]

This patch allows grub2 to find SCSI disks. Please apply.

2004-10-01	Hollis Blanchard	<hollis@penguinppc.org>

	* disk/powerpc/ieee1275/ofdisk.c (grub_ofdisk_iterate):
	call grub_children_iterate for device nodes of type `scsi'.
	(grub_ofdisk_open): remove manual device alias resolution.
	Fix memory leak when device cannot be opened.
	Fix error checking after grub_ieee1275_get_property.
	* include/grub/powerpc/ieee1275/ieee1275.h 
	(grub_children_iterate): new prototype.
	* kern/powerpc/ieee1275/openfw.c (grub_children_iterate):
	new function.

Index: disk/powerpc/ieee1275/ofdisk.c
===================================================================
RCS file: /cvsroot/grub/grub2/disk/powerpc/ieee1275/ofdisk.c,v
retrieving revision 1.3
diff -u -r1.3 ofdisk.c
--- disk/powerpc/ieee1275/ofdisk.c	21 Aug 2004 13:54:22 -0000	1.3
+++ disk/powerpc/ieee1275/ofdisk.c	2 Oct 2004 01:49:35 -0000
@@ -30,6 +30,9 @@
     {
       if (! grub_strcmp (alias->type, "block"))
 	hook (alias->name);
+      else if (! grub_strcmp (alias->type, "scsi"))
+	/* SCSI nodes are not type "block", but have block-type children.  */
+	grub_children_iterate (alias->name, dev_iterate);
       return 0;
     }
 
@@ -49,22 +52,18 @@
   grub_size_t pathlen;
   int actual;
 
-  if (grub_ieee1275_finddevice ("/aliases", &devalias))
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't read the aliases");
-  
-  grub_ieee1275_get_property_length (devalias, name, &pathlen);
-  devpath = grub_malloc (pathlen);
+  devpath = grub_strndup (name, grub_strlen (devpath) + 2);
   if (! devpath)
     return grub_errno;
 
-  if (grub_ieee1275_get_property (devalias, name, devpath, pathlen, &actual))
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device alias");
-
   /* To access the complete disk add `:0'.  */
   grub_strcat (devpath, ":0");
   grub_ieee1275_open (devpath, &dev_ihandle);
   if (! dev_ihandle)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
+      goto fail;
+    }
   
   if (grub_ieee1275_finddevice (devpath, &dev))
     {
@@ -72,8 +71,9 @@
       goto fail;
     }
 
-  if (grub_ieee1275_get_property (dev, "device_type", prop, sizeof (prop),
-				  &actual))
+  grub_ieee1275_get_property (dev, "device_type", prop, sizeof (prop),
+				  &actual);
+  if (actual == -1)
     {
       grub_error (GRUB_ERR_BAD_DEVICE, "Can't read the device type");
       goto fail;
@@ -99,7 +99,7 @@
   disk->data = (void *) dev_ihandle;
 
  fail:
-  if (grub_errno)
+  if (grub_errno && dev_ihandle)
     grub_ieee1275_close (dev_ihandle);
   grub_free (devpath);
   return grub_errno;
Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.4
diff -u -r1.4 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	14 Sep 2004 08:56:51 -0000	1.4
+++ include/grub/powerpc/ieee1275/ieee1275.h	2 Oct 2004 01:49:35 -0000
@@ -96,6 +96,8 @@
 
 grub_err_t EXPORT_FUNC(grub_devalias_iterate)
      (int (*hook) (struct grub_ieee1275_devalias *alias));
+grub_err_t EXPORT_FUNC(grub_children_iterate) (char *devpath,
+     int (*hook) (struct grub_ieee1275_devalias *alias));
 
 
 #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.2
diff -u -r1.2 openfw.c
--- kern/powerpc/ieee1275/openfw.c	4 Apr 2004 13:46:02 -0000	1.2
+++ kern/powerpc/ieee1275/openfw.c	2 Oct 2004 01:49:35 -0000
@@ -23,6 +23,59 @@
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
 
+/* Walk children of 'devpath', calling hook for each.  */
+grub_err_t
+grub_children_iterate (char *devpath,
+		  int (*hook) (struct grub_ieee1275_devalias *alias))
+{
+  grub_ieee1275_phandle_t dev;
+  grub_ieee1275_phandle_t child;
+
+  grub_ieee1275_finddevice (devpath, &dev);
+  if (dev == -1)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
+
+  grub_ieee1275_child (dev, &child);
+  if (child == -1)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "Device has no children");
+
+  do
+    {
+      /* XXX: Don't use hardcoded path lengths.  */
+      char childtype[64];
+      char childpath[64];
+      char childname[64];
+      char fullname[64];
+      struct grub_ieee1275_devalias alias;
+      int actual;
+
+      grub_ieee1275_get_property (child, "device_type", &childtype,
+				  sizeof childtype, &actual);
+      if (actual == -1)
+	continue;
+
+      grub_ieee1275_package_to_path (child, childpath, sizeof childpath,
+      				     &actual);
+      if (actual == -1)
+	continue;
+
+      grub_ieee1275_get_property (child, "name", &childname,
+				  sizeof childname, &actual);
+      if (actual == -1)
+	continue;
+
+      grub_sprintf(fullname, "%s/%s", devpath, childname);
+
+      alias.type = childtype;
+      alias.path = childpath;
+      alias.name = fullname;
+      hook (&alias);
+    }
+  while (grub_ieee1275_peer (child, &child));
+
+  return 0;
+}
+
 /* Iterate through all device aliasses.  Thisfunction can be used to
    find a device of a specific type.  */
 grub_err_t

Re: [ppc patch] support SCSI disks

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> This patch allows grub2 to find SCSI disks. Please apply.
>
> 2004-10-01	Hollis Blanchard	<hollis@penguinppc.org>
>
> 	* disk/powerpc/ieee1275/ofdisk.c (grub_ofdisk_iterate):
> 	call grub_children_iterate for device nodes of type `scsi'.
> 	(grub_ofdisk_open): remove manual device alias resolution.
> 	Fix memory leak when device cannot be opened.
> 	Fix error checking after grub_ieee1275_get_property.
> 	* include/grub/powerpc/ieee1275/ieee1275.h 
> 	(grub_children_iterate): new prototype.
> 	* kern/powerpc/ieee1275/openfw.c (grub_children_iterate):
> 	new function.


Isn't it possible to make this a bit more generic?  So not iterating
over only scsi but also over `ide' when it is available, for example?
That way it is more likely the code will work on other machines as
well.

Or is this idea just silly?

Thanks,
Marco

Re: [ppc patch] support SCSI disks

[Hollis Blanchard]

On Oct 3, 2004, at 4:21 AM, Marco Gerards wrote:
>
> Isn't it possible to make this a bit more generic?  So not iterating
> over only scsi but also over `ide' when it is available, for example?
> That way it is more likely the code will work on other machines as
> well.

A given SCSI device could be either a byte-oriented tape or a 
block-oriented disk. However, I do not believe that the ATA protocol 
allows you to read a single byte, so ATA tape drives must implement 
blocking in the hardware. In other words, it is not possible to have 
both block- and byte-oriented device nodes representing an ATA device. 
That is probably why I have never seen an IDE/ATA "tape" node in a 
device tree.

If a system does turn up later in which some additional searching is 
needed, the problem can be resolved then. For now, my preference is not 
to write and maintain code that isn't needed, as unneeded code can only 
cause bugs.

-Hollis

Re: [ppc patch] support SCSI disks

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> On Oct 3, 2004, at 4:21 AM, Marco Gerards wrote:
>>
>> Isn't it possible to make this a bit more generic?  So not iterating
>> over only scsi but also over `ide' when it is available, for example?
>> That way it is more likely the code will work on other machines as
>> well.
>
> A given SCSI device could be either a byte-oriented tape or a
> block-oriented disk. However, I do not believe that the ATA protocol
> allows you to read a single byte, so ATA tape drives must implement
> blocking in the hardware. In other words, it is not possible to have
> both block- and byte-oriented device nodes representing an ATA
> device. That is probably why I have never seen an IDE/ATA "tape" node
> in a device tree.

I never mentioned tapes...  All I currently care about are block
devices.

> If a system does turn up later in which some additional searching is
> needed, the problem can be resolved then. For now, my preference is
> not to write and maintain code that isn't needed, as unneeded code can
> only cause bugs.

What I mean is not writing unneeded code, just making it a bit more
generic.  For example scan from all the childs of aliases for block
devices.  In your case scsi/foo will show up.

Thanks,
Marco

Re: [ppc patch] support SCSI disks

[Hollis Blanchard]

On Oct 3, 2004, at 1:10 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> On Oct 3, 2004, at 4:21 AM, Marco Gerards wrote:
>>>
>>> Isn't it possible to make this a bit more generic?  So not iterating
>>> over only scsi but also over `ide' when it is available, for example?
>>> That way it is more likely the code will work on other machines as
>>> well.
>>
>> A given SCSI device could be either a byte-oriented tape or a
>> block-oriented disk. However, I do not believe that the ATA protocol
>> allows you to read a single byte, so ATA tape drives must implement
>> blocking in the hardware. In other words, it is not possible to have
>> both block- and byte-oriented device nodes representing an ATA
>> device. That is probably why I have never seen an IDE/ATA "tape" node
>> in a device tree.
>
> I never mentioned tapes...  All I currently care about are block
> devices.

Ok. I was submitting this code to solve a specific problem: on Old 
World Macintosh, I don't have a "disk" or "hd" devalias. Instead I have 
a "scsi" alias, which has the disk and tape nodes below it. The disk 
node is type block; the tape node is type char.

>> If a system does turn up later in which some additional searching is
>> needed, the problem can be resolved then. For now, my preference is
>> not to write and maintain code that isn't needed, as unneeded code can
>> only cause bugs.
>
> What I mean is not writing unneeded code, just making it a bit more
> generic.  For example scan from all the childs of aliases for block
> devices.  In your case scsi/foo will show up.

Keep in mind that on my New World Mac, I have both an "ide1" and an 
"hd" devalias. That will mean that GRUB will find and list both "hd" 
and "ide1/disk" as separate devices, even though they are just 
different names for the same thing. If you're happy with that then it's 
ok with me. Just make sure to check for both device_type == "ide" and 
"ata"; my New World says "ata" but my briQ says "ide".

-Hollis

Re: [ppc patch] support SCSI disks

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

>>> If a system does turn up later in which some additional searching is
>>> needed, the problem can be resolved then. For now, my preference is
>>> not to write and maintain code that isn't needed, as unneeded code can
>>> only cause bugs.
>>
>> What I mean is not writing unneeded code, just making it a bit more
>> generic.  For example scan from all the childs of aliases for block
>> devices.  In your case scsi/foo will show up.
>
> Keep in mind that on my New World Mac, I have both an "ide1" and an
> "hd" devalias. That will mean that GRUB will find and list both "hd"
> and "ide1/disk" as separate devices, even though they are just
> different names for the same thing. If you're happy with that then
> it's ok with me. Just make sure to check for both device_type == "ide"
> and "ata"; my New World says "ata" but my briQ says "ide".

It is not a problem to be able to access one device in multiple ways.
At least not for me.  It is more important to me to find all block
devices dynamically.

Thanks,
Marco

Re: [ppc patch] support SCSI disks

[Hollis Blanchard]

I've tested this patch on a B&W G3, and as expected it listed both "hd" 
and "ide1/disk" as present disks. Please apply.

2004-10-01	Hollis Blanchard	<hollis@penguinppc.org>

	* disk/powerpc/ieee1275/ofdisk.c (grub_ofdisk_iterate):
	call grub_children_iterate for device nodes of type `scsi',
	`ide', or `ata'.
	(grub_ofdisk_open): remove manual device alias resolution.
	Fix memory leak when device cannot be opened.
	Fix error checking after grub_ieee1275_get_property.
	* include/grub/powerpc/ieee1275/ieee1275.h 
	(grub_children_iterate): new prototype.
	* kern/powerpc/ieee1275/openfw.c (grub_children_iterate):
	new function.

Index: disk/powerpc/ieee1275/ofdisk.c
===================================================================
RCS file: /cvsroot/grub/grub2/disk/powerpc/ieee1275/ofdisk.c,v
retrieving revision 1.3
diff -u -r1.3 ofdisk.c
--- disk/powerpc/ieee1275/ofdisk.c	21 Aug 2004 13:54:22 -0000	1.3
+++ disk/powerpc/ieee1275/ofdisk.c	3 Oct 2004 22:54:59 -0000
@@ -30,6 +30,11 @@
     {
       if (! grub_strcmp (alias->type, "block"))
 	hook (alias->name);
+      else if ((! grub_strcmp (alias->type, "scsi"))
+	       || (! grub_strcmp (alias->type, "ide"))
+	       || (! grub_strcmp (alias->type, "ata")))
+	/* Search for block-type children of these bus controllers.  */
+	grub_children_iterate (alias->name, dev_iterate);
       return 0;
     }
 
@@ -40,31 +45,25 @@
 static grub_err_t
 grub_ofdisk_open (const char *name, grub_disk_t disk)
 {
-  grub_ieee1275_phandle_t devalias;
   grub_ieee1275_phandle_t dev;
   grub_ieee1275_ihandle_t dev_ihandle = 0;
   char *devpath = 0;
   /* XXX: This should be large enough for any possible case.  */
   char prop[64];
-  grub_size_t pathlen;
   int actual;
 
-  if (grub_ieee1275_finddevice ("/aliases", &devalias))
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't read the aliases");
-  
-  grub_ieee1275_get_property_length (devalias, name, &pathlen);
-  devpath = grub_malloc (pathlen);
+  devpath = grub_strndup (name, grub_strlen (devpath) + 2);
   if (! devpath)
     return grub_errno;
 
-  if (grub_ieee1275_get_property (devalias, name, devpath, pathlen, &actual))
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device alias");
-
   /* To access the complete disk add `:0'.  */
   grub_strcat (devpath, ":0");
   grub_ieee1275_open (devpath, &dev_ihandle);
   if (! dev_ihandle)
-    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
+    {
+      grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Can't open device");
+      goto fail;
+    }
   
   if (grub_ieee1275_finddevice (devpath, &dev))
     {
@@ -72,8 +71,9 @@
       goto fail;
     }
 
-  if (grub_ieee1275_get_property (dev, "device_type", prop, sizeof (prop),
-				  &actual))
+  grub_ieee1275_get_property (dev, "device_type", prop, sizeof (prop),
+				  &actual);
+  if (actual == -1)
     {
       grub_error (GRUB_ERR_BAD_DEVICE, "Can't read the device type");
       goto fail;
@@ -99,7 +99,7 @@
   disk->data = (void *) dev_ihandle;
 
  fail:
-  if (grub_errno)
+  if (grub_errno && dev_ihandle)
     grub_ieee1275_close (dev_ihandle);
   grub_free (devpath);
   return grub_errno;
Index: include/grub/powerpc/ieee1275/ieee1275.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/ieee1275.h,v
retrieving revision 1.5
diff -u -r1.5 ieee1275.h
--- include/grub/powerpc/ieee1275/ieee1275.h	3 Oct 2004 09:19:10 -0000	1.5
+++ include/grub/powerpc/ieee1275/ieee1275.h	3 Oct 2004 22:54:59 -0000
@@ -96,6 +96,8 @@
 
 grub_err_t EXPORT_FUNC(grub_devalias_iterate)
      (int (*hook) (struct grub_ieee1275_devalias *alias));
+grub_err_t EXPORT_FUNC(grub_children_iterate) (char *devpath,
+     int (*hook) (struct grub_ieee1275_devalias *alias));
 
 
 #endif /* ! GRUB_IEEE1275_MACHINE_HEADER */
Index: kern/powerpc/ieee1275/openfw.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/powerpc/ieee1275/openfw.c,v
retrieving revision 1.2
diff -u -r1.2 openfw.c
--- kern/powerpc/ieee1275/openfw.c	4 Apr 2004 13:46:02 -0000	1.2
+++ kern/powerpc/ieee1275/openfw.c	3 Oct 2004 22:54:59 -0000
@@ -23,6 +23,59 @@
 #include <grub/mm.h>
 #include <grub/machine/ieee1275.h>
 
+/* Walk children of 'devpath', calling hook for each.  */
+grub_err_t
+grub_children_iterate (char *devpath,
+		  int (*hook) (struct grub_ieee1275_devalias *alias))
+{
+  grub_ieee1275_phandle_t dev;
+  grub_ieee1275_phandle_t child;
+
+  grub_ieee1275_finddevice (devpath, &dev);
+  if (dev == -1)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
+
+  grub_ieee1275_child (dev, &child);
+  if (child == -1)
+    return grub_error (GRUB_ERR_BAD_DEVICE, "Device has no children");
+
+  do
+    {
+      /* XXX: Don't use hardcoded path lengths.  */
+      char childtype[64];
+      char childpath[64];
+      char childname[64];
+      char fullname[64];
+      struct grub_ieee1275_devalias alias;
+      int actual;
+
+      grub_ieee1275_get_property (child, "device_type", &childtype,
+				  sizeof childtype, &actual);
+      if (actual == -1)
+	continue;
+
+      grub_ieee1275_package_to_path (child, childpath, sizeof childpath,
+      				     &actual);
+      if (actual == -1)
+	continue;
+
+      grub_ieee1275_get_property (child, "name", &childname,
+				  sizeof childname, &actual);
+      if (actual == -1)
+	continue;
+
+      grub_sprintf(fullname, "%s/%s", devpath, childname);
+
+      alias.type = childtype;
+      alias.path = childpath;
+      alias.name = fullname;
+      hook (&alias);
+    }
+  while (grub_ieee1275_peer (child, &child));
+
+  return 0;
+}
+
 /* Iterate through all device aliasses.  Thisfunction can be used to
    find a device of a specific type.  */
 grub_err_t

Re: [ppc patch] support SCSI disks

[Stefan Reinauer]

* Hollis Blanchard <hollis@penguinppc.org> [041003 19:48]:
> A given SCSI device could be either a byte-oriented tape or a 
> block-oriented disk. However, I do not believe that the ATA protocol 
> allows you to read a single byte, so ATA tape drives must implement 
> blocking in the hardware. In other words, it is not possible to have 
> both block- and byte-oriented device nodes representing an ATA device. 
> That is probably why I have never seen an IDE/ATA "tape" node in a 
> device tree.
 
It can be used as a stream through the "deblocker" though.

Stefan

Re: [ppc patch] support SCSI disks

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

Some gcs things you can hopefully do next time.  I'll fix these before
applying the patch.

As far as I am concerned this patch can be committed.  So I will do
that when I have the time for that.

> 2004-10-01	Hollis Blanchard	<hollis@penguinppc.org>

Use only two spaces before and after your name.

>
> 	* disk/powerpc/ieee1275/ofdisk.c (grub_ofdisk_iterate):
> 	call grub_children_iterate for device nodes of type `scsi',
> 	`ide', or `ata'.

Please begin a sentence with a capital.

> 	Fix error checking after grub_ieee1275_get_property.

Better describe what was changed.

> +      else if ((! grub_strcmp (alias->type, "scsi"))
> +	       || (! grub_strcmp (alias->type, "ide"))
> +	       || (! grub_strcmp (alias->type, "ata")))

What I meant with my previous email was finding these aliases that
function as controller dynamically if possible.  But this is ok for
now.

> +      /* XXX: Don't use hardcoded path lengths.  */
> +      char childtype[64];
> +      char childpath[64];
> +      char childname[64];
> +      char fullname[64];

I assume you tested this length is sufficient.

Thanks,
Marco

Re: [ppc patch] support SCSI disks

[Hollis Blanchard]

On Oct 4, 2004, at 6:21 AM, Marco Gerards wrote:
>
> As far as I am concerned this patch can be committed.  So I will do
> that when I have the time for that.

Great!

>> +      else if ((! grub_strcmp (alias->type, "scsi"))
>> +	       || (! grub_strcmp (alias->type, "ide"))
>> +	       || (! grub_strcmp (alias->type, "ata")))
>
> What I meant with my previous email was finding these aliases that
> function as controller dynamically if possible.  But this is ok for
> now.

To handle something like USB devices, we would need to fully recurse 
below each node. The USB bus topology is represented in device tree 
nodes, so a USB block device could appear N levels deep below a "usb" 
alias. Firewire may be the same way. While I do have a USB flash 
device, my B&W G3's firmware doesn't recognize it as a block device, so 
I can't really test. However, this code supports SCSI for now, so I 
agree we can address the more general problem later.

>> +      /* XXX: Don't use hardcoded path lengths.  */
>> +      char childtype[64];
>> +      char childpath[64];
>> +      char childname[64];
>> +      char fullname[64];
>
> I assume you tested this length is sufficient.

"Works for me", but as we discussed once I would prefer a get_property 
function that returns malloc'ed data. There may indeed be systems whose 
bus topology is so deep that we exhaust these buffers.

-Hollis