On Thu, Apr 02, 2020 at 02:16:18PM +0100, Rui Miguel Silva wrote: > > > --- a/drivers/staging/greybus/light.c > > > +++ b/drivers/staging/greybus/light.c > > > @@ -1026,7 +1026,8 @@ static int gb_lights_light_config(struct gb_lights *glights, u8 id) > > > > > > light->channels_count = conf.channel_count; > > > light->name = kstrndup(conf.name, NAMES_MAX, GFP_KERNEL); > > > - > > > + if (!light->name) > > > + return -ENOMEM; > > > light->channels = kcalloc(light->channels_count, > > > sizeof(struct gb_channel), GFP_KERNEL); > > > if (!light->channels) > > > > The clean up in this function is non-existant. :( > > Yeah, this have a central point to do the cleanups, gb_lights_release, > since we may have other lights already configured at this point, we > could cleanup this specific one here, but than would need to make sure > all other already configure got clean also. Central clean up functions never work correctly. For example, we allocate "cdev->name" in gb_lights_channel_config() before we register the channel later in gb_lights_register_all(glights);. Now imagine that the register fails. Then when we're freeing it in __gb_lights_led_unregister() we see that the ->is_registered is false so we don't kfree(cdev->name). That's just a small memory leak. But there are going to be tons of little bugs like that. Anyway it doesn't affect this patch so it's fine. regards, dan carpenter _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|