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

[Sascha Hauer <sha@pengutronix.de>]

On Thu, Jun 01, 2023 at 06:56:47AM +0200, 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.

As a compromise you could use a 'conflict' and 'identical' label and
place them directly under each other. No functional change, but makes
the intention clearer.

Sascha

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