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

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

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