Hi,
These two patches fix some minor error path mistakes in the device module.
Changes -------
v1->v2 * Add fixes tag * Add imperative statement in the commit description v2->v3 * Add a goto exit label kunit_device_register_internal v3->v4 * Remove some changes requested by Marcus Elfring, as I was alerted he is a known troll.
Wander Lairson Costa (2): kunit: unregister the device on error kunit: avoid memory leak on device register error
lib/kunit/device.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") --- lib/kunit/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index abc603730b8e..25c81ed465fb 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -51,7 +51,7 @@ int kunit_bus_init(void)
error = bus_register(&kunit_bus_type); if (error) - bus_unregister(&kunit_bus_type); + root_device_unregister(kunit_bus_device); return error; }
On Fri, Apr 19, 2024 at 10:25:01AM -0300, Wander Lairson Costa wrote:
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
…
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Would you ever like to distinguish hardware register errors from item registration failures according to further improved commit messages?
Regards, Markus
On Fri, Apr 19, 2024 at 06:32:05PM +0200, Markus Elfring wrote:
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
…
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Would you ever like to distinguish hardware register errors from item registration failures according to further improved commit messages?
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.
thanks,
greg k-h's patch email bot
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") --- lib/kunit/device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..d8c09dcb3e79 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev); + kfree(kunit_dev); return ERR_PTR(err); }
On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa wrote:
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
lib/kunit/device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..d8c09dcb3e79 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev);
kfree(kunit_dev);
This still looks wrong, the release function for the device should free the memory here, not this kfree, as the reference count in the embedded 'struct device' handles the memory logic for the whole structure (if not, then something is REALLY wrong...)
You _do_ have a release function for the device, right? If not, you should be getting loud messages in the kernel log when releasing a device here.
thanks,
greg k-h
On Fri, Apr 19, 2024 at 11:03 AM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa wrote:
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
lib/kunit/device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..d8c09dcb3e79 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev);
kfree(kunit_dev);This still looks wrong, the release function for the device should free the memory here, not this kfree, as the reference count in the embedded 'struct device' handles the memory logic for the whole structure (if not, then something is REALLY wrong...)
You _do_ have a release function for the device, right? If not, you should be getting loud messages in the kernel log when releasing a device here.
Ok, I got it. Yes, there is a release function. So this patch is wrong, ignore it. Should I send a v5 with only the other patch?
thanks,
greg k-h
On Fri, 19 Apr 2024 at 22:11, Wander Lairson Costa wander@redhat.com wrote:
On Fri, Apr 19, 2024 at 11:03 AM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa wrote:
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
lib/kunit/device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..d8c09dcb3e79 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev);
kfree(kunit_dev);This still looks wrong, the release function for the device should free the memory here, not this kfree, as the reference count in the embedded 'struct device' handles the memory logic for the whole structure (if not, then something is REALLY wrong...)
You _do_ have a release function for the device, right? If not, you should be getting loud messages in the kernel log when releasing a device here.
Ok, I got it. Yes, there is a release function. So this patch is wrong, ignore it. Should I send a v5 with only the other patch?
Thanks. Don't worry about sending a v5: just patch 1 of v4 is now in the kunit branch: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
Cheers, -- David
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Greg Kroah-Hartman -
Markus Elfring -
Wander Lairson Costa