| Message ID | 20220428044916.52250-1-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 6c640422baa46c5061efa5d2ff1b386bca72253c |
| Headers | show |
| Series | efi_loader: disk: allow blk devices even without UCLASS_PARTITION | expand |
On 4/28/22 06:49, AKASHI Takahiro wrote: > While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is > seen optional under the current implementation. > So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. > > Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 8fb5b2363c45..f5b462fb164a 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > if (buffer_size & (blksz - 1)) > return EFI_BAD_BUFFER_SIZE; > > -#if CONFIG_IS_ENABLED(PARTITIONS) > - if (direction == EFI_DISK_READ) > - n = dev_read(diskobj->dev, lba, blocks, buffer); > - else > - n = dev_write(diskobj->dev, lba, blocks, buffer); > -#else > - /* dev is always a block device (UCLASS_BLK) */ > - struct blk_desc *desc; > + if (CONFIG_IS_ENABLED(PARTITIONS) && > + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > + if (direction == EFI_DISK_READ) > + n = dev_read(diskobj->dev, lba, blocks, buffer); > + else > + n = dev_write(diskobj->dev, lba, blocks, buffer); Thanks for the patch. It solves the problem with the block IO protocol. Why should dev_read() and dev_write only work for UCLASS_PARTITION? Can't we make it work for any block device? I think the treatment of different types of block devices should be moved to disk/disk-uclass.c. I will pull this patch as a fast fix. But this should not be the final design. Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > + } else { > + /* dev is a block device (UCLASS_BLK) */ > + struct blk_desc *desc; > > - desc = dev_get_uclass_plat(diskobj->dev); > - if (direction == EFI_DISK_READ) > - n = blk_dread(desc, lba, blocks, buffer); > - else > - n = blk_dwrite(desc, lba, blocks, buffer); > -#endif > + desc = dev_get_uclass_plat(diskobj->dev); > + if (direction == EFI_DISK_READ) > + n = blk_dread(desc, lba, blocks, buffer); > + else > + n = blk_dwrite(desc, lba, blocks, buffer); > + } > > /* We don't do interrupts, so check for timers cooperatively */ > efi_timer_check();
> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > Date: Thu, 28 Apr 2022 13:49:16 +0900 > > While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is > seen optional under the current implementation. > So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. > > Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) confirmed that this does indeed fix the regression Tested-by: Mark Kettenis <kettenis@openbsd.org> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 8fb5b2363c45..f5b462fb164a 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > if (buffer_size & (blksz - 1)) > return EFI_BAD_BUFFER_SIZE; > > -#if CONFIG_IS_ENABLED(PARTITIONS) > - if (direction == EFI_DISK_READ) > - n = dev_read(diskobj->dev, lba, blocks, buffer); > - else > - n = dev_write(diskobj->dev, lba, blocks, buffer); > -#else > - /* dev is always a block device (UCLASS_BLK) */ > - struct blk_desc *desc; > + if (CONFIG_IS_ENABLED(PARTITIONS) && > + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > + if (direction == EFI_DISK_READ) > + n = dev_read(diskobj->dev, lba, blocks, buffer); > + else > + n = dev_write(diskobj->dev, lba, blocks, buffer); > + } else { > + /* dev is a block device (UCLASS_BLK) */ > + struct blk_desc *desc; > > - desc = dev_get_uclass_plat(diskobj->dev); > - if (direction == EFI_DISK_READ) > - n = blk_dread(desc, lba, blocks, buffer); > - else > - n = blk_dwrite(desc, lba, blocks, buffer); > -#endif > + desc = dev_get_uclass_plat(diskobj->dev); > + if (direction == EFI_DISK_READ) > + n = blk_dread(desc, lba, blocks, buffer); > + else > + n = blk_dwrite(desc, lba, blocks, buffer); > + } > > /* We don't do interrupts, so check for timers cooperatively */ > efi_timer_check(); > -- > 2.33.0 > >
Simon, On Thu, Apr 28, 2022 at 12:43:03PM +0200, Heinrich Schuchardt wrote: > On 4/28/22 06:49, AKASHI Takahiro wrote: > > While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is > > seen optional under the current implementation. > > So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. > > > > Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 8fb5b2363c45..f5b462fb164a 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > > if (buffer_size & (blksz - 1)) > > return EFI_BAD_BUFFER_SIZE; > > > > -#if CONFIG_IS_ENABLED(PARTITIONS) > > - if (direction == EFI_DISK_READ) > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > - else > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > -#else > > - /* dev is always a block device (UCLASS_BLK) */ > > - struct blk_desc *desc; > > + if (CONFIG_IS_ENABLED(PARTITIONS) && > > + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > + if (direction == EFI_DISK_READ) > > + n = dev_read(diskobj->dev, lba, blocks, buffer); > > + else > > + n = dev_write(diskobj->dev, lba, blocks, buffer); > > Thanks for the patch. It solves the problem with the block IO protocol. > > Why should dev_read() and dev_write only work for UCLASS_PARTITION? I don't remember well, but I have expected that even a whole disk be represented as a partition (0). > Can't we make it work for any block device? I think the treatment of > different types of block devices should be moved to disk/disk-uclass.c. @Simon, what are your thoughts? -Takahiro Akashi > I will pull this patch as a fast fix. But this should not be the final > design. > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > + } else { > > + /* dev is a block device (UCLASS_BLK) */ > > + struct blk_desc *desc; > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > - if (direction == EFI_DISK_READ) > > - n = blk_dread(desc, lba, blocks, buffer); > > - else > > - n = blk_dwrite(desc, lba, blocks, buffer); > > -#endif > > + desc = dev_get_uclass_plat(diskobj->dev); > > + if (direction == EFI_DISK_READ) > > + n = blk_dread(desc, lba, blocks, buffer); > > + else > > + n = blk_dwrite(desc, lba, blocks, buffer); > > + } > > > > /* We don't do interrupts, so check for timers cooperatively */ > > efi_timer_check(); >
Hi Simon, On Mon, May 09, 2022 at 02:33:10PM +0900, AKASHI Takahiro wrote: > Simon, > > On Thu, Apr 28, 2022 at 12:43:03PM +0200, Heinrich Schuchardt wrote: > > On 4/28/22 06:49, AKASHI Takahiro wrote: > > > While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is > > > seen optional under the current implementation. > > > So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. > > > > > > Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > index 8fb5b2363c45..f5b462fb164a 100644 > > > --- a/lib/efi_loader/efi_disk.c > > > +++ b/lib/efi_loader/efi_disk.c > > > @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > > > if (buffer_size & (blksz - 1)) > > > return EFI_BAD_BUFFER_SIZE; > > > > > > -#if CONFIG_IS_ENABLED(PARTITIONS) > > > - if (direction == EFI_DISK_READ) > > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > > - else > > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > > -#else > > > - /* dev is always a block device (UCLASS_BLK) */ > > > - struct blk_desc *desc; > > > + if (CONFIG_IS_ENABLED(PARTITIONS) && > > > + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > > + if (direction == EFI_DISK_READ) > > > + n = dev_read(diskobj->dev, lba, blocks, buffer); > > > + else > > > + n = dev_write(diskobj->dev, lba, blocks, buffer); > > > > Thanks for the patch. It solves the problem with the block IO protocol. > > > > Why should dev_read() and dev_write only work for UCLASS_PARTITION? > > I don't remember well, but I have expected that even a whole disk be > represented as a partition (0). > > > Can't we make it work for any block device? I think the treatment of > > different types of block devices should be moved to disk/disk-uclass.c. > > @Simon, what are your thoughts? Gentle ping. Any comment here? -Takahiro Akashi > > -Takahiro Akashi > > > I will pull this patch as a fast fix. But this should not be the final > > design. > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > > + } else { > > > + /* dev is a block device (UCLASS_BLK) */ > > > + struct blk_desc *desc; > > > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > > - if (direction == EFI_DISK_READ) > > > - n = blk_dread(desc, lba, blocks, buffer); > > > - else > > > - n = blk_dwrite(desc, lba, blocks, buffer); > > > -#endif > > > + desc = dev_get_uclass_plat(diskobj->dev); > > > + if (direction == EFI_DISK_READ) > > > + n = blk_dread(desc, lba, blocks, buffer); > > > + else > > > + n = blk_dwrite(desc, lba, blocks, buffer); > > > + } > > > > > > /* We don't do interrupts, so check for timers cooperatively */ > > > efi_timer_check(); > >
Hi Simon, On Thu, May 19, 2022 at 02:11:02PM +0900, AKASHI Takahiro wrote: > Hi Simon, > > On Mon, May 09, 2022 at 02:33:10PM +0900, AKASHI Takahiro wrote: > > Simon, > > > > On Thu, Apr 28, 2022 at 12:43:03PM +0200, Heinrich Schuchardt wrote: > > > On 4/28/22 06:49, AKASHI Takahiro wrote: > > > > While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is > > > > seen optional under the current implementation. > > > > So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. > > > > > > > > Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- > > > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > > > index 8fb5b2363c45..f5b462fb164a 100644 > > > > --- a/lib/efi_loader/efi_disk.c > > > > +++ b/lib/efi_loader/efi_disk.c > > > > @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, > > > > if (buffer_size & (blksz - 1)) > > > > return EFI_BAD_BUFFER_SIZE; > > > > > > > > -#if CONFIG_IS_ENABLED(PARTITIONS) > > > > - if (direction == EFI_DISK_READ) > > > > - n = dev_read(diskobj->dev, lba, blocks, buffer); > > > > - else > > > > - n = dev_write(diskobj->dev, lba, blocks, buffer); > > > > -#else > > > > - /* dev is always a block device (UCLASS_BLK) */ > > > > - struct blk_desc *desc; > > > > + if (CONFIG_IS_ENABLED(PARTITIONS) && > > > > + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { > > > > + if (direction == EFI_DISK_READ) > > > > + n = dev_read(diskobj->dev, lba, blocks, buffer); > > > > + else > > > > + n = dev_write(diskobj->dev, lba, blocks, buffer); > > > > > > Thanks for the patch. It solves the problem with the block IO protocol. > > > > > > Why should dev_read() and dev_write only work for UCLASS_PARTITION? > > > > I don't remember well, but I have expected that even a whole disk be > > represented as a partition (0). > > > > > Can't we make it work for any block device? I think the treatment of > > > different types of block devices should be moved to disk/disk-uclass.c. > > > > @Simon, what are your thoughts? > > Gentle ping. Any comment here? Ping again. I hope that you have some opinion here as the maintainer of DM. -Takahiro Akashi > -Takahiro Akashi > > > > > -Takahiro Akashi > > > > > I will pull this patch as a fast fix. But this should not be the final > > > design. > > > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > > > > + } else { > > > > + /* dev is a block device (UCLASS_BLK) */ > > > > + struct blk_desc *desc; > > > > > > > > - desc = dev_get_uclass_plat(diskobj->dev); > > > > - if (direction == EFI_DISK_READ) > > > > - n = blk_dread(desc, lba, blocks, buffer); > > > > - else > > > > - n = blk_dwrite(desc, lba, blocks, buffer); > > > > -#endif > > > > + desc = dev_get_uclass_plat(diskobj->dev); > > > > + if (direction == EFI_DISK_READ) > > > > + n = blk_dread(desc, lba, blocks, buffer); > > > > + else > > > > + n = blk_dwrite(desc, lba, blocks, buffer); > > > > + } > > > > > > > > /* We don't do interrupts, so check for timers cooperatively */ > > > > efi_timer_check(); > > >
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 8fb5b2363c45..f5b462fb164a 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -99,21 +99,22 @@ static efi_status_t efi_disk_rw_blocks(struct efi_block_io *this, if (buffer_size & (blksz - 1)) return EFI_BAD_BUFFER_SIZE; -#if CONFIG_IS_ENABLED(PARTITIONS) - if (direction == EFI_DISK_READ) - n = dev_read(diskobj->dev, lba, blocks, buffer); - else - n = dev_write(diskobj->dev, lba, blocks, buffer); -#else - /* dev is always a block device (UCLASS_BLK) */ - struct blk_desc *desc; + if (CONFIG_IS_ENABLED(PARTITIONS) && + device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) { + if (direction == EFI_DISK_READ) + n = dev_read(diskobj->dev, lba, blocks, buffer); + else + n = dev_write(diskobj->dev, lba, blocks, buffer); + } else { + /* dev is a block device (UCLASS_BLK) */ + struct blk_desc *desc; - desc = dev_get_uclass_plat(diskobj->dev); - if (direction == EFI_DISK_READ) - n = blk_dread(desc, lba, blocks, buffer); - else - n = blk_dwrite(desc, lba, blocks, buffer); -#endif + desc = dev_get_uclass_plat(diskobj->dev); + if (direction == EFI_DISK_READ) + n = blk_dread(desc, lba, blocks, buffer); + else + n = blk_dwrite(desc, lba, blocks, buffer); + } /* We don't do interrupts, so check for timers cooperatively */ efi_timer_check();
While GPT partition is mandated in UEFI specification, CONFIG_PARTITION is seen optional under the current implementation. So modify efi_disk_rw_blocks() to allow accepting UCLASS_BLK devices. Fixes: commit d97e98c887ed ("efi_loader: disk: use udevice instead of blk_desc") Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_disk.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)