re: [media] Add driver for Samsung S5K5BAF camera sensor

re: [media] Add driver for Samsung S5K5BAF camera sensor

[Dan Carpenter]

Hello Andrzej Hajda,

This is a semi-automatic email about new static checker warnings.

The patch 7d459937dc09: "[media] Add driver for Samsung S5K5BAF 
camera sensor" from Dec 5, 2013, leads to the following Smatch 
complaint:

drivers/media/i2c/s5k5baf.c:554 s5k5baf_fw_get_seq()
	 warn: variable dereferenced before check 'fw' (see line 551)

drivers/media/i2c/s5k5baf.c
   550		struct s5k5baf_fw *fw = state->fw;
   551		u16 *data = fw->data + 2 * fw->count;
                                           ^^^^^^^^^
Dereference.

   552		int i;
   553	
   554		if (fw == NULL)
                    ^^^^^^^^^^
Check.

   555			return NULL;
   556	

regards,
dan carpenter

re: [media] Add driver for Samsung S5K5BAF camera sensor

[Dan Carpenter]

Hello Andrzej Hajda,

The patch 7d459937dc09: "[media] Add driver for Samsung S5K5BAF
camera sensor" from Dec 5, 2013, leads to the following
static checker warning:

	drivers/media/i2c/s5k5baf.c:1043 s5k5baf_set_power()
	warn: add some parenthesis here?

drivers/media/i2c/s5k5baf.c
  1036  static int s5k5baf_set_power(struct v4l2_subdev *sd, int on)
  1037  {
  1038          struct s5k5baf *state = to_s5k5baf(sd);
  1039          int ret = 0;
  1040  
  1041          mutex_lock(&state->lock);
  1042  
  1043          if (!on != state->power)
                    ^^^^^^^^^^^^^^^^^^^
This would be cleaner if it were "if (on == state->power)"

  1044                  goto out;
  1045  
  1046          if (on) {

regards,
dan carpenter

Re: [media] Add driver for Samsung S5K5BAF camera sensor

[Sachin Kamat]

Hi Dan,

On 8 January 2014 14:39, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Andrzej Hajda,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 7d459937dc09: "[media] Add driver for Samsung S5K5BAF
> camera sensor" from Dec 5, 2013, leads to the following Smatch
> complaint:
>
> drivers/media/i2c/s5k5baf.c:554 s5k5baf_fw_get_seq()
>          warn: variable dereferenced before check 'fw' (see line 551)
>
> drivers/media/i2c/s5k5baf.c
>    550          struct s5k5baf_fw *fw = state->fw;
>    551          u16 *data = fw->data + 2 * fw->count;
>                                            ^^^^^^^^^
> Dereference.
>
>    552          int i;
>    553
>    554          if (fw == NULL)
>                     ^^^^^^^^^^
> Check.
>
>    555                  return NULL;
>    556
>

A patch [1] to fix this has already been queued up by Mauro.

[1] https://linuxtv.org/patch/21292/

-- 
With warm regards,
Sachin

Re: [media] Add driver for Samsung S5K5BAF camera sensor

[Andrzej Hajda]

On 01/08/2014 10:58 AM, Dan Carpenter wrote:
> Hello Andrzej Hajda,
> 
> The patch 7d459937dc09: "[media] Add driver for Samsung S5K5BAF
> camera sensor" from Dec 5, 2013, leads to the following
> static checker warning:
> 
> 	drivers/media/i2c/s5k5baf.c:1043 s5k5baf_set_power()
> 	warn: add some parenthesis here?
> 
> drivers/media/i2c/s5k5baf.c
>   1036  static int s5k5baf_set_power(struct v4l2_subdev *sd, int on)
>   1037  {
>   1038          struct s5k5baf *state = to_s5k5baf(sd);
>   1039          int ret = 0;
>   1040  
>   1041          mutex_lock(&state->lock);
>   1042  
>   1043          if (!on != state->power)
>                     ^^^^^^^^^^^^^^^^^^^
> This would be cleaner if it were "if (on == state->power)"

This version works correctly only for 'on' equal 0 and 1, my version
works for all ints. On the other side documentation says only 0 and 1 is
allowed for s_power callbacks :)
I would stay with my version, similar approach is in other drivers.

Regards
Andrzej

> 
>   1044                  goto out;
>   1045  
>   1046          if (on) {
> 
> regards,
> dan carpenter
> 

Re: [media] Add driver for Samsung S5K5BAF camera sensor

[Dan Carpenter]

On Wed, Jan 08, 2014 at 12:58:35PM +0100, Andrzej Hajda wrote:
> On 01/08/2014 10:58 AM, Dan Carpenter wrote:
> > Hello Andrzej Hajda,
> > 
> > The patch 7d459937dc09: "[media] Add driver for Samsung S5K5BAF
> > camera sensor" from Dec 5, 2013, leads to the following
> > static checker warning:
> > 
> > 	drivers/media/i2c/s5k5baf.c:1043 s5k5baf_set_power()
> > 	warn: add some parenthesis here?
> > 
> > drivers/media/i2c/s5k5baf.c
> >   1036  static int s5k5baf_set_power(struct v4l2_subdev *sd, int on)
> >   1037  {
> >   1038          struct s5k5baf *state = to_s5k5baf(sd);
> >   1039          int ret = 0;
> >   1040  
> >   1041          mutex_lock(&state->lock);
> >   1042  
> >   1043          if (!on != state->power)
> >                     ^^^^^^^^^^^^^^^^^^^
> > This would be cleaner if it were "if (on == state->power)"
> 
> This version works correctly only for 'on' equal 0 and 1, my version
> works for all ints. On the other side documentation says only 0 and 1 is
> allowed for s_power callbacks :)
> I would stay with my version, similar approach is in other drivers.

Even "if (!!on == state->power)" like you do in s5k5baf_s_stream() would
be more readable than the current code.

regards,
dan carpenter