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