Re: [PATCH 09/15] blk: blkmap: Support mapping to device of any block size

[Simon Glass]

On Tue, 26 Sept 2023 at 02:53, Bin Meng  wrote:
>
> At present if a device to map has a block size other than 512,
> the blkmap map process just fails. There is no reason why we
> can't just use the block size of the mapped device.
>
> Signed-off-by: Bin Meng 
> ---
>
>  drivers/block/blkmap.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass 

Re: [PATCH 09/15] blk: blkmap: Support mapping to device of any block size

[Bin Meng]

Hi Tobias,

On Wed, Sep 27, 2023 at 3:29 AM Tobias Waldekranz  wrote:
>
> On tis, sep 26, 2023 at 16:43, Bin Meng  wrote:
> > At present if a device to map has a block size other than 512,
> > the blkmap map process just fails. There is no reason why we
> > can't just use the block size of the mapped device.
>
> Won't this be very confusing to the user?

I don't see any confusion.

>
> The blkmap device uses a fixed block size of 512:
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blkmap.c?ref_type=heads#L393

Yes, the blkmap device was originally created with a fixed block size
of 512, and that's fine.

>
> So if I map a slice of a 4k device into a blkmap, then
>
> blkmap read 0x8000 0 1
>
> would copy 4k instead of 512 bytes from the lower device to 0x8000,
> even though the blkmap reports a block size of 512.
>
> It seems to me that the expected behavior would be that only the first
> 512 bytes would be copied in the command above.

No, the blkmap block size was later updated to match the real
underlying device parameter during the map process. So it will copy
all 4k to 0x8000.

>
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  drivers/block/blkmap.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
> > index f6acfa8927..149a4cac3e 100644
> > --- a/drivers/block/blkmap.c
> > +++ b/drivers/block/blkmap.c
> > @@ -171,11 +171,11 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t 
> > blknr, lbaint_t blkcnt,
> >
> >   bd = dev_get_uclass_plat(bm->blk);
> >   lbd = dev_get_uclass_plat(lblk);
> > - if (lbd->blksz != bd->blksz)
> > - /* We could support block size translation, but we
> > -  * don't yet.
> > -  */
>
> Hence this comment ^

This comment was completely removed with the new updates. There is no
need to do any block size translation. We could just use whatever
block size the lower device is using, hence this patch.

>
> > - return -EINVAL;
> > + if (lbd->blksz != bd->blksz) {
> > + /* update to match the mapped device */
> > + bd->blksz = lbd->blksz;
> > + bd->log2blksz = LOG2(bd->blksz);
> > + }
> >
> >   linear = malloc(sizeof(*linear));
> >   if (!linear)
> > --

Regards,
Bin

Re: [PATCH 09/15] blk: blkmap: Support mapping to device of any block size

[Tobias Waldekranz]

On tis, sep 26, 2023 at 16:43, Bin Meng  wrote:
> At present if a device to map has a block size other than 512,
> the blkmap map process just fails. There is no reason why we
> can't just use the block size of the mapped device.

Won't this be very confusing to the user?

The blkmap device uses a fixed block size of 512:

https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blkmap.c?ref_type=heads#L393

So if I map a slice of a 4k device into a blkmap, then

blkmap read 0x8000 0 1

would copy 4k instead of 512 bytes from the lower device to 0x8000,
even though the blkmap reports a block size of 512.

It seems to me that the expected behavior would be that only the first
512 bytes would be copied in the command above.

>
> Signed-off-by: Bin Meng 
> ---
>
>  drivers/block/blkmap.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
> index f6acfa8927..149a4cac3e 100644
> --- a/drivers/block/blkmap.c
> +++ b/drivers/block/blkmap.c
> @@ -171,11 +171,11 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t 
> blknr, lbaint_t blkcnt,
>  
>   bd = dev_get_uclass_plat(bm->blk);
>   lbd = dev_get_uclass_plat(lblk);
> - if (lbd->blksz != bd->blksz)
> - /* We could support block size translation, but we
> -  * don't yet.
> -  */

Hence this comment ^

> - return -EINVAL;
> + if (lbd->blksz != bd->blksz) {
> + /* update to match the mapped device */
> + bd->blksz = lbd->blksz;
> + bd->log2blksz = LOG2(bd->blksz);
> + }
>  
>   linear = malloc(sizeof(*linear));
>   if (!linear)
> -- 
> 2.25.1

[PATCH 09/15] blk: blkmap: Support mapping to device of any block size

[Bin Meng]

At present if a device to map has a block size other than 512,
the blkmap map process just fails. There is no reason why we
can't just use the block size of the mapped device.

Signed-off-by: Bin Meng 
---

 drivers/block/blkmap.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
index f6acfa8927..149a4cac3e 100644
--- a/drivers/block/blkmap.c
+++ b/drivers/block/blkmap.c
@@ -171,11 +171,11 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t 
blknr, lbaint_t blkcnt,
 
bd = dev_get_uclass_plat(bm->blk);
lbd = dev_get_uclass_plat(lblk);
-   if (lbd->blksz != bd->blksz)
-   /* We could support block size translation, but we
-* don't yet.
-*/
-   return -EINVAL;
+   if (lbd->blksz != bd->blksz) {
+   /* update to match the mapped device */
+   bd->blksz = lbd->blksz;
+   bd->log2blksz = LOG2(bd->blksz);
+   }
 
linear = malloc(sizeof(*linear));
if (!linear)
-- 
2.25.1