Re: [PATCH 10/18] cdev: have devfs_add_partition return existing identical partition, not NULL

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-06-01, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 31.05.23 19:23, Marco Felsch wrote:
> > Hi Ahmad,
> > 
> > On 23-05-31, Ahmad Fatoum wrote:
> >> Starting with commit 7f9f45b9bfef ("devfs: Do not create overlapping
> >> partitions"), any overlapping is disallowed. Overlapping can be useful
> >> though to bridge the gap between partition described in DT and via
> >> on-disk partition tables. Let's handle the case of identical partitions
> >> specially and have it neither be an error or a duplicate partition, but
> >> instead just return the existing partition. This existing partition will
> >> be given a device tree node and thus enabling schemes like:
> >>
> >>   &{/state} {
> >>   	backend = <&state_part>;
> >>   };
> >>
> >>   &mmc1 {
> >>          partitions {
> >>                  compatible = "fixed-partitions";
> >>                  #address-cells = <2>;
> >>                  #size-cells = <2>;
> >>
> >>                  state_part: partition@5300000 {
> >>                          label = "barebox-state";
> >> 			 /* will be folded with overlapping GPT partition if found */
> >>                          reg = <0x0 0x5300000 0x0 0x100000>;
> >>                  };
> >>          };
> >>   };
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  fs/devfs-core.c | 50 ++++++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 39 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> >> index a0732dafca42..b3a274d01ee0 100644
> >> --- a/fs/devfs-core.c
> >> +++ b/fs/devfs-core.c
> >> @@ -402,6 +402,12 @@ int devfs_remove(struct cdev *cdev)
> >>  	return 0;
> >>  }
> >>  
> >> +static bool region_identical(loff_t starta, loff_t lena,
> >> +			     loff_t startb, loff_t lenb)
> >> +{
> >> +	return starta == startb && lena == lenb;
> >> +}
> >> +
> >>  static bool region_overlap(loff_t starta, loff_t lena,
> >>  			   loff_t startb, loff_t lenb)
> >>  {
> >> @@ -412,10 +418,22 @@ static bool region_overlap(loff_t starta, loff_t lena,
> >>  	return 1;
> >>  }
> >>  
> >> -static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >> +/**
> >> + * check_overlap() - check overlap with existing partitions
> >> + * @cdev: parent cdev
> >> + * @name: partition name for informational purposes on conflict
> >> + * @offset: offset of new partition to be added
> >> + * @size: size of new partition to be added
> >> + *
> >> + * Return: NULL if no overlapping partition found or overlapping
> >> + *         partition if and only if it's identical in offset and size
> >> + *         to an existing partition. Otherwise, PTR_ERR(-EINVAL).
> >> + */
> >> +static struct cdev *check_overlap(struct cdev *cdev, const char *name, loff_t offset, loff_t size)
> >>  {
> >>  	struct cdev *cpart;
> >>  	loff_t cpart_offset;
> >> +	int ret;
> >>  
> >>  	list_for_each_entry(cpart, &cdev->partitions, partition_entry) {
> >>  		cpart_offset = cpart->offset;
> >> @@ -428,20 +446,28 @@ static int check_overlap(struct cdev *cdev, const char *name, loff_t offset, lof
> >>  		if (cpart->mtd)
> >>  			cpart_offset = cpart->mtd->master_offset;
> >>  
> >> -		if (region_overlap(cpart_offset, cpart->size,
> >> -				   offset, size))
> >> +		if (region_identical(cpart_offset, cpart->size, offset, size)) {
> >> +			ret = 0;
> >>  			goto conflict;
> >> +		}
> > 
> > The 'goto conflict' is a bit misleading here since this is no conflict
> > as you described within the commit message.
> 
> It's still a conflict, but one that can be resolved by returning the existing
> partition.
> 
> > I would rather do:
> > 
> > 		if (region_identical(cpart_offset, cpart->size, offset, size))
> > 			goto out_identical;
> > 
> > and replace the __pr_printk() by pr_debug(). This way you split
> > __pr_printk() and drop the ternary operator. The rest lgtm.
> 
> The print line is going to be long anyway, so why not share it between
> the two cases?

It's long but at least to me it is easier to read and later on to grep
for albeit it already has many parameters. Anyway this is just a nit and
at least to me easier to read since you don't need the additional ret
parameter which you need to re-evaluate later on.

Regards,
  Marco

> 
> > 
> > Regards,
> >   Marco
> > 
> >> +
> >> +		if (region_overlap(cpart_offset, cpart->size, offset, size)) {
> >> +			ret = -EINVAL;
> >> +			goto conflict;
> >> +		}
> >>  	}
> >>  
> >> -	return 0;
> >> +	return NULL;
> >>  
> >>  conflict:
> >> -	pr_err("New partition %s (0x%08llx-0x%08llx) on %s "
> >> -		"overlaps with partition %s (0x%08llx-0x%08llx), not creating it\n",
> >> -		name, offset, offset + size - 1, cdev->name,
> >> -		cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
> >> +	__pr_printk(ret ? MSG_WARNING : MSG_DEBUG,
> >> +		    "New partition %s (0x%08llx-0x%08llx) on %s "
> >> +		    "%s with partition %s (0x%08llx-0x%08llx), not creating it\n",
> >> +		    name, offset, offset + size - 1, cdev->name,
> >> +		    ret ? "conflicts" : "identical",
> >> +		    cpart->name, cpart_offset, cpart_offset + cpart->size - 1);
> >>  
> >> -	return -EINVAL;
> >> +	return ret ? ERR_PTR(ret) : cpart;
> >>  }
> >>  
> >>  static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >> @@ -449,6 +475,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >>  {
> >>  	loff_t offset, size;
> >>  	static struct cdev *new;
> >> +	struct cdev *overlap;
> >>  
> >>  	if (cdev_by_name(partinfo->name))
> >>  		return ERR_PTR(-EEXIST);
> >> @@ -479,8 +506,9 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> >>  		return ERR_PTR(-EINVAL);
> >>  	}
> >>  
> >> -	if (check_overlap(cdev, partinfo->name, offset, size))
> >> -		return ERR_PTR(-EINVAL);
> >> +	overlap = check_overlap(cdev, partinfo->name, offset, size);
> >> +	if (overlap)
> >> +		return overlap;
> >>  
> >>  	if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
> >>  		struct mtd_info *mtd;
> >> -- 
> >> 2.39.2
> >>
> >>
> >>
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
>