[PATCH 0/2] kunit: fix minor error path mistakes

14 views
Skip to first unread message

Wander Lairson Costa

unread,
Apr 18, 2024, 9:18:05 AMApr 18
to Brendan Higgins, David Gow, Rae Moar, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
Hi,

These two patches fix some minor error path mistakes in the device
module.

Wander Lairson Costa (2):
kunit: unregister the device on error
kunit: avoid memory leak on device register error

lib/kunit/device.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

--
2.44.0

Wander Lairson Costa

unread,
Apr 18, 2024, 9:18:12 AMApr 18
to Brendan Higgins, David Gow, Rae Moar, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
kunit_init_device() should unregister the device on bus register error.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
---
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;
}

--
2.44.0

Wander Lairson Costa

unread,
Apr 18, 2024, 9:18:13 AMApr 18
to Brendan Higgins, David Gow, Rae Moar, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
---
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);
}

--
2.44.0

Ivan Orlov

unread,
Apr 18, 2024, 10:43:23 AMApr 18
to kuni...@googlegroups.com
Maybe it worth adding a 'Fixes' tag, since it is definitely a bug.
--
Kind regards,
Ivan Orlov

Markus Elfring

unread,
Apr 18, 2024, 11:00:39 AMApr 18
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Rae Moar, LKML
> kunit_init_device() should unregister the device on bus register error.

* Would another imperative wording be desirable also for this change description?

* Will the tag “Fixes” become relevant here?

Regards,
Markus

Wander Lairson Costa

unread,
Apr 18, 2024, 11:21:37 AMApr 18
to Markus Elfring, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Rae Moar, LKML
On Thu, Apr 18, 2024 at 12:06 PM Markus Elfring <Markus....@web.de> wrote:
>
> > kunit_init_device() should unregister the device on bus register error.
>


> * Would another imperative wording be desirable also for this change description?

It makes sense, I will change the comment description.

>
> * Will the tag “Fixes” become relevant here?

I often forget this tag. I will add it.

>
> Regards,
> Markus
>

Markus Elfring

unread,
Apr 18, 2024, 11:24:50 AMApr 18
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Rae Moar, LKML
> If the device register fails, free the allocated memory before
> returning.

* I suggest to use the word “registration” (instead of “register”)
in the commit message.

* Would you like to add the tag “Fixes” accordingly?


> +++ 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);
> }

Common error handling code can be used instead
if an additional label would be applied for a corresponding jump target.

How do you think about to increase the application of scope-based resource management here?

Regards,
Markus

Wander Lairson Costa

unread,
Apr 18, 2024, 12:00:52 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Shuah Khan, Maxime Ripard, Greg Kroah-Hartman, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
Hi,

These two patches fix some minor error path mistakes in the device
module.

Changes:
--------

v1->v2:
* Add fixes tag.
* Add an imperative statement in the first commit descripton.

Wander Lairson Costa

unread,
Apr 18, 2024, 12:00:55 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Maxime Ripard, Shuah Khan, Greg Kroah-Hartman, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
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 <wan...@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;
}

--
2.44.0

Wander Lairson Costa

unread,
Apr 18, 2024, 12:01:00 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Matti Vaittinen, Greg Kroah-Hartman, Maxime Ripard, Shuah Khan, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@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);
}

--
2.44.0

Maxime Ripard

unread,
Apr 18, 2024, 12:18:28 PMApr 18
to Wander Lairson Costa, Brendan Higgins, David Gow, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, open list, "open list:KERNEL UNIT TESTING FRAMEWORK (KUnit)", open list:KERNEL UNIT TESTING FRAMEWORK (KUnit)
On Thu, 18 Apr 2024 13:00:36 -0300, Wander Lairson Costa wrote:
> Hi,
>
> These two patches fix some minor error path mistakes in the device
> module.
>
>
> [ ... ]

Reviewed-by: Maxime Ripard <mri...@kernel.org>

Thanks!
Maxime

Wander Lairson Costa

unread,
Apr 18, 2024, 1:19:15 PMApr 18
to Markus Elfring, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Rae Moar, LKML
I thought about that. But I think the code is simple enough (for now)
to not require an exit label.

> Regards,
> Markus
>

Markus Elfring

unread,
Apr 18, 2024, 2:08:25 PMApr 18
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Rae Moar, LKML
>> Common error handling code can be used instead
>> if an additional label would be applied for a corresponding jump target.
>>
>> How do you think about to increase the application of scope-based resource management here?
>
> I thought about that. But I think the code is simple enough (for now)
> to not require an exit label.

Please follow a known advice (besides other recommended improvements).
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

Regards,
Markus

Wander Lairson Costa

unread,
Apr 18, 2024, 5:02:46 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Shuah Khan, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
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

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(-)

--
2.44.0

Wander Lairson Costa

unread,
Apr 18, 2024, 5:02:48 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Shuah Khan, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
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 <wan...@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

Wander Lairson Costa

unread,
Apr 18, 2024, 5:02:57 PMApr 18
to Brendan Higgins, David Gow, Rae Moar, Greg Kroah-Hartman, Shuah Khan, Maxime Ripard, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
---
lib/kunit/device.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 25c81ed465fb..bc2e2032e505 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -119,10 +119,8 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
kunit_dev->owner = test;

err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
- if (err) {
- kfree(kunit_dev);
- return ERR_PTR(err);
- }
+ if (err)
+ goto error;

kunit_dev->dev.release = kunit_device_release;
kunit_dev->dev.bus = &kunit_bus_type;
@@ -131,7 +129,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);
- return ERR_PTR(err);
+ goto error;
}

kunit_dev->dev.dma_mask = &kunit_dev->dev.coherent_dma_mask;
@@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);

return kunit_dev;
+error:
+ kfree(kunit_dev);
+ return ERR_PTR(err);
}

/*
--
2.44.0

David Gow

unread,
Apr 19, 2024, 12:58:54 AMApr 19
to Wander Lairson Costa, Brendan Higgins, Rae Moar, Shuah Khan, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@redhat.com> 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 <wan...@redhat.com>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> ---

Nice catch!

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

David Gow

unread,
Apr 19, 2024, 12:58:56 AMApr 19
to Wander Lairson Costa, Brendan Higgins, Rae Moar, Greg Kroah-Hartman, Shuah Khan, Maxime Ripard, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@redhat.com> wrote:
>
> If the device register fails, free the allocated memory before
> returning.
>
> Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> ---

Thanks.

I'm not sure this is correct, though... Shouldn't put_device() free this for us?

The documentation for device_register() says to never free a device
after device_register() has been called, even if it fails:
https://docs.kernel.org/driver-api/infrastructure.html#c.device_register

Or am I missing something?

Cheers,
-- David

Markus Elfring

unread,
Apr 19, 2024, 1:41:06 AMApr 19
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML
> kunit_init_device() should unregister the device on bus register error,
> but mistakenly it tries to unregister the bus.

Would the following description variant be more appropriate?

kunit_init_device() should unregister the device on bus registration error.
But it mistakenly tries to unregister the bus.


Regards,
Markus

Greg Kroah-Hartman

unread,
Apr 19, 2024, 2:07:11 AMApr 19
to Markus Elfring, Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML
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

Markus Elfring

unread,
Apr 19, 2024, 2:15:30 AMApr 19
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML
> If the device register fails, free the allocated memory before
> returning.

Can a description variant (like the following) be more appropriate?

Free the allocated memory (after a device registration failure)
before returning.
Thus add a jump target so that a bit of exception handling can be better
reused at the end of this function implementation.


Would you like to replace the word “register” by “registration” also
in the summary phrase?



> +++ b/lib/kunit/device.c

> @@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
> kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
>
> return kunit_dev;
> +error:
> + kfree(kunit_dev);
> + return ERR_PTR(err);
> }


I find it nicer to use a label like free_device.

Regards,
Markus

Greg Kroah-Hartman

unread,
Apr 19, 2024, 2:34:06 AMApr 19
to Markus Elfring, Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML
On Fri, Apr 19, 2024 at 08:15:25AM +0200, Markus Elfring wrote:
> > If the device register fails, free the allocated memory before
> > returning.
>
> Can a description variant (like the following) be more appropriate?
>
> Free the allocated memory (after a device registration failure)
> before returning.
> Thus add a jump target so that a bit of exception handling can be better
> reused at the end of this function implementation.
>
>
> Would you like to replace the word “register” by “registration” also
> in the summary phrase?
>

Wander Lairson Costa

unread,
Apr 19, 2024, 8:30:22 AMApr 19
to David Gow, Brendan Higgins, Rae Moar, Greg Kroah-Hartman, Shuah Khan, Maxime Ripard, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
On Fri, Apr 19, 2024 at 1:59 AM David Gow <davi...@google.com> wrote:
>
> On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@redhat.com> wrote:
> >
> > If the device register fails, free the allocated memory before
> > returning.
> >
> > Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
> > Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> > ---
>
> Thanks.
>
> I'm not sure this is correct, though... Shouldn't put_device() free this for us?
>
> The documentation for device_register() says to never free a device
> after device_register() has been called, even if it fails:
> https://docs.kernel.org/driver-api/infrastructure.html#c.device_register
>
> Or am I missing something?
>

I am not freeing the device object passed to device_register, but its
parent structure.

As a side note, the behavior of device_register() seems
counterintuitive and error-prone, IMO. If the function returns an
error, it should ensure it leaks no resource and shouldn't require the
caller to do any cleanup.

Wander Lairson Costa

unread,
Apr 19, 2024, 9:25:17 AMApr 19
to Brendan Higgins, David Gow, Rae Moar, Matti Vaittinen, Greg Kroah-Hartman, Shuah Khan, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
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

unread,
Apr 19, 2024, 9:25:26 AMApr 19
to Brendan Higgins, David Gow, Rae Moar, Shuah Khan, Matti Vaittinen, Greg Kroah-Hartman, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@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);

Wander Lairson Costa

unread,
Apr 19, 2024, 9:25:28 AMApr 19
to Brendan Higgins, David Gow, Rae Moar, Matti Vaittinen, Shuah Khan, Greg Kroah-Hartman, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list, Wander Lairson Costa
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 <wan...@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

Greg Kroah-Hartman

unread,
Apr 19, 2024, 9:59:57 AMApr 19
to Wander Lairson Costa, David Gow, Brendan Higgins, Rae Moar, Shuah Khan, Maxime Ripard, Matti Vaittinen, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
On Fri, Apr 19, 2024 at 09:30:06AM -0300, Wander Lairson Costa wrote:
> As a side note, the behavior of device_register() seems
> counterintuitive and error-prone, IMO. If the function returns an
> error, it should ensure it leaks no resource and shouldn't require the
> caller to do any cleanup.

I too want a pony, but that's not the way the code works here, sorry.
It's always been like this, and has always been a problem, but last time
I looked, there was no way to really fix this. That's why we document
it a lot to make sure people don't get the error paths wrong here. I
know it's a pain :(

sorry,

greg k-h

Greg Kroah-Hartman

unread,
Apr 19, 2024, 10:03:20 AMApr 19
to Wander Lairson Costa, Brendan Higgins, David Gow, Rae Moar, Shuah Khan, Matti Vaittinen, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
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

Greg Kroah-Hartman

unread,
Apr 19, 2024, 10:04:13 AMApr 19
to Wander Lairson Costa, Brendan Higgins, David Gow, Rae Moar, Matti Vaittinen, Shuah Khan, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
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 <wan...@redhat.com>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")

Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Wander Lairson Costa

unread,
Apr 19, 2024, 10:11:45 AMApr 19
to Greg Kroah-Hartman, Brendan Higgins, David Gow, Rae Moar, Shuah Khan, Matti Vaittinen, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
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
>

Markus Elfring

unread,
Apr 19, 2024, 12:18:26 PMApr 19
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML

> * Remove some changes requested by Marcus Elfring,

I became curious how affected software components can evolve further.


> as I was alerted he is a known troll.

I would appreciate if this interpretation will be reconsidered somehow.

Regards,
Markus

Markus Elfring

unread,
Apr 19, 2024, 12:32:10 PMApr 19
to Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Greg Kroah-Hartman, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML
> > 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 <gre...@linuxfoundation.org>

Would you ever like to distinguish hardware register errors from
item registration failures according to further improved commit messages?

Regards,
Markus

Greg Kroah-Hartman

unread,
Apr 20, 2024, 2:27:32 AMApr 20
to Markus Elfring, Wander Lairson Costa, kuni...@googlegroups.com, linux-k...@vger.kernel.org, kernel-...@vger.kernel.org, Brendan Higgins, David Gow, Matti Vaittinen, Maxime Ripard, Rae Moar, Shuah Khan, LKML

David Gow

unread,
Apr 23, 2024, 4:00:41 AMApr 23
to Wander Lairson Costa, Greg Kroah-Hartman, Brendan Higgins, Rae Moar, Shuah Khan, Matti Vaittinen, Maxime Ripard, open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list:KERNEL UNIT TESTING FRAMEWORK (KUnit), open list
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/commit/?h=kunit&id=63761ec5971ea47c1f2d7698f03e1c6ffb9fb22a

Cheers,
-- David
Reply all
Reply to author
Forward
0 new messages