Re: [PATCH V3] ieee1275: obdisk driver

[Eric Snowberg]

> On Mar 6, 2019, at 4:32 AM, Daniel Kiper <address@hidden> wrote:
> 
> On Mon, Mar 04, 2019 at 05:27:39PM -0800, Eric Snowberg wrote:
>> Add a new disk driver called obdisk for IEEE1275 platforms.  Currently
>> the only platform using this disk driver is SPARC, however other IEEE1275
>> platforms could start using it if they so choose.  While the functionality
>> within the current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
>> presented too many problems on SPARC hardware.
>> 
>> Within the old ofdisk, there is not a way to determine the true canonical
>> name for the disk.  Within Open Boot, the same disk can have multiple names
>> but all reference the same disk.  For example the same disk can be referenced
>> by its SAS WWN, using this form:
>> 
>> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden,0
>> 
>> It can also be referenced by its PHY identifier using this form:
>> 
>> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden
>> 
>> It can also be referenced by its Target identifier using this form:
>> 
>> /address@hidden/address@hidden/address@hidden/address@hidden/LSI,address@hidden/address@hidden
>> 
>> Also, when the LUN=0, it is legal to omit the ,0 from the device name.  So 
>> with
>> the disk above, before taking into account the device aliases, there are 6 
>> ways
>> to reference the same disk.
>> 
>> Then it is possible to have 0 .. n device aliases all representing the same 
>> disk.
>> Within this new driver the true canonical name is determined using the the
>> IEEE1275 encode-unit and decode-unit commands when address_cells == 4.  This
>> will determine the true single canonical name for the device so multiple 
>> ihandles
>> are not opened for the same device.  This is what frequently happens with 
>> the old
>> ofdisk driver.  With some devices when they are opened multiple times it 
>> causes
>> the entire system to hang.
>> 
>> Another problem solved with this driver is devices that do not have a device
>> alias can be booted and used within GRUB. Within the old ofdisk, this was not
>> possible, unless it was the original boot device.  All devices behind a SAS
>> or SCSI parent can be found.   Within the old ofdisk, finding these disks
>> relied on there being an alias defined.  The alias requirement is not
>> necessary with this new driver.  It can also find devices behind a parent
>> after they have been hot-plugged.  This is something that is not possible
>> with the old ofdisk driver.
>> 
>> The old ofdisk driver also incorrectly assumes that the device pointing to 
>> by a
>> device alias is in its true canonical form. This assumption is never made 
>> with
>> this new driver.
>> 
>> Another issue solved with this driver is that it properly caches the ihandle
>> for all open devices.  The old ofdisk tries to do this by caching the last
>> opened ihandle.  However this does not work properly because the layer above
>> does not use a consistent device name for the same disk when calling into the
>> driver.  This is because the upper layer uses the bootpath value returned 
>> within
>> /chosen, other times it uses the device alias, and other times it uses the
>> value within grub.cfg.  It does not have a way to figure out that these 
>> devices
>> are the same disk.  This is not a problem with this new driver.
>> 
>> Due to the way GRUB repeatedly opens and closes the same disk. Caching the
>> ihandle is important on SPARC.  Without caching, some SAS devices can take
>> 15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
>> without correctly having the canonical disk name.
>> 
>> When available, this driver also tries to use the deblocker #blocks and
>> a way of determining the disk size.
>> 
>> Finally and probably most importantly, this new driver is also capable of
>> seeing all partitions on a GPT disk.  With the old driver, the GPT
>> partition table can not be read and only the first partition on the disk
>> can be seen.
>> 
>> Signed-off-by: Eric Snowberg <address@hidden>
> 
> In the general I am OK with the patch but...
> 
>> ---
>> Changes since V2 (all requested in the last review):
>> 
>> Changed all if (x) to if (x != NULL)
>> 
>> Removed static NULL initializations
>> 
>> Removed all code to allow a user to easily retrieve the disk name thru the
>> shell without needing to understand the grub2 escaping policy for
>> disk names with commas. SPARC disks frequently contain commas, figuring
>> this out is now up to the user. Cut and pasting the disk name is no
>> longer possible. Also, this will make automated testing thru the shell
>> extremely difficult.
>> 
>> Changed the order of the functions to escape_commas(),
>> replace_escaped_commas(), and finally count_commas().
> 
> ... I am a bit confused with this. You left escape_commas() and
> count_commas(). Both are used in encode_grub_devname(). IMO this does
> not agree with "Removed all code to allow a user to easily retrieve the
> disk name thru the shell without needing to understand the grub2
> escaping policy for disk names with commas." Does it?
> 

The disk enumeration code within this driver retrieves device names. For 
example it can find all SAS disks in a system.  These device names are not in a 
format understandable by GRUB2.  These functions are required for converting a 
device name OpenFirmware understands to a device name GRUB2 understands. These 
functions have nothing to do with the code for a user to easily retrieve the 
disk name thru the shell.