| Message ID | 20240418210236.194190-3-wander@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: fix minor error path mistakes | expand |
On Fri, Apr 19, 2024 at 1:59 AM David Gow <davidgow@google.com> wrote: > > On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wander@redhat.com> 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") > > --- > > 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. > Cheers, > -- David > > > > 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 > >
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
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); } /*
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 | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)