[PATCH 0/3] block: make blk_create_device() static

[PATCH 0/3] block: make blk_create_device() static

[Heinrich Schuchardt]

There should be a defined interface between block devices drivers and the
block class layer. Most drivers already use blk_create_devicef() to
generate block devices. The EFI block device driver and the Rockchip
rkmtd driver are the exceptions.

* Convert the remaining drivers to use blk_create_devicef().
* Make blk_create_device() a static function.

This will ensure that future block drivers will use blk_create_devicef(),
too.

Heinrich Schuchardt (3):
  rockchip: block: simplify rkmtd driver
  efi_driver: use blk_create_devicef()
  block: make blk_create_device() static

 drivers/block/blk-uclass.c        | 19 ++++++++++++++++---
 drivers/block/rkmtd.c             | 21 ++-------------------
 include/blk.h                     | 17 -----------------
 lib/efi_driver/efi_block_device.c |  8 +++-----
 4 files changed, 21 insertions(+), 44 deletions(-)

-- 
2.45.2

[PATCH 1/3] rockchip: block: simplify rkmtd driver

[Heinrich Schuchardt]

By using blk_create_devicef() instead of blk_create_devicef() the driver
can be simplified and brought into line with other block device drivers.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/block/rkmtd.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/block/rkmtd.c b/drivers/block/rkmtd.c
index c55f052e51b..f84cacd7ead 100644
--- a/drivers/block/rkmtd.c
+++ b/drivers/block/rkmtd.c
@@ -794,36 +794,19 @@ int rkmtd_init_plat(struct udevice *dev)
 	return 0;
 }
 
-static void rkmtd_blk_kmalloc_release(struct udevice *dev, void *res)
-{
-	/* noop */
-}
-
 static int rkmtd_bind(struct udevice *dev)
 {
 	struct rkmtd_dev *plat = dev_get_plat(dev);
-	char dev_name[30], *str;
 	struct blk_desc *desc;
 	struct udevice *bdev;
 	int ret;
 
-	snprintf(dev_name, sizeof(dev_name), "%s.%s", dev->name, "blk");
-
-	str = devres_alloc(rkmtd_blk_kmalloc_release, strlen(dev_name) + 1, GFP_KERNEL);
-	if (unlikely(!str))
-		return -ENOMEM;
-
-	strcpy(str, dev_name);
-
-	ret = blk_create_device(dev, "rkmtd_blk", str, UCLASS_RKMTD,
-				-1, 512, LBA, &bdev);
+	ret = blk_create_devicef(dev, "rkmtd_blk", "blk", UCLASS_RKMTD,
+				 -1, 512, LBA, &bdev);
 	if (ret) {
-		free(str);
 		return log_msg_ret("blk", ret);
 	}
 
-	devres_add(dev, str);
-
 	desc = dev_get_uclass_plat(bdev);
 	sprintf(desc->vendor, "0x%.4x", 0x2207);
 	memcpy(desc->product, "RKMTD", sizeof("RKMTD"));
-- 
2.45.2

[PATCH 2/3] efi_driver: use blk_create_devicef()

[Heinrich Schuchardt]

The EFI block device driver is the only user of blk_create_device() outside
the block device uclass. Use blk_create_devicef() instead like other block
device drivers.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/efi_driver/efi_block_device.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
index 34a0365739d..19a5ee24794 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -133,15 +133,13 @@ efi_bl_create_block_device(efi_handle_t handle, void *interface)
 	sprintf(name, "efiblk#%d", devnum);
 
 	/* Create driver model udevice for the EFI block io device */
-	if (blk_create_device(parent, "efi_blk", name, UCLASS_EFI_LOADER,
-			      devnum, io->media->block_size,
-			      (lbaint_t)io->media->last_block, &bdev)) {
+	if (blk_create_devicef(parent, "efi_blk", name, UCLASS_EFI_LOADER,
+			       devnum, io->media->block_size,
+			       (lbaint_t)io->media->last_block, &bdev)) {
 		ret = EFI_OUT_OF_RESOURCES;
 		free(name);
 		goto err;
 	}
-	/* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */
-	device_set_name_alloced(bdev);
 
 	plat = dev_get_plat(bdev);
 	plat->handle = handle;
-- 
2.45.2

[PATCH 3/3] block: make blk_create_device() static

[Heinrich Schuchardt]

There are no users of the blk_create_device() function outside the uclass.
Let's make it static. This will ensure that new block drivers will use
blk_create_devicef().

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/block/blk-uclass.c | 19 ++++++++++++++++---
 include/blk.h              | 17 -----------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 312e038445c..f3ac8db9464 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -695,9 +695,22 @@ static int blk_claim_devnum(enum uclass_id uclass_id, int devnum)
 	return -ENOENT;
 }
 
-int blk_create_device(struct udevice *parent, const char *drv_name,
-		      const char *name, int uclass_id, int devnum, int blksz,
-		      lbaint_t lba, struct udevice **devp)
+/**
+ * blk_create_device() - Create a new block device
+ *
+ * @parent:	Parent of the new device
+ * @drv_name:	Driver name to use for the block device
+ * @name:	Name for the device
+ * @uclass_id:	Interface type (enum uclass_id_t)
+ * @devnum:	Device number, specific to the interface type, or -1 to
+ *		allocate the next available number
+ * @blksz:	Block size of the device in bytes (typically 512)
+ * @lba:	Total number of blocks of the device
+ * @devp:	the new device (which has not been probed)
+ */
+static int blk_create_device(struct udevice *parent, const char *drv_name,
+			     const char *name, int uclass_id, int devnum,
+			     int blksz, lbaint_t lba, struct udevice **devp)
 {
 	struct blk_desc *desc;
 	struct udevice *dev;
diff --git a/include/blk.h b/include/blk.h
index 1fc9a5b8471..261c7dd6616 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -376,23 +376,6 @@ int blk_first_device(int uclass_id, struct udevice **devp);
  */
 int blk_next_device(struct udevice **devp);
 
-/**
- * blk_create_device() - Create a new block device
- *
- * @parent:	Parent of the new device
- * @drv_name:	Driver name to use for the block device
- * @name:	Name for the device
- * @uclass_id:	Interface type (enum uclass_id_t)
- * @devnum:	Device number, specific to the interface type, or -1 to
- *		allocate the next available number
- * @blksz:	Block size of the device in bytes (typically 512)
- * @lba:	Total number of blocks of the device
- * @devp:	the new device (which has not been probed)
- */
-int blk_create_device(struct udevice *parent, const char *drv_name,
-		      const char *name, int uclass_id, int devnum, int blksz,
-		      lbaint_t lba, struct udevice **devp);
-
 /**
  * blk_create_devicef() - Create a new named block device
  *
-- 
2.45.2

Re: [PATCH 1/3] rockchip: block: simplify rkmtd driver

[Johan Jonker]

On 10/18/24 03:30, Heinrich Schuchardt wrote:
> By using blk_create_devicef() instead of blk_create_devicef() the driver
> can be simplified and brought into line with other block device drivers.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/block/rkmtd.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/block/rkmtd.c b/drivers/block/rkmtd.c
> index c55f052e51b..f84cacd7ead 100644
> --- a/drivers/block/rkmtd.c
> +++ b/drivers/block/rkmtd.c
> @@ -794,36 +794,19 @@ int rkmtd_init_plat(struct udevice *dev)
>  	return 0;
>  }
>  
> -static void rkmtd_blk_kmalloc_release(struct udevice *dev, void *res)
> -{
> -	/* noop */
> -}
> -
>  static int rkmtd_bind(struct udevice *dev)
>  {
>  	struct rkmtd_dev *plat = dev_get_plat(dev);
> -	char dev_name[30], *str;
>  	struct blk_desc *desc;
>  	struct udevice *bdev;
>  	int ret;
>  
> -	snprintf(dev_name, sizeof(dev_name), "%s.%s", dev->name, "blk");
> -

> -	str = devres_alloc(rkmtd_blk_kmalloc_release, strlen(dev_name) + 1, GFP_KERNEL);

Hi Heinrich, Simon,

The function strdup() is not an exact replacement for the devres_alloc() function in relation to a device.
It is in use for memory leak detection / device resource management. 
Not sure what the status of that devres project currently is? Also tracking in general in relation to EFI and blk-class.

https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blk-uclass.c?ref_type=heads#L739

https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/rkmtd.c?ref_type=heads#L812

Test for this driver are based on work done by Simon.
https://source.denx.de/u-boot/u-boot/-/blob/master/test/dm/host.c?ref_type=heads#L71

This rkmtd driver makes use of devm_kzalloc(). All the memory that rkmtd reserves is freed.
But if I remember well somehow I was not able to free all memory on unbind and was not able to find the source, so I didn't include that last test.
https://elixir.bootlin.com/u-boot/v2024.10/source/test/dm/rkmtd.c#L84

Johan







> -	if (unlikely(!str))
> -		return -ENOMEM;
> -
> -	strcpy(str, dev_name);
> -
> -	ret = blk_create_device(dev, "rkmtd_blk", str, UCLASS_RKMTD,
> -				-1, 512, LBA, &bdev);
> +	ret = blk_create_devicef(dev, "rkmtd_blk", "blk", UCLASS_RKMTD,
> +				 -1, 512, LBA, &bdev);
>  	if (ret) {
> -		free(str);
>  		return log_msg_ret("blk", ret);
>  	}
>  
> -	devres_add(dev, str);
> -
>  	desc = dev_get_uclass_plat(bdev);
>  	sprintf(desc->vendor, "0x%.4x", 0x2207);
>  	memcpy(desc->product, "RKMTD", sizeof("RKMTD"));

Re: [PATCH 3/3] block: make blk_create_device() static

[Simon Glass]

On Thu, 17 Oct 2024 at 19:30, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> There are no users of the blk_create_device() function outside the uclass.
> Let's make it static. This will ensure that new block drivers will use
> blk_create_devicef().
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/block/blk-uclass.c | 19 ++++++++++++++++---
>  include/blk.h              | 17 -----------------
>  2 files changed, 16 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/3] efi_driver: use blk_create_devicef()

[Simon Glass]

On Thu, 17 Oct 2024 at 19:30, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The EFI block device driver is the only user of blk_create_device() outside
> the block device uclass. Use blk_create_devicef() instead like other block
> device drivers.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_driver/efi_block_device.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/3] rockchip: block: simplify rkmtd driver

[Simon Glass]

Hi Johan,

On Fri, 18 Oct 2024 at 07:33, Johan Jonker <jbx6244@gmail.com> wrote:
>
>
>
> On 10/18/24 03:30, Heinrich Schuchardt wrote:
> > By using blk_create_devicef() instead of blk_create_devicef() the driver
> > can be simplified and brought into line with other block device drivers.
> >
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> >  drivers/block/rkmtd.c | 21 ++-------------------
> >  1 file changed, 2 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/block/rkmtd.c b/drivers/block/rkmtd.c
> > index c55f052e51b..f84cacd7ead 100644
> > --- a/drivers/block/rkmtd.c
> > +++ b/drivers/block/rkmtd.c
> > @@ -794,36 +794,19 @@ int rkmtd_init_plat(struct udevice *dev)
> >       return 0;
> >  }
> >
> > -static void rkmtd_blk_kmalloc_release(struct udevice *dev, void *res)
> > -{
> > -     /* noop */
> > -}
> > -
> >  static int rkmtd_bind(struct udevice *dev)
> >  {
> >       struct rkmtd_dev *plat = dev_get_plat(dev);
> > -     char dev_name[30], *str;
> >       struct blk_desc *desc;
> >       struct udevice *bdev;
> >       int ret;
> >
> > -     snprintf(dev_name, sizeof(dev_name), "%s.%s", dev->name, "blk");
> > -
>
> > -     str = devres_alloc(rkmtd_blk_kmalloc_release, strlen(dev_name) + 1, GFP_KERNEL);
>
> Hi Heinrich, Simon,
>
> The function strdup() is not an exact replacement for the devres_alloc() function in relation to a device.
> It is in use for memory leak detection / device resource management.
> Not sure what the status of that devres project currently is? Also tracking in general in relation to EFI and blk-class.
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blk-uclass.c?ref_type=heads#L739
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/rkmtd.c?ref_type=heads#L812
>
> Test for this driver are based on work done by Simon.
> https://source.denx.de/u-boot/u-boot/-/blob/master/test/dm/host.c?ref_type=heads#L71
>
> This rkmtd driver makes use of devm_kzalloc(). All the memory that rkmtd reserves is freed.
> But if I remember well somehow I was not able to free all memory on unbind and was not able to find the source, so I didn't include that last test.
> https://elixir.bootlin.com/u-boot/v2024.10/source/test/dm/rkmtd.c#L84

Yes, you can use that API. But I think Heinrich's patch is correct,
and separate from your points here. Yes, blk_create_devicef()
strdup()s the name, but it sets a flag indicating that it has done so.
Then device_unbind() checks DM_FLAG_NAME_ALLOCED later. So the code
should be equivalent.

Re the memory leak, I don't have any ideas, but test/dm/devres.c does
have some checks for some of that. I have often wondered about having
a way to store the filename and line (or just the line?) with every
allocation, so we can list out what is left at the end, since valgrind
doesn't work on target devices.

Regards,
Simon

Re: [PATCH 3/3] block: make blk_create_device() static

[Ilias Apalodimas]

On Fri, 18 Oct 2024 at 04:30, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> There are no users of the blk_create_device() function outside the uclass.
> Let's make it static. This will ensure that new block drivers will use
> blk_create_devicef().
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/block/blk-uclass.c | 19 ++++++++++++++++---
>  include/blk.h              | 17 -----------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 312e038445c..f3ac8db9464 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -695,9 +695,22 @@ static int blk_claim_devnum(enum uclass_id uclass_id, int devnum)
>         return -ENOENT;
>  }
>
> -int blk_create_device(struct udevice *parent, const char *drv_name,
> -                     const char *name, int uclass_id, int devnum, int blksz,
> -                     lbaint_t lba, struct udevice **devp)
> +/**
> + * blk_create_device() - Create a new block device
> + *
> + * @parent:    Parent of the new device
> + * @drv_name:  Driver name to use for the block device
> + * @name:      Name for the device
> + * @uclass_id: Interface type (enum uclass_id_t)
> + * @devnum:    Device number, specific to the interface type, or -1 to
> + *             allocate the next available number
> + * @blksz:     Block size of the device in bytes (typically 512)
> + * @lba:       Total number of blocks of the device
> + * @devp:      the new device (which has not been probed)
> + */
> +static int blk_create_device(struct udevice *parent, const char *drv_name,
> +                            const char *name, int uclass_id, int devnum,
> +                            int blksz, lbaint_t lba, struct udevice **devp)
>  {
>         struct blk_desc *desc;
>         struct udevice *dev;
> diff --git a/include/blk.h b/include/blk.h
> index 1fc9a5b8471..261c7dd6616 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -376,23 +376,6 @@ int blk_first_device(int uclass_id, struct udevice **devp);
>   */
>  int blk_next_device(struct udevice **devp);
>
> -/**
> - * blk_create_device() - Create a new block device
> - *
> - * @parent:    Parent of the new device
> - * @drv_name:  Driver name to use for the block device
> - * @name:      Name for the device
> - * @uclass_id: Interface type (enum uclass_id_t)
> - * @devnum:    Device number, specific to the interface type, or -1 to
> - *             allocate the next available number
> - * @blksz:     Block size of the device in bytes (typically 512)
> - * @lba:       Total number of blocks of the device
> - * @devp:      the new device (which has not been probed)
> - */
> -int blk_create_device(struct udevice *parent, const char *drv_name,
> -                     const char *name, int uclass_id, int devnum, int blksz,
> -                     lbaint_t lba, struct udevice **devp);
> -
>  /**
>   * blk_create_devicef() - Create a new named block device
>   *
> --
> 2.45.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/3] rockchip: block: simplify rkmtd driver

[Heinrich Schuchardt]

On 10/19/24 13:50, Simon Glass wrote:
> Hi Johan,
> 
> On Fri, 18 Oct 2024 at 07:33, Johan Jonker <jbx6244@gmail.com> wrote:
>>
>>
>>
>> On 10/18/24 03:30, Heinrich Schuchardt wrote:
>>> By using blk_create_devicef() instead of blk_create_devicef() the driver
>>> can be simplified and brought into line with other block device drivers.
>>>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> ---
>>>   drivers/block/rkmtd.c | 21 ++-------------------
>>>   1 file changed, 2 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/block/rkmtd.c b/drivers/block/rkmtd.c
>>> index c55f052e51b..f84cacd7ead 100644
>>> --- a/drivers/block/rkmtd.c
>>> +++ b/drivers/block/rkmtd.c
>>> @@ -794,36 +794,19 @@ int rkmtd_init_plat(struct udevice *dev)
>>>        return 0;
>>>   }
>>>
>>> -static void rkmtd_blk_kmalloc_release(struct udevice *dev, void *res)
>>> -{
>>> -     /* noop */
>>> -}
>>> -
>>>   static int rkmtd_bind(struct udevice *dev)
>>>   {
>>>        struct rkmtd_dev *plat = dev_get_plat(dev);
>>> -     char dev_name[30], *str;
>>>        struct blk_desc *desc;
>>>        struct udevice *bdev;
>>>        int ret;
>>>
>>> -     snprintf(dev_name, sizeof(dev_name), "%s.%s", dev->name, "blk");
>>> -
>>
>>> -     str = devres_alloc(rkmtd_blk_kmalloc_release, strlen(dev_name) + 1, GFP_KERNEL);
>>
>> Hi Heinrich, Simon,
>>
>> The function strdup() is not an exact replacement for the devres_alloc() function in relation to a device.
>> It is in use for memory leak detection / device resource management.
>> Not sure what the status of that devres project currently is? Also tracking in general in relation to EFI and blk-class.
>>
>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blk-uclass.c?ref_type=heads#L739
>>
>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/rkmtd.c?ref_type=heads#L812
>>
>> Test for this driver are based on work done by Simon.
>> https://source.denx.de/u-boot/u-boot/-/blob/master/test/dm/host.c?ref_type=heads#L71
>>
>> This rkmtd driver makes use of devm_kzalloc(). All the memory that rkmtd reserves is freed.
>> But if I remember well somehow I was not able to free all memory on unbind and was not able to find the source, so I didn't include that last test.
>> https://elixir.bootlin.com/u-boot/v2024.10/source/test/dm/rkmtd.c#L84
> 
> Yes, you can use that API. But I think Heinrich's patch is correct,
> and separate from your points here. Yes, blk_create_devicef()
> strdup()s the name, but it sets a flag indicating that it has done so.
> Then device_unbind() checks DM_FLAG_NAME_ALLOCED later. So the code
> should be equivalent.
> 
> Re the memory leak, I don't have any ideas, but test/dm/devres.c does
> have some checks for some of that. I have often wondered about having
> a way to store the filename and line (or just the line?) with every
> allocation, so we can list out what is left at the end, since valgrind
> doesn't work on target devices.
> 
> Regards,
> Simon

Hello Johan,

I hope that Simon's comment clarifies how freeing the string resource works.

Do you have a device to test this patch?

Best regards

Heinrich

Re: [PATCH 1/3] rockchip: block: simplify rkmtd driver

[Johan Jonker]

On 10/21/24 15:35, Heinrich Schuchardt wrote:
> On 10/19/24 13:50, Simon Glass wrote:
>> Hi Johan,
>>
>> On Fri, 18 Oct 2024 at 07:33, Johan Jonker <jbx6244@gmail.com> wrote:
>>>
>>>
>>>
>>> On 10/18/24 03:30, Heinrich Schuchardt wrote:
>>>> By using blk_create_devicef() instead of blk_create_devicef() the driver
>>>> can be simplified and brought into line with other block device drivers.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---

Tested-by: Johan Jonker <jbx6244@gmail.com>

>>>>   drivers/block/rkmtd.c | 21 ++-------------------
>>>>   1 file changed, 2 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/block/rkmtd.c b/drivers/block/rkmtd.c
>>>> index c55f052e51b..f84cacd7ead 100644
>>>> --- a/drivers/block/rkmtd.c
>>>> +++ b/drivers/block/rkmtd.c
>>>> @@ -794,36 +794,19 @@ int rkmtd_init_plat(struct udevice *dev)
>>>>        return 0;
>>>>   }
>>>>
>>>> -static void rkmtd_blk_kmalloc_release(struct udevice *dev, void *res)
>>>> -{
>>>> -     /* noop */
>>>> -}
>>>> -
>>>>   static int rkmtd_bind(struct udevice *dev)
>>>>   {
>>>>        struct rkmtd_dev *plat = dev_get_plat(dev);
>>>> -     char dev_name[30], *str;
>>>>        struct blk_desc *desc;
>>>>        struct udevice *bdev;
>>>>        int ret;
>>>>
>>>> -     snprintf(dev_name, sizeof(dev_name), "%s.%s", dev->name, "blk");
>>>> -
>>>
>>>> -     str = devres_alloc(rkmtd_blk_kmalloc_release, strlen(dev_name) + 1, GFP_KERNEL);
>>>
>>> Hi Heinrich, Simon,
>>>
>>> The function strdup() is not an exact replacement for the devres_alloc() function in relation to a device.
>>> It is in use for memory leak detection / device resource management.
>>> Not sure what the status of that devres project currently is? Also tracking in general in relation to EFI and blk-class.
>>>
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/blk-uclass.c?ref_type=heads#L739
>>>
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/rkmtd.c?ref_type=heads#L812
>>>
>>> Test for this driver are based on work done by Simon.
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/test/dm/host.c?ref_type=heads#L71
>>>
>>> This rkmtd driver makes use of devm_kzalloc(). All the memory that rkmtd reserves is freed.
>>> But if I remember well somehow I was not able to free all memory on unbind and was not able to find the source, so I didn't include that last test.
>>> https://elixir.bootlin.com/u-boot/v2024.10/source/test/dm/rkmtd.c#L84
>>
>> Yes, you can use that API. But I think Heinrich's patch is correct,
>> and separate from your points here. Yes, blk_create_devicef()
>> strdup()s the name, but it sets a flag indicating that it has done so.
>> Then device_unbind() checks DM_FLAG_NAME_ALLOCED later. So the code
>> should be equivalent.
>>

>> Re the memory leak, I don't have any ideas, but test/dm/devres.c does
>> have some checks for some of that. I have often wondered about having
>> a way to store the filename and line (or just the line?) with every
>> allocation, so we can list out what is left at the end, since valgrind
>> doesn't work on target devices.

Please have a look at a little test patch below.
I'm out of ideas. All I reserve for RKMTD as memory is freed I think.
Also replaced a second devres_alloc() function with strdup() with the some result.
The difference with the host device is that it adds an GPT/EFI partition.
Maybe it add a few bytes while parsing the partitions?
Anyone able verify a partition on other block devices then RKMTD in a test?

Johan


>>
>> Regards,
>> Simon
> 
> Hello Johan,
> 
> I hope that Simon's comment clarifies how freeing the string resource works.
> 
> Do you have a device to test this patch?
> 
> Best regards
> 
> Heinrich

From b767850c66e51b4b4d2b8f9f9d314dd430b9a8d0 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6244@gmail.com>
Date: Tue, 22 Oct 2024 20:12:37 +0200
Subject: [PATCH v1] RKMTD memory leak test

To reproduce:
git clone --depth 1 https://source.denx.de/u-boot/u-boot.git
make sandbox_defconfig
make

./u-boot -T -c "ut dm dm_test_rkmtd"

Test: dm_test_rkmtd: rkmtd.c (flat tree)
test/dm/rkmtd.c:95, dm_test_rkmtd(): report:
mem_start: 2844096
mem_test1: 3374736
mem_exit: 2844128
diff: 32

Or inside U-boot:
./u-boot

ut dm dm_test_rkmtd

Test: dm_test_rkmtd: rkmtd.c (flat tree)
test/dm/rkmtd.c:95, dm_test_rkmtd(): report:
mem_start: 113040
mem_test1: 643824
mem_exit: 113216
diff: 176
---
 test/dm/rkmtd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/test/dm/rkmtd.c b/test/dm/rkmtd.c
index d1ca5d1acacb..ba8e211f60a2 100644
--- a/test/dm/rkmtd.c
+++ b/test/dm/rkmtd.c
@@ -29,11 +29,14 @@ static int dm_test_rkmtd(struct unit_test_state *uts)
 	struct rkmtd_dev *plat;
 	struct blk_desc *desc;
 	struct sector0 *sec0;
+	ulong mem_start, mem_exit, mem_test1;
 	int i;
 
 	ut_asserteq(-ENODEV, uclass_first_device_err(UCLASS_RKMTD, &dev));
 	ut_asserteq(-ENODEV, uclass_first_device_err(UCLASS_PARTITION, &part));
 
+	mem_start = ut_check_delta(0);
+
 	ut_assertok(rkmtd_create_device(label, &dev));
 
 	/* Check that the plat data has been allocated */
@@ -78,11 +81,19 @@ static int dm_test_rkmtd(struct unit_test_state *uts)
 	ut_asserteq(12, blk_dread(desc, 64, 12, read));
 	ut_asserteq_mem(write, read, RW_BUF_SIZE);
 
+	mem_test1 = ut_check_delta(0);
+
 	ut_assertok(rkmtd_detach(dev));
 
 	ut_asserteq(-ENODEV, blk_get_from_parent(dev, &blk));
 	ut_assertok(device_unbind(dev));
 
+	/* check there were no memory leaks */
+	//ut_asserteq(0, ut_check_delta(mem_start));
+
+	mem_exit = ut_check_delta(0);
+	ut_reportf("\nmem_start: %lu\nmem_test1: %lu\nmem_exit: %lu\ndiff: %lu\n", mem_start, mem_test1, mem_exit, mem_exit - mem_start);
+
 	return 0;
 }
 DM_TEST(dm_test_rkmtd, UTF_SCAN_FDT);
-- 
2.39.2

Re: (subset) [PATCH 0/3] block: make blk_create_device() static

[Tom Rini]

On Fri, 18 Oct 2024 03:30:12 +0200, Heinrich Schuchardt wrote:

> There should be a defined interface between block devices drivers and the
> block class layer. Most drivers already use blk_create_devicef() to
> generate block devices. The EFI block device driver and the Rockchip
> rkmtd driver are the exceptions.
> 
> * Convert the remaining drivers to use blk_create_devicef().
> * Make blk_create_device() a static function.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom