[PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Mauro Carvalho Chehab]

As such macro will check if the expression is true, it may fall through, as
warned:

In file included from ./include/uapi/linux/stddef.h:1:0,
                 from ./include/linux/stddef.h:4,
                 from ./include/uapi/linux/posix_types.h:4,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/types.h:5,
                 from ./drivers/media/dvb-core/dvb_frontend.h:35,
                 from drivers/media/tuners/fc0011.h:4,
                 from drivers/media/tuners/fc0011.c:20:
drivers/media/tuners/fc0011.c: In function 'fc0011_set_params':
./include/linux/compiler.h:179:22: warning: this statement may fall through [-Wimplicit-fallthrough=]
 # define unlikely(x) __builtin_expect(!!(x), 0)
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/asm-generic/bug.h:109:2: note: in expansion of macro 'unlikely'
  unlikely(__ret_warn_on);     \
  ^~~~~~~~
drivers/media/tuners/fc0011.c:344:3: note: in expansion of macro 'WARN_ON'
   WARN_ON(1);
   ^~~~~~~
drivers/media/tuners/fc0011.c:345:2: note: here
  case 0:
  ^~~~
In file included from ./include/uapi/linux/stddef.h:1:0,
                 from ./include/linux/stddef.h:4,
                 from ./include/uapi/linux/posix_types.h:4,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/types.h:5,
                 from ./include/linux/list.h:4,
                 from ./include/linux/module.h:9,
                 from drivers/media/pci/tw5864/tw5864-video.c:17:
drivers/media/pci/tw5864/tw5864-video.c: In function 'tw5864_fmt_vid_cap':
./include/linux/compiler.h:179:22: warning: this statement may fall through [-Wimplicit-fallthrough=]
 # define unlikely(x) __builtin_expect(!!(x), 0)
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
./include/asm-generic/bug.h:68:2: note: in expansion of macro 'unlikely'
  unlikely(__ret_warn_on);    \
  ^~~~~~~~
drivers/media/pci/tw5864/tw5864-video.c:547:3: note: in expansion of macro 'WARN_ON_ONCE'
   WARN_ON_ONCE(1);
   ^~~~~~~~~~~~
drivers/media/pci/tw5864/tw5864-video.c:548:2: note: here
  case STD_NTSC:
  ^~~~

On both cases, it means an error, so, let's return an error
code, to make gcc happy.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/tw5864/tw5864-video.c | 1 +
 drivers/media/tuners/fc0011.c           | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
index 2a044be729da..e7bd2b8484e3 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void *priv,
 	switch (input->std) {
 	default:
 		WARN_ON_ONCE(1);
+		return -EINVAL;
 	case STD_NTSC:
 		f->fmt.pix.height = 480;
 		break;
diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
index 192b1c7740df..145407dee3db 100644
--- a/drivers/media/tuners/fc0011.c
+++ b/drivers/media/tuners/fc0011.c
@@ -342,6 +342,7 @@ static int fc0011_set_params(struct dvb_frontend *fe)
 	switch (vco_sel) {
 	default:
 		WARN_ON(1);
+		return -EINVAL;
 	case 0:
 		if (vco_cal < 8) {
 			regs[FC11_REG_VCOSEL] &= ~(FC11_VCOSEL_1 | FC11_VCOSEL_2);
-- 
2.9.3

[PATCH 2/4] [media] imon: better code the pad_mouse toggling

[Mauro Carvalho Chehab]

The logic with toggles the pad_mouse is confusing. Now, gcc 7.1
complains about it:

drivers/media/rc/imon.c: In function 'imon_incoming_scancode':
drivers/media/rc/imon.c:1725:23: warning: '~' on a boolean expression [-Wbool-operation]
    ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
                       ^
drivers/media/rc/imon.c:1725:23: note: did you mean to use logical not?
    ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
                       ^
                       !

Rewrite it to be clearer for both code reviewers and gcc.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/rc/imon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 3489010601b5..9c510fe54b2a 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1722,7 +1722,7 @@ static void imon_incoming_scancode(struct imon_context *ictx,
 	if (kc == KEY_KEYBOARD && !ictx->release_code) {
 		ictx->last_keycode = kc;
 		if (!nomouse) {
-			ictx->pad_mouse = ~(ictx->pad_mouse) & 0x1;
+			ictx->pad_mouse = !(ictx->pad_mouse & 0x1);
 			dev_dbg(dev, "toggling to %s mode\n",
 				ictx->pad_mouse ? "mouse" : "keyboard");
 			spin_unlock_irqrestore(&ictx->kc_lock, flags);
-- 
2.9.3

[PATCH 3/4] [media] s5p-jpeg: don't return a random width/height

[Mauro Carvalho Chehab]

Gcc 7.1 complains about:

drivers/media/platform/s5p-jpeg/jpeg-core.c: In function 's5p_jpeg_parse_hdr.isra.9':
drivers/media/platform/s5p-jpeg/jpeg-core.c:1207:12: warning: 'width' may be used uninitialized in this function [-Wmaybe-uninitialized]
  result->w = width;
  ~~~~~~~~~~^~~~~~~
drivers/media/platform/s5p-jpeg/jpeg-core.c:1208:12: warning: 'height' may be used uninitialized in this function [-Wmaybe-uninitialized]
  result->h = height;
  ~~~~~~~~~~^~~~~~~~

Indeed the code would allow it to return a random value (although
it shouldn't happen, in practice). So, explicitly set both to zero,
just in case.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 52dc7941db65..1da2c94e1dca 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1099,10 +1099,10 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
 			       struct s5p_jpeg_ctx *ctx)
 {
 	int c, components = 0, notfound, n_dht = 0, n_dqt = 0;
-	unsigned int height, width, word, subsampling = 0, sos = 0, sof = 0,
-		     sof_len = 0;
-	unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER],
-		     dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
+	unsigned int height = 0, width = 0, word, subsampling = 0;
+	unsigned int sos = 0, sof = 0, sof_len = 0;
+	unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER];
+	unsigned int dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
 	long length;
 	struct s5p_jpeg_buffer jpeg_buffer;
 
-- 
2.9.3

[PATCH 4/4] [media] mtk_vcodec_dec: return error at mtk_vdec_pic_info_update()

[Mauro Carvalho Chehab]

Gcc 7.1 complains that:

	drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c: In function 'mtk_vdec_pic_info_update':
	drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c:284:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
	  int ret;
	      ^~~

Indeed, if debug is disabled, "ret" is never used. The best
fix for it seems to make the fuction to return an error code.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index a60b538686ea..843510979ad8 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -278,7 +278,7 @@ static void mtk_vdec_flush_decoder(struct mtk_vcodec_ctx *ctx)
 	clean_free_buffer(ctx);
 }
 
-static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
+static int mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
 {
 	unsigned int dpbsize = 0;
 	int ret;
@@ -288,7 +288,7 @@ static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
 				&ctx->last_decoded_picinfo)) {
 		mtk_v4l2_err("[%d]Error!! Cannot get param : GET_PARAM_PICTURE_INFO ERR",
 				ctx->id);
-		return;
+		return -EINVAL;
 	}
 
 	if (ctx->last_decoded_picinfo.pic_w == 0 ||
@@ -296,12 +296,12 @@ static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
 		ctx->last_decoded_picinfo.buf_w == 0 ||
 		ctx->last_decoded_picinfo.buf_h == 0) {
 		mtk_v4l2_err("Cannot get correct pic info");
-		return;
+		return -EINVAL;
 	}
 
 	if ((ctx->last_decoded_picinfo.pic_w == ctx->picinfo.pic_w) ||
 	    (ctx->last_decoded_picinfo.pic_h == ctx->picinfo.pic_h))
-		return;
+		return 0;
 
 	mtk_v4l2_debug(1,
 			"[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
@@ -316,6 +316,8 @@ static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
 		mtk_v4l2_err("Incorrect dpb size, ret=%d", ret);
 
 	ctx->dpb_size = dpbsize;
+
+	return ret;
 }
 
 static void mtk_vdec_worker(struct work_struct *work)
-- 
2.9.3

Re: [PATCH 2/4] [media] imon: better code the pad_mouse toggling

[Sean Young]

On Thu, May 18, 2017 at 11:06:44AM -0300, Mauro Carvalho Chehab wrote:
> The logic with toggles the pad_mouse is confusing. Now, gcc 7.1
> complains about it:
> 
> drivers/media/rc/imon.c: In function 'imon_incoming_scancode':
> drivers/media/rc/imon.c:1725:23: warning: '~' on a boolean expression [-Wbool-operation]
>     ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
>                        ^
> drivers/media/rc/imon.c:1725:23: note: did you mean to use logical not?
>     ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
>                        ^
>                        !
> 
> Rewrite it to be clearer for both code reviewers and gcc.

This was already spotted by Arnd.

https://patchwork.linuxtv.org/patch/41270/

Thanks
Sean

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Andrey Utkin]

On Thu, May 18, 2017 at 11:06:43AM -0300, Mauro Carvalho Chehab wrote:
> As such macro will check if the expression is true, it may fall through, as
> warned:

> On both cases, it means an error, so, let's return an error
> code, to make gcc happy.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/pci/tw5864/tw5864-video.c | 1 +
>  drivers/media/tuners/fc0011.c           | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
> index 2a044be729da..e7bd2b8484e3 100644
> --- a/drivers/media/pci/tw5864/tw5864-video.c
> +++ b/drivers/media/pci/tw5864/tw5864-video.c
> @@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void *priv,
>  	switch (input->std) {
>  	default:
>  		WARN_ON_ONCE(1);
> +		return -EINVAL;
>  	case STD_NTSC:
>  		f->fmt.pix.height = 480;
>  		break;

Hi Mauro,

Thanks for the patch.

I actually meant it to fall through, but I agree this is not how it
should be.
I'm fine with this patch. Unfortunately I don't possess tw5864 hardware
now. CCing Anton Sviridenko whom I've handed it (I guess he's on
Bluecherry Maintainers groupmail as well).

Anton, just in case, could you please ensure the driver with this patch
works well in runtime?

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Michael Büsch]

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

On Thu, 18 May 2017 11:06:43 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> wrote:

> diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
> index 192b1c7740df..145407dee3db 100644
> --- a/drivers/media/tuners/fc0011.c
> +++ b/drivers/media/tuners/fc0011.c
> @@ -342,6 +342,7 @@ static int fc0011_set_params(struct dvb_frontend *fe)
>  	switch (vco_sel) {
>  	default:
>  		WARN_ON(1);
> +		return -EINVAL;
>  	case 0:
>  		if (vco_cal < 8) {
>  			regs[FC11_REG_VCOSEL] &= ~(FC11_VCOSEL_1 | FC11_VCOSEL_2);

This fall through is intentional, but I guess returning an error is OK,
too. This should not happen anyway.
I cannot test this, though.

Acked-by: Michael Büsch <m@bues.ch>

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] [media] s5p-jpeg: don't return a random width/height

[Andrzej Pietrasiewicz]

W dniu 18.05.2017 o 16:06, Mauro Carvalho Chehab pisze:
> Gcc 7.1 complains about:
> 
> drivers/media/platform/s5p-jpeg/jpeg-core.c: In function 's5p_jpeg_parse_hdr.isra.9':
> drivers/media/platform/s5p-jpeg/jpeg-core.c:1207:12: warning: 'width' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    result->w = width;
>    ~~~~~~~~~~^~~~~~~
> drivers/media/platform/s5p-jpeg/jpeg-core.c:1208:12: warning: 'height' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    result->h = height;
>    ~~~~~~~~~~^~~~~~~~
> 
> Indeed the code would allow it to return a random value (although
> it shouldn't happen, in practice). So, explicitly set both to zero,
> just in case.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>


Acked-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com>



> ---
>   drivers/media/platform/s5p-jpeg/jpeg-core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 52dc7941db65..1da2c94e1dca 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -1099,10 +1099,10 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,
>   			       struct s5p_jpeg_ctx *ctx)
>   {
>   	int c, components = 0, notfound, n_dht = 0, n_dqt = 0;
> -	unsigned int height, width, word, subsampling = 0, sos = 0, sof = 0,
> -		     sof_len = 0;
> -	unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER],
> -		     dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
> +	unsigned int height = 0, width = 0, word, subsampling = 0;
> +	unsigned int sos = 0, sof = 0, sof_len = 0;
> +	unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER];
> +	unsigned int dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
>   	long length;
>   	struct s5p_jpeg_buffer jpeg_buffer;
>   
> 

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Anton Sviridenko]

ok, I'll check this patch on tw5864 hardware bit later  and report results here

On Thu, May 18, 2017 at 10:48 PM, Andrey Utkin
<andrey_utkin@fastmail.com> wrote:
> On Thu, May 18, 2017 at 11:06:43AM -0300, Mauro Carvalho Chehab wrote:
>> As such macro will check if the expression is true, it may fall through, as
>> warned:
>
>> On both cases, it means an error, so, let's return an error
>> code, to make gcc happy.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> ---
>>  drivers/media/pci/tw5864/tw5864-video.c | 1 +
>>  drivers/media/tuners/fc0011.c           | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
>> index 2a044be729da..e7bd2b8484e3 100644
>> --- a/drivers/media/pci/tw5864/tw5864-video.c
>> +++ b/drivers/media/pci/tw5864/tw5864-video.c
>> @@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void *priv,
>>       switch (input->std) {
>>       default:
>>               WARN_ON_ONCE(1);
>> +             return -EINVAL;
>>       case STD_NTSC:
>>               f->fmt.pix.height = 480;
>>               break;
>
> Hi Mauro,
>
> Thanks for the patch.
>
> I actually meant it to fall through, but I agree this is not how it
> should be.
> I'm fine with this patch. Unfortunately I don't possess tw5864 hardware
> now. CCing Anton Sviridenko whom I've handed it (I guess he's on
> Bluecherry Maintainers groupmail as well).
>
> Anton, just in case, could you please ensure the driver with this patch
> works well in runtime?