[PATCH] Don't open same disk twice on OpenFirmware.

[PATCH] Don't open same disk twice on OpenFirmware.

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 214 bytes --]

Hello, all. According to David Miller sparc's openboot doesn't support
opening same disk twice. So I implemented handle reusage logic. Tested
on imac g3

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: ofdisk.diff --]
[-- Type: text/x-diff; name="ofdisk.diff", Size: 4151 bytes --]

=== modified file 'disk/ieee1275/ofdisk.c'
--- disk/ieee1275/ofdisk.c	2009-12-07 10:54:25 +0000
+++ disk/ieee1275/ofdisk.c	2009-12-18 16:12:28 +0000
@@ -1,7 +1,7 @@
 /* ofdisk.c - Open Firmware disk access.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2004,2006,2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2004,2006,2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -26,6 +26,8 @@
 struct ofdisk_hash_ent
 {
   char *devpath;
+  int refs;
+  grub_ieee1275_ihandle_t dev_ihandle;
   struct ofdisk_hash_ent *next;
 };
 
@@ -65,6 +67,8 @@ ofdisk_hash_add (char *devpath)
     {
       p->devpath = devpath;
       p->next = *head;
+      p->refs = 0;
+      p->dev_ihandle = 0;
       *head = p;
     }
   return p;
@@ -170,6 +174,23 @@ grub_ofdisk_open (const char *name, grub
   if (!op)
     return grub_errno;
 
+  if (op->dev_ihandle)
+    {
+      op->refs++;
+
+      /* XXX: There is no property to read the number of blocks.  There
+	 should be a property `#blocks', but it is not there.  Perhaps it
+	 is possible to use seek for this.  */
+      disk->total_sectors = 0xFFFFFFFFUL;
+
+      disk->id = (unsigned long) op;
+
+      /* XXX: Read this, somehow.  */
+      disk->has_partitions = 1;
+      disk->data = op;
+      return 0;
+    }
+
   grub_dprintf ("disk", "Opening `%s'.\n", op->devpath);
 
   grub_ieee1275_open (op->devpath, &dev_ihandle);
@@ -179,8 +200,8 @@ grub_ofdisk_open (const char *name, grub
       goto fail;
     }
 
-  grub_dprintf ("disk", "Opened `%s' as handle %p.\n", op->devpath,
-		(void *) (unsigned long) dev_ihandle);
+  grub_dprintf ("disk", "Opened `%s' as handle 0x%lx.\n", op->devpath,
+		(unsigned long) dev_ihandle);
 
   if (grub_ieee1275_finddevice (op->devpath, &dev))
     {
@@ -201,6 +222,9 @@ grub_ofdisk_open (const char *name, grub
       goto fail;
     }
 
+  op->dev_ihandle = dev_ihandle;
+  op->refs++;
+
   /* XXX: There is no property to read the number of blocks.  There
      should be a property `#blocks', but it is not there.  Perhaps it
      is possible to use seek for this.  */
@@ -210,7 +234,7 @@ grub_ofdisk_open (const char *name, grub
 
   /* XXX: Read this, somehow.  */
   disk->has_partitions = 1;
-  disk->data = (void *) (unsigned long) dev_ihandle;
+  disk->data = op;
   return 0;
 
  fail:
@@ -222,9 +246,15 @@ grub_ofdisk_open (const char *name, grub
 static void
 grub_ofdisk_close (grub_disk_t disk)
 {
-  grub_dprintf ("disk", "Closing handle %p.\n",
-		(void *) disk->data);
-  grub_ieee1275_close ((grub_ieee1275_ihandle_t) (unsigned long) disk->data);
+  struct ofdisk_hash_ent *data = disk->data;
+
+  data->refs--;
+  if (data->refs)
+    return;
+
+  grub_dprintf ("disk", "Closing handle %p.\n", data);
+  grub_ieee1275_close (data->dev_ihandle);
+  data->dev_ihandle = 0;
 }
 
 static grub_err_t
@@ -233,21 +263,21 @@ grub_ofdisk_read (grub_disk_t disk, grub
 {
   grub_ssize_t status, actual;
   unsigned long long pos;
+  struct ofdisk_hash_ent *data = disk->data;
 
   grub_dprintf ("disk",
 		"Reading handle %p: sector 0x%llx, size 0x%lx, buf %p.\n",
-		(void *) disk->data, (long long) sector, (long) size, buf);
+		data, (long long) sector, (long) size, buf);
 
   pos = sector * 512UL;
 
-  grub_ieee1275_seek ((grub_ieee1275_ihandle_t) (unsigned long) disk->data,
+  grub_ieee1275_seek (data->dev_ihandle,
 		      (int) (pos >> 32), (int) pos & 0xFFFFFFFFUL, &status);
   if (status < 0)
     return grub_error (GRUB_ERR_READ_ERROR,
 		       "Seek error, can't seek block %llu",
 		       (long long) sector);
-  grub_ieee1275_read ((grub_ieee1275_ihandle_t) (unsigned long) disk->data,
-		      buf, size * 512UL, &actual);
+  grub_ieee1275_read (data->dev_ihandle, buf, size * 512UL, &actual);
   if (actual != actual)
     return grub_error (GRUB_ERR_READ_ERROR, "Read error on block: %llu",
 		       (long long) sector);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: [PATCH] Don't open same disk twice on OpenFirmware.

[David Miller]

From: Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>
Date: Fri, 18 Dec 2009 17:17:13 +0100

> Hello, all. According to David Miller sparc's openboot doesn't support
> opening same disk twice. So I implemented handle reusage logic. Tested
> on imac g3

At a minimum you have to seek to the beginning of the device
as that is one of the many side effects of openning the
device.

I really don't think this is a good idea, really.

We sould just enforce the restriction, and put a debugging
check there to make sure the invariant is never violated.

Re: [PATCH] Don't open same disk twice on OpenFirmware.

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 18 Dec 2009 10:33:13 -0800 (PST)

> From: Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>
> Date: Fri, 18 Dec 2009 17:17:13 +0100
> 
>> Hello, all. According to David Miller sparc's openboot doesn't support
>> opening same disk twice. So I implemented handle reusage logic. Tested
>> on imac g3
> 
> At a minimum you have to seek to the beginning of the device
> as that is one of the many side effects of openning the
> device.

In fact this proves that your idea can't ever work.
Each openned instance will expect the file offset
pointer to be at different locations.

You can't share open instances, therefore.

We really just have to make sure GRUB never violates
this restriction.

Re: [PATCH] Don't open same disk twice on OpenFirmware.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Fri, 18 Dec 2009 10:33:13 -0800 (PST)
>
>   
>> From: Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com>
>> Date: Fri, 18 Dec 2009 17:17:13 +0100
>>
>>     
>>> Hello, all. According to David Miller sparc's openboot doesn't support
>>> opening same disk twice. So I implemented handle reusage logic. Tested
>>> on imac g3
>>>       
>> At a minimum you have to seek to the beginning of the device
>> as that is one of the many side effects of openning the
>> device.
>>     
>
> In fact this proves that your idea can't ever work.
> Each openned instance will expect the file offset
> pointer to be at different locations.
>
> You can't share open instances, therefore.
>
> We really just have to make sure GRUB never violates
> this restriction.
>   
Actually we don't open files, only disks and we explicitely seek to
desired position. It was so even before my patch.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]