[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V3] ieee1275: obdisk driver
From: |
Daniel Kiper |
Subject: |
Re: [PATCH V3] ieee1275: obdisk driver |
Date: |
Thu, 7 Mar 2019 20:05:33 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Wed, Mar 06, 2019 at 08:38:27AM -0700, Eric Snowberg wrote:
> > 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.
OK, thanks. Then Reviewed-by: Daniel Kiper <address@hidden>
If there are no objections I will apply this patch next week.
Thank you for doing the work.
Daniel