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

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

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

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