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....
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/block/rkmtd.c?ref...
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=h...
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);