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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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