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

[Simon Glass]

Hi Johan,

On Fri, 18 Oct 2024 at 07:33, Johan Jonker <jbx6244 at 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 at 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