[PATCHv2 0/3] Three fixes to clean up daily build warnings

[PATCHv2 0/3] Three fixes to clean up daily build warnings

[Hans Verkuil]

New warnings appeared in the daily build after switching to gcc 10.2.0.
These patches fix those warnings.

Changes since v1: incorporated irc comments from Mauro

- tvp7002.c: rather than initializing val, just don't fill 'reg' on error.
- drxk_hard.c: the underlying cause is a wrong while condition, fix that.

	Hans

Hans Verkuil (3):
  s5k5baf: drop 'data' field in struct s5k5baf_fw
  tvp7002: fix uninitialized variable warning
  dvb-frontends/drxk_hard.c: fix uninitialized variable warning

 drivers/media/dvb-frontends/drxk_hard.c | 3 +--
 drivers/media/i2c/s5k5baf.c             | 5 ++---
 drivers/media/i2c/tvp7002.c             | 4 +++-
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.28.0

[PATCHv2 1/3] s5k5baf: drop 'data' field in struct s5k5baf_fw

[Hans Verkuil]

struct s5k5baf_fw ends with this:

       struct {
               u16 id;
               u16 offset;
       } seq[0];
       u16 data[];
};

which is rather confusing and can cause gcc warnings:

s5k5baf.c: In function 's5k5baf_load_setfile.isra':
s5k5baf.c:390:13: warning: array subscript 65535 is outside the bounds of an interior zero-length array 'struct <anonymous>[0]' [-Wzero-length-bounds]
  390 |   if (f->seq[i].offset + d <= end)
      |       ~~~~~~^~~

It turns out that 'data[]' is used in only one place and it can
easily be replaced by &fw->seq[0].id and 'seq[0]' can be replaced by
'seq[]'.

This is both more readable and solved that warnings.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
---
 drivers/media/i2c/s5k5baf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index 42584a088273..ec6f22efe19a 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -280,8 +280,7 @@ struct s5k5baf_fw {
 	struct {
 		u16 id;
 		u16 offset;
-	} seq[0];
-	u16 data[];
+	} seq[];
 };
 
 struct s5k5baf {
@@ -563,7 +562,7 @@ static u16 *s5k5baf_fw_get_seq(struct s5k5baf *state, u16 seq_id)
 	if (fw == NULL)
 		return NULL;
 
-	data = fw->data + 2 * fw->count;
+	data = &fw->seq[0].id + 2 * fw->count;
 
 	for (i = 0; i < fw->count; ++i) {
 		if (fw->seq[i].id == seq_id)
-- 
2.28.0

[PATCHv2 2/3] tvp7002: fix uninitialized variable warning

[Hans Verkuil]

tvp7002.c: In function 'tvp7002_g_register':
tvp7002.c:691:11: warning: 'val' may be used uninitialized in this function [-Wmaybe-uninitialized]
  691 |  reg->val = val;
      |  ~~~~~~~~~^~~~~

Just return without setting 'reg' if tvp7002_read returns an error.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/i2c/tvp7002.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index de313b1306da..ada4ec5ef782 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -688,9 +688,11 @@ static int tvp7002_g_register(struct v4l2_subdev *sd,
 	int ret;
 
 	ret = tvp7002_read(sd, reg->reg & 0xff, &val);
+	if (ret < 0)
+		return ret;
 	reg->val = val;
 	reg->size = 1;
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.28.0

[PATCHv2 3/3] dvb-frontends/drxk_hard.c: fix uninitialized variable warning

[Hans Verkuil]

drxk_hard.c: In function 'hi_command.constprop':
drxk_hard.c:1016:5: warning: 'wait_cmd' may be used uninitialized in this function [-Wmaybe-uninitialized]
 1015 |   } while ((status < 0) && (retry_count < DRXK_MAX_RETRIES)
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1016 |     && (wait_cmd != 0));
      |     ^~~~~~~~~~~~~~~~~~

The underlying cause is that the while condition is wrong. It should be:

(status < 0 || wait_cmd) && (retry_count < DRXK_MAX_RETRIES)

'wait_cmd' is only valid if '!(status < 0)'.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/dvb-frontends/drxk_hard.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
index 32f9346deb3e..a57470bf71bf 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -1011,8 +1011,7 @@ static int hi_command(struct drxk_state *state, u16 cmd, u16 *p_result)
 			retry_count += 1;
 			status = read16(state, SIO_HI_RA_RAM_CMD__A,
 					  &wait_cmd);
-		} while ((status < 0) && (retry_count < DRXK_MAX_RETRIES)
-			 && (wait_cmd != 0));
+		} while ((status < 0 || wait_cmd) && (retry_count < DRXK_MAX_RETRIES));
 		if (status < 0)
 			goto error;
 		status = read16(state, SIO_HI_RA_RAM_RES__A, p_result);
-- 
2.28.0

Re: [PATCHv2 1/3] s5k5baf: drop 'data' field in struct s5k5baf_fw

[Philipp Zabel]

On Thu, 2020-10-08 at 13:59 +0200, Hans Verkuil wrote:
> struct s5k5baf_fw ends with this:
> 
>        struct {
>                u16 id;
>                u16 offset;
>        } seq[0];
>        u16 data[];
> };
> 
> which is rather confusing and can cause gcc warnings:
> 
> s5k5baf.c: In function 's5k5baf_load_setfile.isra':
> s5k5baf.c:390:13: warning: array subscript 65535 is outside the bounds of an interior zero-length array 'struct <anonymous>[0]' [-Wzero-length-bounds]
>   390 |   if (f->seq[i].offset + d <= end)
>       |       ~~~~~~^~~
> 
> It turns out that 'data[]' is used in only one place and it can
> easily be replaced by &fw->seq[0].id and 'seq[0]' can be replaced by
> 'seq[]'.
> 
> This is both more readable and solved that warnings.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> ---
>  drivers/media/i2c/s5k5baf.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> index 42584a088273..ec6f22efe19a 100644
> --- a/drivers/media/i2c/s5k5baf.c
> +++ b/drivers/media/i2c/s5k5baf.c
> @@ -280,8 +280,7 @@ struct s5k5baf_fw {
>  	struct {
>  		u16 id;
>  		u16 offset;
> -	} seq[0];
> -	u16 data[];
> +	} seq[];
>  };
>  
>  struct s5k5baf {
> @@ -563,7 +562,7 @@ static u16 *s5k5baf_fw_get_seq(struct s5k5baf *state, u16 seq_id)
>  	if (fw == NULL)
>  		return NULL;
>  
> -	data = fw->data + 2 * fw->count;
> +	data = &fw->seq[0].id + 2 * fw->count;

Would it make sense to make this

+	data = &fw->seq[fw->count];

or

+	data = fw->seq + fw->count;

instead?

regards
Philipp

Re: [PATCHv2 1/3] s5k5baf: drop 'data' field in struct s5k5baf_fw

[Philipp Zabel]

On Thu, 2020-10-08 at 14:58 +0200, Philipp Zabel wrote:
> On Thu, 2020-10-08 at 13:59 +0200, Hans Verkuil wrote:
> > struct s5k5baf_fw ends with this:
> > 
> >        struct {
> >                u16 id;
> >                u16 offset;
> >        } seq[0];
> >        u16 data[];
> > };
> > 
> > which is rather confusing and can cause gcc warnings:
> > 
> > s5k5baf.c: In function 's5k5baf_load_setfile.isra':
> > s5k5baf.c:390:13: warning: array subscript 65535 is outside the bounds of an interior zero-length array 'struct <anonymous>[0]' [-Wzero-length-bounds]
> >   390 |   if (f->seq[i].offset + d <= end)
> >       |       ~~~~~~^~~
> > 
> > It turns out that 'data[]' is used in only one place and it can
> > easily be replaced by &fw->seq[0].id and 'seq[0]' can be replaced by
> > 'seq[]'.
> > 
> > This is both more readable and solved that warnings.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Sylwester Nawrocki <snawrocki@kernel.org>
> > ---
> >  drivers/media/i2c/s5k5baf.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> > index 42584a088273..ec6f22efe19a 100644
> > --- a/drivers/media/i2c/s5k5baf.c
> > +++ b/drivers/media/i2c/s5k5baf.c
> > @@ -280,8 +280,7 @@ struct s5k5baf_fw {
> >  	struct {
> >  		u16 id;
> >  		u16 offset;
> > -	} seq[0];
> > -	u16 data[];
> > +	} seq[];
> >  };
> >  
> >  struct s5k5baf {
> > @@ -563,7 +562,7 @@ static u16 *s5k5baf_fw_get_seq(struct s5k5baf *state, u16 seq_id)
> >  	if (fw == NULL)
> >  		return NULL;
> >  
> > -	data = fw->data + 2 * fw->count;
> > +	data = &fw->seq[0].id + 2 * fw->count;
> 
> Would it make sense to make this
> 
> +	data = &fw->seq[fw->count];
> 
> or
> 
> +	data = fw->seq + fw->count;
>
> instead?

Sorry for the noise, I missed that data is of type (u16 *).

regards
Philipp