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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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?

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