Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-10 Thread Rui Miguel Silva 
Hi,
On Thu, Aug 10, 2017 at 04:56:50PM +0300, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Aug 10, 2017 at 03:02:46PM +0200, Hans Verkuil wrote:
> > > @@ -534,25 +534,20 @@ static int gb_lights_light_v4l2_register(struct 
> > > gb_light *light)
> > >  {
> > >   struct gb_connection *connection = get_conn_from_light(light);
> > >   struct device *dev = >bundle->dev;
> > > - struct v4l2_flash_config *sd_cfg;
> > > + struct v4l2_flash_config sd_cfg = { {0} };
> > 
> > Just use '= {};'
> 
> This is GCC specific whereas { {0} } is standard C. The latter is thus
> obviously better IMO.

My thought exactly. Let keep it this way, please.

> 
> > 
> > >   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(_torch->intensity_uA,
> > > - _cfg->torch_intensity);
> > > + _cfg.torch_intensity);
> > >  
> > >   channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> > >   if (channel_ind) {
> > >   __gb_lights_channel_v4l2_config(_ind->intensity_uA,
> > > - _cfg->indicator_intensity);
> > > + _cfg.indicator_intensity);
> > >   iled = _ind->fled.led_cdev;
> > >   }
> > >  
> > > @@ -561,27 +556,21 @@ static int gb_lights_light_v4l2_register(struct 
> > > gb_light *light)
> > >  
> > >   fled = _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,
> > > - _flash_ops, sd_cfg);
> > > - if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> > > - ret = PTR_ERR(light->v4l2_flash);
> > > - goto out_free;
> > > - }
> > > + _flash_ops, _cfg);
> > > + if (IS_ERR_OR_NULL(light->v4l2_flash))
> > 
> > Just IS_ERR since v4l2_flash_init() never returns NULL.
> 
> Will fix.

Thanks for that Sakari.

---
Cheers,
Rui

> 
> > 
> > > + return PTR_ERR(light->v4l2_flash);
> > >  
> > > - return ret;
> > > -
> > > -out_free:
> > > - kfree(sd_cfg);
> > > - return ret;
> > > + return 0;
> > >  }
> > >  
> > >  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
> > > 
> > 
> > After those two changes:
> > 
> > Acked-by: Hans Verkuil 
> 
> -- 
> Regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-10 Thread Sakari Ailus 
Hi Hans,

On Thu, Aug 10, 2017 at 03:02:46PM +0200, Hans Verkuil wrote:
> > @@ -534,25 +534,20 @@ static int gb_lights_light_v4l2_register(struct 
> > gb_light *light)
> >  {
> > struct gb_connection *connection = get_conn_from_light(light);
> > struct device *dev = >bundle->dev;
> > -   struct v4l2_flash_config *sd_cfg;
> > +   struct v4l2_flash_config sd_cfg = { {0} };
> 
> Just use '= {};'

This is GCC specific whereas { {0} } is standard C. The latter is thus
obviously better IMO.

> 
> > 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(_torch->intensity_uA,
> > -   _cfg->torch_intensity);
> > +   _cfg.torch_intensity);
> >  
> > channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
> > if (channel_ind) {
> > __gb_lights_channel_v4l2_config(_ind->intensity_uA,
> > -   _cfg->indicator_intensity);
> > +   _cfg.indicator_intensity);
> > iled = _ind->fled.led_cdev;
> > }
> >  
> > @@ -561,27 +556,21 @@ static int gb_lights_light_v4l2_register(struct 
> > gb_light *light)
> >  
> > fled = _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,
> > -   _flash_ops, sd_cfg);
> > -   if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> > -   ret = PTR_ERR(light->v4l2_flash);
> > -   goto out_free;
> > -   }
> > +   _flash_ops, _cfg);
> > +   if (IS_ERR_OR_NULL(light->v4l2_flash))
> 
> Just IS_ERR since v4l2_flash_init() never returns NULL.

Will fix.

> 
> > +   return PTR_ERR(light->v4l2_flash);
> >  
> > -   return ret;
> > -
> > -out_free:
> > -   kfree(sd_cfg);
> > -   return ret;
> > +   return 0;
> >  }
> >  
> >  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
> > 
> 
> After those two changes:
> 
> Acked-by: Hans Verkuil 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-10 Thread Hans Verkuil 
On 10/08/17 15:02, Hans Verkuil wrote:
> On 09/08/17 13:15, Sakari Ailus wrote:
>> From: Rui Miguel Silva 
>>
>> We are allocating memory for the v4l2 flash configuration structure and
>> leak it in the normal path. Just use the stack for this as we do not
>> use it outside of this function.
> 
> I'm not sure why this is part of this patch series. This is a greybus
> bug fix, right? And independent from the other two patches.

Ah, I see. The second patch sits on top of this change. Sorry, I was a bit
too quick there.

Hans

> 
>>
>> Fixes: 2870b52bae4c ("greybus: lights: add lights implementation")
>> Reported-by: Sakari Ailus 
>> Signed-off-by: Rui Miguel Silva 
>> Reviewed-by: Viresh Kumar 
>> ---
>>  drivers/staging/greybus/light.c | 29 +
>>  1 file changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/light.c 
>> b/drivers/staging/greybus/light.c
>> index 129ceed39829..0771db418f84 100644
>> --- a/drivers/staging/greybus/light.c
>> +++ b/drivers/staging/greybus/light.c
>> @@ -534,25 +534,20 @@ static int gb_lights_light_v4l2_register(struct 
>> gb_light *light)
>>  {
>>  struct gb_connection *connection = get_conn_from_light(light);
>>  struct device *dev = >bundle->dev;
>> -struct v4l2_flash_config *sd_cfg;
>> +struct v4l2_flash_config sd_cfg = { {0} };
> 
> Just use '= {};'
> 
>>  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(_torch->intensity_uA,
>> -_cfg->torch_intensity);
>> +_cfg.torch_intensity);
>>  
>>  channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
>>  if (channel_ind) {
>>  __gb_lights_channel_v4l2_config(_ind->intensity_uA,
>> -_cfg->indicator_intensity);
>> +_cfg.indicator_intensity);
>>  iled = _ind->fled.led_cdev;
>>  }
>>  
>> @@ -561,27 +556,21 @@ static int gb_lights_light_v4l2_register(struct 
>> gb_light *light)
>>  
>>  fled = _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,
>> -_flash_ops, sd_cfg);
>> -if (IS_ERR_OR_NULL(light->v4l2_flash)) {
>> -ret = PTR_ERR(light->v4l2_flash);
>> -goto out_free;
>> -}
>> +_flash_ops, _cfg);
>> +if (IS_ERR_OR_NULL(light->v4l2_flash))
> 
> Just IS_ERR since v4l2_flash_init() never returns NULL.
> 
>> +return PTR_ERR(light->v4l2_flash);
>>  
>> -return ret;
>> -
>> -out_free:
>> -kfree(sd_cfg);
>> -return ret;
>> +return 0;
>>  }
>>  
>>  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
>>
> 
> After those two changes:
> 
> Acked-by: Hans Verkuil 
> 
> Regards,
> 
>   Hans
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-10 Thread Hans Verkuil 
On 09/08/17 13:15, Sakari Ailus wrote:
> From: Rui Miguel Silva 
> 
> We are allocating memory for the v4l2 flash configuration structure and
> leak it in the normal path. Just use the stack for this as we do not
> use it outside of this function.

I'm not sure why this is part of this patch series. This is a greybus
bug fix, right? And independent from the other two patches.

> 
> Fixes: 2870b52bae4c ("greybus: lights: add lights implementation")
> Reported-by: Sakari Ailus 
> Signed-off-by: Rui Miguel Silva 
> Reviewed-by: Viresh Kumar 
> ---
>  drivers/staging/greybus/light.c | 29 +
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 129ceed39829..0771db418f84 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -534,25 +534,20 @@ static int gb_lights_light_v4l2_register(struct 
> gb_light *light)
>  {
>   struct gb_connection *connection = get_conn_from_light(light);
>   struct device *dev = >bundle->dev;
> - struct v4l2_flash_config *sd_cfg;
> + struct v4l2_flash_config sd_cfg = { {0} };

Just use '= {};'

>   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(_torch->intensity_uA,
> - _cfg->torch_intensity);
> + _cfg.torch_intensity);
>  
>   channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
>   if (channel_ind) {
>   __gb_lights_channel_v4l2_config(_ind->intensity_uA,
> - _cfg->indicator_intensity);
> + _cfg.indicator_intensity);
>   iled = _ind->fled.led_cdev;
>   }
>  
> @@ -561,27 +556,21 @@ static int gb_lights_light_v4l2_register(struct 
> gb_light *light)
>  
>   fled = _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,
> - _flash_ops, sd_cfg);
> - if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> - ret = PTR_ERR(light->v4l2_flash);
> - goto out_free;
> - }
> + _flash_ops, _cfg);
> + if (IS_ERR_OR_NULL(light->v4l2_flash))

Just IS_ERR since v4l2_flash_init() never returns NULL.

> + return PTR_ERR(light->v4l2_flash);
>  
> - return ret;
> -
> -out_free:
> - kfree(sd_cfg);
> - return ret;
> + return 0;
>  }
>  
>  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
> 

After those two changes:

Acked-by: Hans Verkuil 

Regards,

Hans
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-09 Thread Sakari Ailus 
Hi Rui,

On Wed, Aug 09, 2017 at 02:20:02PM +0100, Rui Miguel Silva wrote:
> Hi Sakari,
> On Wed, Aug 09, 2017 at 02:15:53PM +0300, Sakari Ailus wrote:
> > From: Rui Miguel Silva 
> > 
> > We are allocating memory for the v4l2 flash configuration structure and
> > leak it in the normal path. Just use the stack for this as we do not
> > use it outside of this function.
> > 
> > Fixes: 2870b52bae4c ("greybus: lights: add lights implementation")
> > Reported-by: Sakari Ailus 
> > Signed-off-by: Rui Miguel Silva 
> > Reviewed-by: Viresh Kumar 
> > ---
> 
> This patch is *not* the patch that I have send, here are the code
> differences from my patch to the one in this series:
> 
> < struct led_classdev_flash *iled = NULL;
> ---
> > struct led_classdev *iled = NULL;
> 51c57
> < iled = _ind->fled;
> ---
> > iled = _ind->fled.led_cdev;
> 89c95
> 
> So, this do not apply at all.
> Maybe you change something in your side.

It's been rebased on linux-next and in particular, patch 85f7ff9702bc
("media: v4l2-flash: Use led_classdev instead of led_classdev_flash for
indicator").

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/3] staging: greybus: light: fix memory leak in v4l2 register

2017-08-09 Thread Rui Miguel Silva 
Hi Sakari,
On Wed, Aug 09, 2017 at 02:15:53PM +0300, Sakari Ailus wrote:
> From: Rui Miguel Silva 
> 
> We are allocating memory for the v4l2 flash configuration structure and
> leak it in the normal path. Just use the stack for this as we do not
> use it outside of this function.
> 
> Fixes: 2870b52bae4c ("greybus: lights: add lights implementation")
> Reported-by: Sakari Ailus 
> Signed-off-by: Rui Miguel Silva 
> Reviewed-by: Viresh Kumar 
> ---

This patch is *not* the patch that I have send, here are the code
differences from my patch to the one in this series:

<   struct led_classdev_flash *iled = NULL;
---
>   struct led_classdev *iled = NULL;
51c57
<   iled = _ind->fled;
---
>   iled = _ind->fled.led_cdev;
89c95

So, this do not apply at all.
Maybe you change something in your side.

---
Cheers,
Rui


>  drivers/staging/greybus/light.c | 29 +
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 129ceed39829..0771db418f84 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -534,25 +534,20 @@ static int gb_lights_light_v4l2_register(struct 
> gb_light *light)
>  {
>   struct gb_connection *connection = get_conn_from_light(light);
>   struct device *dev = >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(_torch->intensity_uA,
> - _cfg->torch_intensity);
> + _cfg.torch_intensity);
>  
>   channel_ind = get_channel_from_mode(light, GB_CHANNEL_MODE_INDICATOR);
>   if (channel_ind) {
>   __gb_lights_channel_v4l2_config(_ind->intensity_uA,
> - _cfg->indicator_intensity);
> + _cfg.indicator_intensity);
>   iled = _ind->fled.led_cdev;
>   }
>  
> @@ -561,27 +556,21 @@ static int gb_lights_light_v4l2_register(struct 
> gb_light *light)
>  
>   fled = _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,
> - _flash_ops, sd_cfg);
> - if (IS_ERR_OR_NULL(light->v4l2_flash)) {
> - ret = PTR_ERR(light->v4l2_flash);
> - goto out_free;
> - }
> + _flash_ops, _cfg);
> + if (IS_ERR_OR_NULL(light->v4l2_flash))
> + return PTR_ERR(light->v4l2_flash);
>  
> - return ret;
> -
> -out_free:
> - kfree(sd_cfg);
> - return ret;
> + return 0;
>  }
>  
>  static void gb_lights_light_v4l2_unregister(struct gb_light *light)
> -- 
> 2.11.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel