[PATCH 1/3] rockchip: block: simplify rkmtd driver
Johan Jonker
jbx6244 at gmail.com
Tue Oct 22 21:58:13 CEST 2024
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 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>
>>>> ---
Tested-by: Johan Jonker <jbx6244 at 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 at 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
More information about the U-Boot
mailing list