Re: [PATCH 1/2] staging: greybus: light: Don't leak memory for no gain

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

Hi!

> On Tue, Jul 25, 2017 at 02:30:31PM +0200, Johan Hovold wrote:
> > [ +CC: Rui and Greg ]
> 
> Thanks Johan. I only got this because of you.

> > >  	return ret;
> > >  }
> > 
> > And while it's fine to take this through linux-media, it would still be
> > good to keep the maintainers on CC.
> 
> Sakari, if you could resend the all series to the right lists and
> maintainers for proper review that would be great.
> 
> I did not get 0/2 and 2/2 patches.

0/2 and 2/2 were unrelated to the memory leak, IIRC. Let me google it
for you...

https://www.mail-archive.com/linux-media@vger.kernel.org/msg115840.html

This is memory leak and the driver is in staging. Acked-by or fixing
it yourself would be appropriate response, asking for resending of the
series... not quite so.

Best regards,

									Pavel

> > On Tue, Jul 18, 2017 at 09:41:06PM +0300, Sakari Ailus wrote:
> > > Memory for struct v4l2_flash_config is allocated in
> > > gb_lights_light_v4l2_register() for no gain and yet the allocated memory is
> > > leaked; the struct isn't used outside the function. Fix this.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/staging/greybus/light.c | 17 ++++++-----------
> > >  1 file changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> > > index 129ceed39829..b25c117ec41a 100644
> > > --- a/drivers/staging/greybus/light.c
> > > +++ b/drivers/staging/greybus/light.c
> > > @@ -534,25 +534,21 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  {
> > >  	struct gb_connection *connection = get_conn_from_light(light);
> > >  	struct device *dev = &connection->bundle->dev;
> > > -	struct v4l2_flash_config *sd_cfg;
> > > +	struct v4l2_flash_config sd_cfg = { 0 };
> > >  	struct led_classdev_flash *fled;
> > >  	struct led_classdev *iled = NULL;
> > >  	struct gb_channel *channel_torch, *channel_ind, *channel_flash;
> > >  	int ret = 0;
> > >  
> > > -	sd_cfg = kcalloc(1, sizeof(*sd_cfg), GFP_KERNEL);
> > > -	if (!sd_cfg)
> > > -		return -ENOMEM;
> > > -
> > >  	channel_torch = get_channel_from_mode(light, GB_CHANNEL_MODE_TORCH);
> > >  	if (channel_torch)
> > >  		__gb_lights_channel_v4l2_config(&channel_torch->intensity_uA,
> > > -						&sd_cfg->torch_intensity);
> > > +						&sd_cfg.torch_intensity);
> > >  
> > >  	channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> > >  	if (channel_ind) {
> > >  		__gb_lights_channel_v4l2_config(&channel_ind->intensity_uA,
> > > -						&sd_cfg->indicator_intensity);
> > > +						&sd_cfg.indicator_intensity);
> > >  		iled = &channel_ind->fled.led_cdev;
> > >  	}
> > >  
> > > @@ -561,17 +557,17 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  
> > >  	fled = &channel_flash->fled;
> > >  
> > > -	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
> > > +	snprintf(sd_cfg.dev_name, sizeof(sd_cfg.dev_name), "%s", light->name);
> > >  
> > >  	/* Set the possible values to faults, in our case all faults */
> > > -	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > > +	sd_cfg.flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
> > >  		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
> > >  		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
> > >  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
> > >  		LED_FAULT_LED_OVER_TEMPERATURE;
> > >  
> > >  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
> > > -					    &v4l2_flash_ops, sd_cfg);
> > > +					    &v4l2_flash_ops, &sd_cfg);
> > >  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> > >  		ret = PTR_ERR(light->v4l2_flash);
> > >  		goto out_free;
> > > @@ -580,7 +576,6 @@ static int gb_lights_light_v4l2_register(struct gb_light *light)
> > >  	return ret;
> > >  
> > >  out_free:
> > > -	kfree(sd_cfg);
> > 
> > This looks a bit lazy, even if I just noticed that you repurpose this
> > error label (without renaming it) in you second patch.
> > 
> > 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]