Re: [PATCH v2] disk: Use a helper function to reduce duplication

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

On Mon, Mar 20, 2023 at 09:31:58AM +0200, Ilias Apalodimas wrote:
> Hi Simon,
> 
> Patch looks good, but isn't the new function name a bit misleading?
> Something like blk_part_find_start() sounds a bit more descriptive, or am I
> missing something?

I don't think that the helper function works as my original code does.

> Cheers
> /Ilias
> 
> On Mon, Mar 20, 2023 at 08:29:57AM +1300, Simon Glass wrote:
> > Reduce the duplicated code slightly by using a helper function to handle
> > the common code.
> >
> > This reduces the code size very slightly.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Rebase to -next
> >
> >  disk/disk-uclass.c | 46 +++++++++++++++++++++++++---------------------
> >  1 file changed, 25 insertions(+), 21 deletions(-)
> >
> > diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
> > index d32747e2242d..7f1fd80b2248 100644
> > --- a/disk/disk-uclass.c
> > +++ b/disk/disk-uclass.c
> > @@ -65,26 +65,38 @@ int part_create_block_devices(struct udevice *blk_dev)
> >  	return 0;
> >  }
> >
> > +static int blk_part_setup(struct udevice *dev, lbaint_t *startp,
> > +			  lbaint_t blkcnt)
> > +{
> > +	struct disk_part *part;
> > +
> > +	part = dev_get_uclass_plat(dev);
> > +	if (*startp >= part->gpt_part_info.size)
> > +		return -E2BIG;
> > +
> > +	if (*startp + blkcnt > part->gpt_part_info.size)
> > +		blkcnt = part->gpt_part_info.size - *startp;
> > +	*startp += part->gpt_part_info.start;
> > +
> > +	return 0;
> > +}
> > +
> >  static ulong part_blk_read(struct udevice *dev, lbaint_t start,
> >  			   lbaint_t blkcnt, void *buffer)
> >  {
> >  	struct udevice *parent;
> > -	struct disk_part *part;
> >  	const struct blk_ops *ops;
> > +	int ret;
> >
> >  	parent = dev_get_parent(dev);
> >  	ops = blk_get_ops(parent);
> >  	if (!ops->read)
> >  		return -ENOSYS;
> >
> > -	part = dev_get_uclass_plat(dev);
> > -	if (start >= part->gpt_part_info.size)
> > +	ret = blk_part_setup(dev, &start, blkcnt);
> > +	if (ret)
> >  		return 0;
> >
> > -	if ((start + blkcnt) > part->gpt_part_info.size)
> > -		blkcnt = part->gpt_part_info.size - start;
> > -	start += part->gpt_part_info.start;
> > -

We cannot read out more blocks than the size of the partition.

> >  	return ops->read(parent, start, blkcnt, buffer);

So blkcnt must also be adjusted, otherwise we may see extra blocks
beyond the partition boundary.

-Takahiro Akashi


> >  }
> >
> > @@ -92,22 +104,18 @@ static ulong part_blk_write(struct udevice *dev, lbaint_t start,
> >  			    lbaint_t blkcnt, const void *buffer)
> >  {
> >  	struct udevice *parent;
> > -	struct disk_part *part;
> >  	const struct blk_ops *ops;
> > +	int ret;
> >
> >  	parent = dev_get_parent(dev);
> >  	ops = blk_get_ops(parent);
> >  	if (!ops->write)
> >  		return -ENOSYS;
> >
> > -	part = dev_get_uclass_plat(dev);
> > -	if (start >= part->gpt_part_info.size)
> > +	ret = blk_part_setup(dev, &start, blkcnt);
> > +	if (ret)
> >  		return 0;
> >
> > -	if ((start + blkcnt) > part->gpt_part_info.size)
> > -		blkcnt = part->gpt_part_info.size - start;
> > -	start += part->gpt_part_info.start;
> > -
> >  	return ops->write(parent, start, blkcnt, buffer);
> >  }
> >
> > @@ -115,22 +123,18 @@ static ulong part_blk_erase(struct udevice *dev, lbaint_t start,
> >  			    lbaint_t blkcnt)
> >  {
> >  	struct udevice *parent;
> > -	struct disk_part *part;
> >  	const struct blk_ops *ops;
> > +	int ret;
> >
> >  	parent = dev_get_parent(dev);
> >  	ops = blk_get_ops(parent);
> >  	if (!ops->erase)
> >  		return -ENOSYS;
> >
> > -	part = dev_get_uclass_plat(dev);
> > -	if (start >= part->gpt_part_info.size)
> > +	ret = blk_part_setup(dev, &start, blkcnt);
> > +	if (ret)
> >  		return 0;
> >
> > -	if ((start + blkcnt) > part->gpt_part_info.size)
> > -		blkcnt = part->gpt_part_info.size - start;
> > -	start += part->gpt_part_info.start;
> > -
> >  	return ops->erase(parent, start, blkcnt);
> >  }
> >
> > --
> > 2.40.0.rc1.284.g88254d51c5-goog
> >