Re: [PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-05-31 Thread AKASHI Takahiro 
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 
> > > > ---
> > > >   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 
> > > 
> > > > +   } 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();
> > >

Re: [PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-05-18 Thread AKASHI Takahiro 
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 
> > > ---
> > >   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 
> > 
> > > + } 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();
> >

Re: [PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-05-08 Thread AKASHI Takahiro 
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 
> > ---
> >   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 
> 
> > +   } 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();
>

Re: [PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-04-28 Thread Mark Kettenis 
> From: AKASHI Takahiro 
> 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 
> ---
>  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 
 
> 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
> 
>

Re: [PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-04-28 Thread Heinrich Schuchardt 

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 
---
  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 


+   } 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();

[PATCH] efi_loader: disk: allow blk devices even without UCLASS_PARTITION

2022-04-27 Thread AKASHI Takahiro 
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 
---
 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);
+   } 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