[PATCH 01/30] media: dvbdev: handle ENOMEM error at dvb_module_probe()

[PATCH 01/30] media: dvbdev: handle ENOMEM error at dvb_module_probe()

[Mauro Carvalho Chehab]

If allocation of struct board_info fails, return NULL from
dvb_module_probe().

Fix this warning:
	drivers/media/dvb-core/dvbdev.c:958 dvb_module_probe() error: potential null dereference 'board_info'.  (kzalloc returns null)

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvbdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index cf747d753a79..787fe06df217 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -953,6 +953,8 @@ struct i2c_client *dvb_module_probe(const char *module_name,
 	struct i2c_board_info *board_info;
 
 	board_info = kzalloc(sizeof(*board_info), GFP_KERNEL);
+	if (!board_info)
+		return NULL;
 
 	if (name)
 		strlcpy(board_info->type, name, I2C_NAME_SIZE);
-- 
2.14.3

[PATCH 02/30] media: imx-media-utils: fix a warning

[Mauro Carvalho Chehab]

The logic at find_format() is a little bit confusing even for
humans, and it tricks static code analyzers:

	drivers/staging/media/imx/imx-media-utils.c:259 find_format() error: buffer overflow 'array' 14 <= 20

Rewrite the logic in a way that it makes it clearer to understand,
while prevent static analyzers to produce false positives.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/staging/media/imx/imx-media-utils.c | 81 +++++++++++++++--------------
 1 file changed, 43 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 40bcb8fb18b9..fab98fc0d6a0 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -225,58 +225,63 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
 					      mbus->ycbcr_enc);
 }
 
+static const
+struct imx_media_pixfmt *__find_format(u32 fourcc,
+				       u32 code,
+				       bool allow_non_mbus,
+				       bool allow_bayer,
+				       const struct imx_media_pixfmt *array,
+				       u32 array_size)
+{
+	const struct imx_media_pixfmt *fmt;
+	int i, j;
+
+	for (i = 0; i < array_size; i++) {
+		fmt = &array[i];
+
+		if ((!allow_non_mbus && !fmt->codes[0]) ||
+		    (!allow_bayer && fmt->bayer))
+			continue;
+
+		if (fourcc && fmt->fourcc == fourcc)
+			return fmt;
+
+		if (!code)
+			continue;
+
+		for (j = 0; fmt->codes[j]; j++) {
+			if (code == fmt->codes[j])
+				return fmt;
+		}
+	}
+	return NULL;
+}
+
 static const struct imx_media_pixfmt *find_format(u32 fourcc,
 						  u32 code,
 						  enum codespace_sel cs_sel,
 						  bool allow_non_mbus,
 						  bool allow_bayer)
 {
-	const struct imx_media_pixfmt *array, *fmt, *ret = NULL;
-	u32 array_size;
-	int i, j;
+	const struct imx_media_pixfmt *ret;
 
 	switch (cs_sel) {
 	case CS_SEL_YUV:
-		array_size = NUM_YUV_FORMATS;
-		array = yuv_formats;
-		break;
+		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
+				     yuv_formats, NUM_YUV_FORMATS);
 	case CS_SEL_RGB:
-		array_size = NUM_RGB_FORMATS;
-		array = rgb_formats;
-		break;
+		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
+				     rgb_formats, NUM_RGB_FORMATS);
 	case CS_SEL_ANY:
-		array_size = NUM_YUV_FORMATS + NUM_RGB_FORMATS;
-		array = yuv_formats;
-		break;
+		ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
+				    yuv_formats, NUM_YUV_FORMATS);
+		if (ret)
+			return ret;
+		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
+				     rgb_formats, NUM_RGB_FORMATS);
 	default:
 		return NULL;
 	}
-
-	for (i = 0; i < array_size; i++) {
-		if (cs_sel == CS_SEL_ANY && i >= NUM_YUV_FORMATS)
-			fmt = &rgb_formats[i - NUM_YUV_FORMATS];
-		else
-			fmt = &array[i];
-
-		if ((!allow_non_mbus && fmt->codes[0] == 0) ||
-		    (!allow_bayer && fmt->bayer))
-			continue;
-
-		if (fourcc && fmt->fourcc == fourcc) {
-			ret = fmt;
-			goto out;
-		}
-
-		for (j = 0; code && fmt->codes[j]; j++) {
-			if (code == fmt->codes[j]) {
-				ret = fmt;
-				goto out;
-			}
-		}
-	}
-
-out:
-	return ret;
 }
 
 static int enum_format(u32 *fourcc, u32 *code, u32 index,
-- 
2.14.3

Re: [PATCH 02/30] media: imx-media-utils: fix a warning

[Dan Carpenter]

On Fri, Mar 23, 2018 at 07:56:48AM -0400, Mauro Carvalho Chehab wrote:
> The logic at find_format() is a little bit confusing even for
> humans, and it tricks static code analyzers:
> 
> 	drivers/staging/media/imx/imx-media-utils.c:259 find_format() error: buffer overflow 'array' 14 <= 20

It's always good to simplify the code, but I have a fix for this that I
will publish very soon.

regards,
dan carpenter

[PATCH 03/30] media: dvb_frontend: add proper __user annotations

[Mauro Carvalho Chehab]

Solves those warnings:
	drivers/media/dvb-core/dvb_frontend.c:2297:39: warning: incorrect type in argument 1 (different address spaces)
	drivers/media/dvb-core/dvb_frontend.c:2297:39:    expected void const [noderef] <asn:1>*<noident>
	drivers/media/dvb-core/dvb_frontend.c:2297:39:    got struct dtv_property *props
	drivers/media/dvb-core/dvb_frontend.c:2331:39: warning: incorrect type in argument 1 (different address spaces)
	drivers/media/dvb-core/dvb_frontend.c:2331:39:    expected void const [noderef] <asn:1>*<noident>
	drivers/media/dvb-core/dvb_frontend.c:2331:39:    got struct dtv_property *props

No functional changes.

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

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index a7ed16e0841d..21a7d4b47e1a 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2294,7 +2294,7 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
 			return -EINVAL;
 
-		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
+		tvp = memdup_user((void __user *)tvps->props, tvps->num * sizeof(*tvp));
 		if (IS_ERR(tvp))
 			return PTR_ERR(tvp);
 
@@ -2328,7 +2328,7 @@ static int dvb_frontend_handle_ioctl(struct file *file,
 		if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS))
 			return -EINVAL;
 
-		tvp = memdup_user(tvps->props, tvps->num * sizeof(*tvp));
+		tvp = memdup_user((void __user *)tvps->props, tvps->num * sizeof(*tvp));
 		if (IS_ERR(tvp))
 			return PTR_ERR(tvp);
 
-- 
2.14.3

[PATCH 04/30] media: vpss: fix annotations for vpss_regs_base2

[Mauro Carvalho Chehab]

Fix those warnings:

	drivers/media/platform/davinci/vpss.c:510:25: warning: incorrect type in argument 1 (different address spaces)
	drivers/media/platform/davinci/vpss.c:510:25:    expected void volatile [noderef] <asn:2>*addr
	drivers/media/platform/davinci/vpss.c:510:25:    got unsigned int [usertype] *static [toplevel] [assigned] vpss_regs_base2
	drivers/media/platform/davinci/vpss.c:520:34: warning: incorrect type in assignment (different address spaces)
	drivers/media/platform/davinci/vpss.c:520:34:    expected unsigned int [usertype] *static [toplevel] [assigned] vpss_regs_base2
	drivers/media/platform/davinci/vpss.c:520:34:    got void [noderef] <asn:2>*
	drivers/media/platform/davinci/vpss.c:522:54: warning: incorrect type in argument 2 (different address spaces)
	drivers/media/platform/davinci/vpss.c:522:54:    expected void volatile [noderef] <asn:2>*addr
	drivers/media/platform/davinci/vpss.c:522:54:    got unsigned int [usertype] *static [toplevel] [assigned] vpss_regs_base2

Weird enough, vpss_regs_base0 and vpss_regs_base1 were
properly annotated.

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

diff --git a/drivers/media/platform/davinci/vpss.c b/drivers/media/platform/davinci/vpss.c
index b73886519f4f..19cf6853411e 100644
--- a/drivers/media/platform/davinci/vpss.c
+++ b/drivers/media/platform/davinci/vpss.c
@@ -116,7 +116,7 @@ struct vpss_hw_ops {
 struct vpss_oper_config {
 	__iomem void *vpss_regs_base0;
 	__iomem void *vpss_regs_base1;
-	resource_size_t *vpss_regs_base2;
+	__iomem void *vpss_regs_base2;
 	enum vpss_platform_type platform;
 	spinlock_t vpss_lock;
 	struct vpss_hw_ops hw_ops;
-- 
2.14.3

[PATCH 05/30] media: rca: declare formats var as static

[Mauro Carvalho Chehab]

As warned:
	drivers/media/platform/rockchip/rga/rga.c:210:16: warning: symbol 'formats' was not declared. Should it be static?

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

diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 89296de9cf4a..d508a8ba6f89 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -207,7 +207,7 @@ static int rga_setup_ctrls(struct rga_ctx *ctx)
 	return 0;
 }
 
-struct rga_fmt formats[] = {
+static struct rga_fmt formats[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_ARGB32,
 		.color_swap = RGA_COLOR_RB_SWAP,
-- 
2.14.3

[PATCH 05/30] media: rca: declare formats var as static

[Mauro Carvalho Chehab]

As warned:
	drivers/media/platform/rockchip/rga/rga.c:210:16: warning: symbol 'formats' was not declared. Should it be static?

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

diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 89296de9cf4a..d508a8ba6f89 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -207,7 +207,7 @@ static int rga_setup_ctrls(struct rga_ctx *ctx)
 	return 0;
 }
 
-struct rga_fmt formats[] = {
+static struct rga_fmt formats[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_ARGB32,
 		.color_swap = RGA_COLOR_RB_SWAP,
-- 
2.14.3

[PATCH 05/30] media: rca: declare formats var as static

[Mauro Carvalho Chehab]

As warned:
	drivers/media/platform/rockchip/rga/rga.c:210:16: warning: symbol 'formats' was not declared. Should it be static?

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

diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 89296de9cf4a..d508a8ba6f89 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -207,7 +207,7 @@ static int rga_setup_ctrls(struct rga_ctx *ctx)
 	return 0;
 }
 
-struct rga_fmt formats[] = {
+static struct rga_fmt formats[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_ARGB32,
 		.color_swap = RGA_COLOR_RB_SWAP,
-- 
2.14.3

[PATCH 06/30] media: ov5670: get rid of a series of __be warnings

[Mauro Carvalho Chehab]

There are some troubles on this driver with respect to the usage
of __be16 and __b32 macros:

	drivers/media/i2c/ov5670.c:1857:27: warning: incorrect type in initializer (different base types)
	drivers/media/i2c/ov5670.c:1857:27:    expected unsigned short [unsigned] [usertype] reg_addr_be
	drivers/media/i2c/ov5670.c:1857:27:    got restricted __be16 [usertype] <noident>
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
	drivers/media/i2c/ov5670.c:1901:13: warning: incorrect type in assignment (different base types)
	drivers/media/i2c/ov5670.c:1901:13:    expected unsigned int [unsigned] [usertype] val
	drivers/media/i2c/ov5670.c:1901:13:    got restricted __be32 [usertype] <noident>

Fix them.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/ov5670.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 556a95c30781..d2db480da1b9 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1853,8 +1853,8 @@ static int ov5670_read_reg(struct ov5670 *ov5670, u16 reg, unsigned int len,
 	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	struct i2c_msg msgs[2];
 	u8 *data_be_p;
-	u32 data_be = 0;
-	u16 reg_addr_be = cpu_to_be16(reg);
+	__be32 data_be = 0;
+	__be16 reg_addr_be = cpu_to_be16(reg);
 	int ret;
 
 	if (len > 4)
@@ -1891,6 +1891,7 @@ static int ov5670_write_reg(struct ov5670 *ov5670, u16 reg, unsigned int len,
 	int val_i;
 	u8 buf[6];
 	u8 *val_p;
+	__be32 tmp;
 
 	if (len > 4)
 		return -EINVAL;
@@ -1898,8 +1899,8 @@ static int ov5670_write_reg(struct ov5670 *ov5670, u16 reg, unsigned int len,
 	buf[0] = reg >> 8;
 	buf[1] = reg & 0xff;
 
-	val = cpu_to_be32(val);
-	val_p = (u8 *)&val;
+	tmp = cpu_to_be32(val);
+	val_p = (u8 *)&tmp;
 	buf_i = 2;
 	val_i = 4 - len;
 
-- 
2.14.3

Re: [PATCH 06/30] media: ov5670: get rid of a series of __be warnings

[Sakari Ailus]

On Fri, Mar 23, 2018 at 07:56:52AM -0400, Mauro Carvalho Chehab wrote:
> There are some troubles on this driver with respect to the usage
> of __be16 and __b32 macros:
> 
> 	drivers/media/i2c/ov5670.c:1857:27: warning: incorrect type in initializer (different base types)
> 	drivers/media/i2c/ov5670.c:1857:27:    expected unsigned short [unsigned] [usertype] reg_addr_be
> 	drivers/media/i2c/ov5670.c:1857:27:    got restricted __be16 [usertype] <noident>
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1880:16: warning: cast to restricted __be32
> 	drivers/media/i2c/ov5670.c:1901:13: warning: incorrect type in assignment (different base types)
> 	drivers/media/i2c/ov5670.c:1901:13:    expected unsigned int [unsigned] [usertype] val
> 	drivers/media/i2c/ov5670.c:1901:13:    got restricted __be32 [usertype] <noident>
> 
> Fix them.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH 07/30] media: v4l2-tpg-core: avoid buffer overflows

[Mauro Carvalho Chehab]

Fix the following warnings:
	drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:1146 gen_twopix() error: buffer overflow 'buf[1]' 8 <= 8
	drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:1152 gen_twopix() error: buffer overflow 'buf[1]' 8 <= 8

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

diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
index d248d1fb9d1d..37632bc524d4 100644
--- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
+++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
@@ -1143,13 +1143,13 @@ static void gen_twopix(struct tpg_data *tpg,
 	case V4L2_PIX_FMT_NV24:
 		buf[0][offset] = r_y_h;
 		buf[1][2 * offset] = g_u_s;
-		buf[1][2 * offset + 1] = b_v;
+		buf[1][(2 * offset + 1) % 8] = b_v;
 		break;
 
 	case V4L2_PIX_FMT_NV42:
 		buf[0][offset] = r_y_h;
 		buf[1][2 * offset] = b_v;
-		buf[1][2 * offset + 1] = g_u_s;
+		buf[1][(2 * offset + 1) %8] = g_u_s;
 		break;
 
 	case V4L2_PIX_FMT_YUYV:
-- 
2.14.3

Re: [PATCH 07/30] media: v4l2-tpg-core: avoid buffer overflows

[Hans Verkuil]

On 03/23/18 12:56, Mauro Carvalho Chehab wrote:
> Fix the following warnings:
> 	drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:1146 gen_twopix() error: buffer overflow 'buf[1]' 8 <= 8
> 	drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:1152 gen_twopix() error: buffer overflow 'buf[1]' 8 <= 8
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> index d248d1fb9d1d..37632bc524d4 100644
> --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> @@ -1143,13 +1143,13 @@ static void gen_twopix(struct tpg_data *tpg,
>  	case V4L2_PIX_FMT_NV24:
>  		buf[0][offset] = r_y_h;
>  		buf[1][2 * offset] = g_u_s;
> -		buf[1][2 * offset + 1] = b_v;
> +		buf[1][(2 * offset + 1) % 8] = b_v;
>  		break;
>  
>  	case V4L2_PIX_FMT_NV42:
>  		buf[0][offset] = r_y_h;
>  		buf[1][2 * offset] = b_v;
> -		buf[1][2 * offset + 1] = g_u_s;
> +		buf[1][(2 * offset + 1) %8] = g_u_s;

Space after '%'

>  		break;
>  
>  	case V4L2_PIX_FMT_YUYV:
> 

Nice! I always wondered how to fix this bogus error, but this will do it.

After fixing the space:

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks,

	Hans

[PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Mauro Carvalho Chehab]

While the code there is right, it produces three false positives:
	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error: memset() 'parg' too small (128 vs 16383)

Store the ioctl size on a cache var, in order to suppress those.

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

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 672ab22ccd96..a5dab16ff2d2 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2833,14 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	size_t  array_size = 0;
 	void __user *user_ptr = NULL;
 	void	**kernel_ptr = NULL;
+	size_t	size = _IOC_SIZE(cmd);
 
 	/*  Copy arguments into temp kernel buffer  */
 	if (_IOC_DIR(cmd) != _IOC_NONE) {
-		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
+		if (size <= sizeof(sbuf)) {
 			parg = sbuf;
 		} else {
 			/* too big to allocate from stack */
-			mbuf = kvmalloc(_IOC_SIZE(cmd), GFP_KERNEL);
+			mbuf = kvmalloc(size, GFP_KERNEL);
 			if (NULL == mbuf)
 				return -ENOMEM;
 			parg = mbuf;
@@ -2848,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 
 		err = -EFAULT;
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
-			unsigned int n = _IOC_SIZE(cmd);
+			unsigned int n = size;
 
 			/*
 			 * In some cases, only a few fields are used as input,
@@ -2869,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 				goto out;
 
 			/* zero out anything we don't copy from userspace */
-			if (n < _IOC_SIZE(cmd))
-				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
+			if (n < size)
+				memset((u8 *)parg + n, 0, size - n);
 		} else {
 			/* read-only ioctl */
-			memset(parg, 0, _IOC_SIZE(cmd));
+			memset(parg, 0, size);
 		}
 	}
 
@@ -2931,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	switch (_IOC_DIR(cmd)) {
 	case _IOC_READ:
 	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
+		if (copy_to_user((void __user *)arg, parg, size))
 			err = -EFAULT;
 		break;
 	}
-- 
2.14.3

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Sakari Ailus]

Hi Mauro,

On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> While the code there is right, it produces three false positives:
> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error: memset() 'parg' too small (128 vs 16383)
> 
> Store the ioctl size on a cache var, in order to suppress those.

I have to say I'm not a big fan of changing perfectly fine code in order to
please static analysers. What's this, smatch? I wonder if it could be fixed
instead of changing the code. That'd be presumably a lot more work though.

On naming --- "size" is rather more generic, but it's not a long function
either. I'd be a bit more specific, e.g. ioc_size or arg_size.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 672ab22ccd96..a5dab16ff2d2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2833,14 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  	size_t  array_size = 0;
>  	void __user *user_ptr = NULL;
>  	void	**kernel_ptr = NULL;
> +	size_t	size = _IOC_SIZE(cmd);
>  
>  	/*  Copy arguments into temp kernel buffer  */
>  	if (_IOC_DIR(cmd) != _IOC_NONE) {
> -		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
> +		if (size <= sizeof(sbuf)) {
>  			parg = sbuf;
>  		} else {
>  			/* too big to allocate from stack */
> -			mbuf = kvmalloc(_IOC_SIZE(cmd), GFP_KERNEL);
> +			mbuf = kvmalloc(size, GFP_KERNEL);
>  			if (NULL == mbuf)
>  				return -ENOMEM;
>  			parg = mbuf;
> @@ -2848,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  
>  		err = -EFAULT;
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> -			unsigned int n = _IOC_SIZE(cmd);
> +			unsigned int n = size;
>  
>  			/*
>  			 * In some cases, only a few fields are used as input,
> @@ -2869,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  				goto out;
>  
>  			/* zero out anything we don't copy from userspace */
> -			if (n < _IOC_SIZE(cmd))
> -				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
> +			if (n < size)
> +				memset((u8 *)parg + n, 0, size - n);
>  		} else {
>  			/* read-only ioctl */
> -			memset(parg, 0, _IOC_SIZE(cmd));
> +			memset(parg, 0, size);
>  		}
>  	}
>  
> @@ -2931,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  	switch (_IOC_DIR(cmd)) {
>  	case _IOC_READ:
>  	case (_IOC_WRITE | _IOC_READ):
> -		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> +		if (copy_to_user((void __user *)arg, parg, size))
>  			err = -EFAULT;
>  		break;
>  	}

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Mauro Carvalho Chehab]

Em Fri, 23 Mar 2018 23:53:56 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> > While the code there is right, it produces three false positives:
> > 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error: memset() 'parg' too small (128 vs 16383)
> > 
> > Store the ioctl size on a cache var, in order to suppress those.  
> 
> I have to say I'm not a big fan of changing perfectly fine code in order to
> please static analysers.

Well, there's a side effect of this patch: it allows gcc to better
optimize the text size, with is good:

   text	   data	    bss	    dec	    hex	filename
  34538	   2320	      0	  36858	   8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o
  34490	   2320	      0	  36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o

> What's this, smatch? I wonder if it could be fixed
> instead of changing the code. That'd be presumably a lot more work though.

Yes, the warnings came from smatch. No idea how easy/hard would be to
change it. 

> 
> On naming --- "size" is rather more generic, but it's not a long function
> either. I'd be a bit more specific, e.g. ioc_size or arg_size.

Agreed.

I'll add the enclosed patch changing it to ioc_size.


Thanks,
Mauro

[PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
    
Instead of just calling it as "size", let's name it as "ioc_size",
as it reflects better its contents.
    
As this is constant along the function, also mark it as const.
    
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index a5dab16ff2d2..f48c505550e0 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2833,15 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	size_t  array_size = 0;
 	void __user *user_ptr = NULL;
 	void	**kernel_ptr = NULL;
-	size_t	size = _IOC_SIZE(cmd);
+	const size_t ioc_size = _IOC_SIZE(cmd);
 
 	/*  Copy arguments into temp kernel buffer  */
 	if (_IOC_DIR(cmd) != _IOC_NONE) {
-		if (size <= sizeof(sbuf)) {
+		if (ioc_size <= sizeof(sbuf)) {
 			parg = sbuf;
 		} else {
 			/* too big to allocate from stack */
-			mbuf = kvmalloc(size, GFP_KERNEL);
+			mbuf = kvmalloc(ioc_size, GFP_KERNEL);
 			if (NULL == mbuf)
 				return -ENOMEM;
 			parg = mbuf;
@@ -2849,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 
 		err = -EFAULT;
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
-			unsigned int n = size;
+			unsigned int n = ioc_size;
 
 			/*
 			 * In some cases, only a few fields are used as input,
@@ -2870,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 				goto out;
 
 			/* zero out anything we don't copy from userspace */
-			if (n < size)
-				memset((u8 *)parg + n, 0, size - n);
+			if (n < ioc_size)
+				memset((u8 *)parg + n, 0, ioc_size - n);
 		} else {
 			/* read-only ioctl */
-			memset(parg, 0, size);
+			memset(parg, 0, ioc_size);
 		}
 	}
 
@@ -2932,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	switch (_IOC_DIR(cmd)) {
 	case _IOC_READ:
 	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void __user *)arg, parg, size))
+		if (copy_to_user((void __user *)arg, parg, ioc_size))
 			err = -EFAULT;
 		break;
 	}

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Sakari Ailus]

Hi Mauro,

On Mon, Mar 26, 2018 at 07:08:16AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 23 Mar 2018 23:53:56 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> > > While the code there is right, it produces three false positives:
> > > 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > > 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error: copy_from_user() 'parg' too small (128 vs 16383)
> > > 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error: memset() 'parg' too small (128 vs 16383)
> > > 
> > > Store the ioctl size on a cache var, in order to suppress those.  
> > 
> > I have to say I'm not a big fan of changing perfectly fine code in order to
> > please static analysers.
> 
> Well, there's a side effect of this patch: it allows gcc to better
> optimize the text size, with is good:
> 
>    text	   data	    bss	    dec	    hex	filename
>   34538	   2320	      0	  36858	   8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o
>   34490	   2320	      0	  36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o
> 
> > What's this, smatch? I wonder if it could be fixed
> > instead of changing the code. That'd be presumably a lot more work though.
> 
> Yes, the warnings came from smatch. No idea how easy/hard would be to
> change it. 
> 
> > 
> > On naming --- "size" is rather more generic, but it's not a long function
> > either. I'd be a bit more specific, e.g. ioc_size or arg_size.
> 
> Agreed.
> 
> I'll add the enclosed patch changing it to ioc_size.

Thanks. To the original + this change:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> 
> Thanks,
> Mauro
> 
> [PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
>     
> Instead of just calling it as "size", let's name it as "ioc_size",
> as it reflects better its contents.
>     
> As this is constant along the function, also mark it as const.
>     
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index a5dab16ff2d2..f48c505550e0 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2833,15 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  	size_t  array_size = 0;
>  	void __user *user_ptr = NULL;
>  	void	**kernel_ptr = NULL;
> -	size_t	size = _IOC_SIZE(cmd);
> +	const size_t ioc_size = _IOC_SIZE(cmd);
>  
>  	/*  Copy arguments into temp kernel buffer  */
>  	if (_IOC_DIR(cmd) != _IOC_NONE) {
> -		if (size <= sizeof(sbuf)) {
> +		if (ioc_size <= sizeof(sbuf)) {
>  			parg = sbuf;
>  		} else {
>  			/* too big to allocate from stack */
> -			mbuf = kvmalloc(size, GFP_KERNEL);
> +			mbuf = kvmalloc(ioc_size, GFP_KERNEL);
>  			if (NULL == mbuf)
>  				return -ENOMEM;
>  			parg = mbuf;
> @@ -2849,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  
>  		err = -EFAULT;
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> -			unsigned int n = size;
> +			unsigned int n = ioc_size;
>  
>  			/*
>  			 * In some cases, only a few fields are used as input,
> @@ -2870,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  				goto out;
>  
>  			/* zero out anything we don't copy from userspace */
> -			if (n < size)
> -				memset((u8 *)parg + n, 0, size - n);
> +			if (n < ioc_size)
> +				memset((u8 *)parg + n, 0, ioc_size - n);
>  		} else {
>  			/* read-only ioctl */
> -			memset(parg, 0, size);
> +			memset(parg, 0, ioc_size);
>  		}
>  	}
>  
> @@ -2932,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
>  	switch (_IOC_DIR(cmd)) {
>  	case _IOC_READ:
>  	case (_IOC_WRITE | _IOC_READ):
> -		if (copy_to_user((void __user *)arg, parg, size))
> +		if (copy_to_user((void __user *)arg, parg, ioc_size))
>  			err = -EFAULT;
>  		break;
>  	}
> 
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Laurent Pinchart]

Hi Mauro,

On Monday, 26 March 2018 13:08:16 EEST Mauro Carvalho Chehab wrote:
> Em Fri, 23 Mar 2018 23:53:56 +0200 Sakari Ailus escreveu:
> > On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> >> While the code there is right, it produces three false positives:
> >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> >> 	copy_from_user() 'parg' too small (128 vs 16383)
> >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> >> 	copy_from_user() 'parg' too small (128 vs 16383)
> >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error:
> >> 	memset() 'parg' too small (128 vs 16383)> > 
> >> Store the ioctl size on a cache var, in order to suppress those.
> > 
> > I have to say I'm not a big fan of changing perfectly fine code in order
> > to please static analysers.
> 
> Well, there's a side effect of this patch: it allows gcc to better
> optimize the text size, with is good:
> 
>    text	   data	    bss	    dec	    hex	filename
>   34538	   2320	      0	  36858	  
> 8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o 34490	   2320	      0	 
> 36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o
> > What's this, smatch? I wonder if it could be fixed
> > instead of changing the code. That'd be presumably a lot more work though.
> 
> Yes, the warnings came from smatch. No idea how easy/hard would be to
> change it.
> 
> > On naming --- "size" is rather more generic, but it's not a long function
> > either. I'd be a bit more specific, e.g. ioc_size or arg_size.
> 
> Agreed.
> 
> I'll add the enclosed patch changing it to ioc_size.
> 
> 
> Thanks,
> Mauro
> 
> [PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
> 
> Instead of just calling it as "size", let's name it as "ioc_size",
> as it reflects better its contents.
> 
> As this is constant along the function, also mark it as const.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

I would have expected a v2 of the original patch, but it seems you pushed it 
to the public master branch before giving anyone a change to review it 
(Sakari's review came 10h after the past was posted).

Patches need to be reviewed before being applied, and that applies to all 
patches, regarding of the author. Please refrain from merging future patches 
before they get reviewed.

> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index a5dab16ff2d2..f48c505550e0
> 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2833,15 +2833,15 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg, size_t  array_size = 0;
>  	void __user *user_ptr = NULL;
>  	void	**kernel_ptr = NULL;
> -	size_t	size = _IOC_SIZE(cmd);
> +	const size_t ioc_size = _IOC_SIZE(cmd);
> 
>  	/*  Copy arguments into temp kernel buffer  */
>  	if (_IOC_DIR(cmd) != _IOC_NONE) {
> -		if (size <= sizeof(sbuf)) {
> +		if (ioc_size <= sizeof(sbuf)) {
>  			parg = sbuf;
>  		} else {
>  			/* too big to allocate from stack */
> -			mbuf = kvmalloc(size, GFP_KERNEL);
> +			mbuf = kvmalloc(ioc_size, GFP_KERNEL);
>  			if (NULL == mbuf)
>  				return -ENOMEM;
>  			parg = mbuf;
> @@ -2849,7 +2849,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg,
> 
>  		err = -EFAULT;
>  		if (_IOC_DIR(cmd) & _IOC_WRITE) {
> -			unsigned int n = size;
> +			unsigned int n = ioc_size;
> 
>  			/*
>  			 * In some cases, only a few fields are used as input,
> @@ -2870,11 +2870,11 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg, goto out;
> 
>  			/* zero out anything we don't copy from userspace */
> -			if (n < size)
> -				memset((u8 *)parg + n, 0, size - n);
> +			if (n < ioc_size)
> +				memset((u8 *)parg + n, 0, ioc_size - n);
>  		} else {
>  			/* read-only ioctl */
> -			memset(parg, 0, size);
> +			memset(parg, 0, ioc_size);
>  		}
>  	}
> 
> @@ -2932,7 +2932,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg, switch (_IOC_DIR(cmd)) {
>  	case _IOC_READ:
>  	case (_IOC_WRITE | _IOC_READ):
> -		if (copy_to_user((void __user *)arg, parg, size))
> +		if (copy_to_user((void __user *)arg, parg, ioc_size))
>  			err = -EFAULT;
>  		break;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Mauro Carvalho Chehab]

Em Mon, 26 Mar 2018 21:47:33 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday, 26 March 2018 13:08:16 EEST Mauro Carvalho Chehab wrote:
> > Em Fri, 23 Mar 2018 23:53:56 +0200 Sakari Ailus escreveu:  
> > > On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:  
> > >> While the code there is right, it produces three false positives:
> > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error:
> > >> 	memset() 'parg' too small (128 vs 16383)> > 
> > >> Store the ioctl size on a cache var, in order to suppress those.  
> > > 
> > > I have to say I'm not a big fan of changing perfectly fine code in order
> > > to please static analysers.  
> > 
> > Well, there's a side effect of this patch: it allows gcc to better
> > optimize the text size, with is good:
> > 
> >    text	   data	    bss	    dec	    hex	filename
> >   34538	   2320	      0	  36858	  
> > 8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o 34490	   2320	      0	 
> > 36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o  
> > > What's this, smatch? I wonder if it could be fixed
> > > instead of changing the code. That'd be presumably a lot more work though.  
> > 
> > Yes, the warnings came from smatch. No idea how easy/hard would be to
> > change it.
> >   
> > > On naming --- "size" is rather more generic, but it's not a long function
> > > either. I'd be a bit more specific, e.g. ioc_size or arg_size.  
> > 
> > Agreed.
> > 
> > I'll add the enclosed patch changing it to ioc_size.
> > 
> > 
> > Thanks,
> > Mauro
> > 
> > [PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
> > 
> > Instead of just calling it as "size", let's name it as "ioc_size",
> > as it reflects better its contents.
> > 
> > As this is constant along the function, also mark it as const.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>  
> 
> I would have expected a v2 of the original patch, but it seems you pushed it 
> to the public master branch before giving anyone a change to review it 
> (Sakari's review came 10h after the past was posted).
> 
> Patches need to be reviewed before being applied, and that applies to all 
> patches, regarding of the author. Please refrain from merging future patches 
> before they get reviewed.

It shouldn't have pushed. It happened due to some script errors,
when I was handling a request from Hans to push upstream some fixes
for -rc7, as mentioned on IRC:
	https://linuxtv.org/irc/irclogger_log/v4l?date=2018-03-23,Fri

As I said there:

"If someone finds an issue on the warning fixes, I can revert or apply a fixup
 I really hate when things like that happens :-("

Thanks,
Mauro

Re: [PATCH 08/30] media: v4l2-ioctl: fix some "too small" warnings

[Laurent Pinchart]

Hi Mauro,

On Monday, 26 March 2018 23:28:55 EEST Mauro Carvalho Chehab wrote:
> Em Mon, 26 Mar 2018 21:47:33 +0300 Laurent Pinchart escreveu:
> > On Monday, 26 March 2018 13:08:16 EEST Mauro Carvalho Chehab wrote:
> > > Em Fri, 23 Mar 2018 23:53:56 +0200 Sakari Ailus escreveu:
> > > > On Fri, Mar 23, 2018 at 07:56:54AM -0400, Mauro Carvalho Chehab wrote:
> > > >> While the code there is right, it produces three false positives:
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2868 video_usercopy() error:
> > > >> 	copy_from_user() 'parg' too small (128 vs 16383)
> > > >> 	drivers/media/v4l2-core/v4l2-ioctl.c:2876 video_usercopy() error:
> > > >> 	memset() 'parg' too small (128 vs 16383)> >
> > > >> 
> > > >> Store the ioctl size on a cache var, in order to suppress those.
> > > > 
> > > > I have to say I'm not a big fan of changing perfectly fine code in
> > > > order
> > > > to please static analysers.
> > > 
> > > Well, there's a side effect of this patch: it allows gcc to better
> > > 
> > > optimize the text size, with is good:
> > >    text	   data	    bss	    dec	    hex	filename
> > >   
> > >   34538	   2320	      0	  36858
> > > 
> > > 8ffa	old/drivers/media/v4l2-core/v4l2-ioctl.o 34490	   2320	      0
> > > 36810	   8fca	new/drivers/media/v4l2-core/v4l2-ioctl.o
> > > 
> > > > What's this, smatch? I wonder if it could be fixed
> > > > instead of changing the code. That'd be presumably a lot more work
> > > > though.
> > > 
> > > Yes, the warnings came from smatch. No idea how easy/hard would be to
> > > change it.
> > > 
> > > > On naming --- "size" is rather more generic, but it's not a long
> > > > function
> > > > either. I'd be a bit more specific, e.g. ioc_size or arg_size.
> > > 
> > > Agreed.
> > > 
> > > I'll add the enclosed patch changing it to ioc_size.
> > > 
> > > 
> > > Thanks,
> > > Mauro
> > > 
> > > [PATCH] media: v4l2-ioctl: rename a temp var that stores _IOC_SIZE(cmd)
> > > 
> > > Instead of just calling it as "size", let's name it as "ioc_size",
> > > as it reflects better its contents.
> > > 
> > > As this is constant along the function, also mark it as const.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > 
> > I would have expected a v2 of the original patch, but it seems you pushed
> > it to the public master branch before giving anyone a change to review it
> > (Sakari's review came 10h after the past was posted).
> > 
> > Patches need to be reviewed before being applied, and that applies to all
> > patches, regarding of the author. Please refrain from merging future
> > patches before they get reviewed.
> 
> It shouldn't have pushed. It happened due to some script errors,
> when I was handling a request from Hans to push upstream some fixes
> for -rc7, as mentioned on IRC:
> 	https://linuxtv.org/irc/irclogger_log/v4l?date=2018-03-23,Fri

Thank you for the explanation. Bugs happen, luckily it should have less impact 
than spectre or meltdown :-)

> As I said there:
> 
> "If someone finds an issue on the warning fixes, I can revert or apply a
> fixup I really hate when things like that happens :-("

-- 
Regards,

Laurent Pinchart

[PATCH 09/30] media: sp887x: fix a warning

[Mauro Carvalho Chehab]

drivers/media/dvb-frontends/sp887x.c:179 sp887x_initial_setup() error: memcpy() '&buf[2]' too small (30 vs 16384)

This is actually a false alarm, but reverting the check order
makes not only for humans to review the code, but also cleans
the warning.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-frontends/sp887x.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb-frontends/sp887x.c b/drivers/media/dvb-frontends/sp887x.c
index 572a297811fe..f39d566d7d1d 100644
--- a/drivers/media/dvb-frontends/sp887x.c
+++ b/drivers/media/dvb-frontends/sp887x.c
@@ -136,7 +136,7 @@ static void sp887x_setup_agc (struct sp887x_state* state)
 static int sp887x_initial_setup (struct dvb_frontend* fe, const struct firmware *fw)
 {
 	struct sp887x_state* state = fe->demodulator_priv;
-	u8 buf [BLOCKSIZE+2];
+	u8 buf [BLOCKSIZE + 2];
 	int i;
 	int fw_size = fw->size;
 	const unsigned char *mem = fw->data;
@@ -144,7 +144,7 @@ static int sp887x_initial_setup (struct dvb_frontend* fe, const struct firmware
 	dprintk("%s\n", __func__);
 
 	/* ignore the first 10 bytes, then we expect 0x4000 bytes of firmware */
-	if (fw_size < FW_SIZE+10)
+	if (fw_size < FW_SIZE + 10)
 		return -ENODEV;
 
 	mem = fw->data + 10;
@@ -167,7 +167,7 @@ static int sp887x_initial_setup (struct dvb_frontend* fe, const struct firmware
 		int c = BLOCKSIZE;
 		int err;
 
-		if (i+c > FW_SIZE)
+		if (c > FW_SIZE - i)
 			c = FW_SIZE - i;
 
 		/* bit 0x8000 in address is set to enable 13bit mode */
-- 
2.14.3

[PATCH 10/30] media: tvaudio: improve error handling

[Mauro Carvalho Chehab]

The error handling logic at tvaudio is broken on several ways,
as it doesn't really check right when an error occurs.

Change it to return the proper error code from read/write
routines and fix the errors on reads.

Shuts up the following warnings:
	drivers/media/i2c/tvaudio.c:222 chip_read() error: uninitialized symbol 'buffer'.
	drivers/media/i2c/tvaudio.c:223 chip_read() error: uninitialized symbol 'buffer'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/tvaudio.c | 92 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/tvaudio.c b/drivers/media/i2c/tvaudio.c
index 772164b848ef..5919214a56bf 100644
--- a/drivers/media/i2c/tvaudio.c
+++ b/drivers/media/i2c/tvaudio.c
@@ -156,14 +156,18 @@ static int chip_write(struct CHIPSTATE *chip, int subaddr, int val)
 	struct v4l2_subdev *sd = &chip->sd;
 	struct i2c_client *c = v4l2_get_subdevdata(sd);
 	unsigned char buffer[2];
+	int rc;
 
 	if (subaddr < 0) {
 		v4l2_dbg(1, debug, sd, "chip_write: 0x%x\n", val);
 		chip->shadow.bytes[1] = val;
 		buffer[0] = val;
-		if (1 != i2c_master_send(c, buffer, 1)) {
+		rc = i2c_master_send(c, buffer, 1);
+		if (rc != 1) {
 			v4l2_warn(sd, "I/O error (write 0x%x)\n", val);
-			return -1;
+			if (rc < 0)
+				return rc;
+			return -EIO;
 		}
 	} else {
 		if (subaddr + 1 >= ARRAY_SIZE(chip->shadow.bytes)) {
@@ -178,10 +182,13 @@ static int chip_write(struct CHIPSTATE *chip, int subaddr, int val)
 		chip->shadow.bytes[subaddr+1] = val;
 		buffer[0] = subaddr;
 		buffer[1] = val;
-		if (2 != i2c_master_send(c, buffer, 2)) {
+		rc = i2c_master_send(c, buffer, 2);
+		if (rc != 2) {
 			v4l2_warn(sd, "I/O error (write reg%d=0x%x)\n",
 				subaddr, val);
-			return -1;
+			if (rc < 0)
+				return rc;
+			return -EIO;
 		}
 	}
 	return 0;
@@ -214,10 +221,14 @@ static int chip_read(struct CHIPSTATE *chip)
 	struct v4l2_subdev *sd = &chip->sd;
 	struct i2c_client *c = v4l2_get_subdevdata(sd);
 	unsigned char buffer;
+	int rc;
 
-	if (1 != i2c_master_recv(c, &buffer, 1)) {
+	rc = i2c_master_recv(c, &buffer, 1);
+	if (rc != 1) {
 		v4l2_warn(sd, "I/O error (read)\n");
-		return -1;
+		if (rc < 0)
+			return rc;
+		return -EIO;
 	}
 	v4l2_dbg(1, debug, sd, "chip_read: 0x%x\n", buffer);
 	return buffer;
@@ -227,6 +238,7 @@ static int chip_read2(struct CHIPSTATE *chip, int subaddr)
 {
 	struct v4l2_subdev *sd = &chip->sd;
 	struct i2c_client *c = v4l2_get_subdevdata(sd);
+	int rc;
 	unsigned char write[1];
 	unsigned char read[1];
 	struct i2c_msg msgs[2] = {
@@ -245,9 +257,12 @@ static int chip_read2(struct CHIPSTATE *chip, int subaddr)
 
 	write[0] = subaddr;
 
-	if (2 != i2c_transfer(c->adapter, msgs, 2)) {
+	rc = i2c_transfer(c->adapter, msgs, 2);
+	if (rc != 2) {
 		v4l2_warn(sd, "I/O error (read2)\n");
-		return -1;
+		if (rc < 0)
+			return rc;
+		return -EIO;
 	}
 	v4l2_dbg(1, debug, sd, "chip_read2: reg%d=0x%x\n",
 		subaddr, read[0]);
@@ -258,7 +273,7 @@ static int chip_cmd(struct CHIPSTATE *chip, char *name, audiocmd *cmd)
 {
 	struct v4l2_subdev *sd = &chip->sd;
 	struct i2c_client *c = v4l2_get_subdevdata(sd);
-	int i;
+	int i, rc;
 
 	if (0 == cmd->count)
 		return 0;
@@ -284,9 +299,12 @@ static int chip_cmd(struct CHIPSTATE *chip, char *name, audiocmd *cmd)
 		printk(KERN_CONT "\n");
 
 	/* send data to the chip */
-	if (cmd->count != i2c_master_send(c, cmd->bytes, cmd->count)) {
+	rc = i2c_master_send(c, cmd->bytes, cmd->count);
+	if (rc != cmd->count) {
 		v4l2_warn(sd, "I/O error (%s)\n", name);
-		return -1;
+		if (rc < 0)
+			return rc;
+		return -EIO;
 	}
 	return 0;
 }
@@ -400,8 +418,12 @@ static int tda9840_getrxsubchans(struct CHIPSTATE *chip)
 	struct v4l2_subdev *sd = &chip->sd;
 	int val, mode;
 
-	val = chip_read(chip);
 	mode = V4L2_TUNER_SUB_MONO;
+
+	val = chip_read(chip);
+	if (val < 0)
+		return mode;
+
 	if (val & TDA9840_DS_DUAL)
 		mode |= V4L2_TUNER_SUB_LANG1 | V4L2_TUNER_SUB_LANG2;
 	if (val & TDA9840_ST_STEREO)
@@ -445,7 +467,12 @@ static void tda9840_setaudmode(struct CHIPSTATE *chip, int mode)
 static int tda9840_checkit(struct CHIPSTATE *chip)
 {
 	int rc;
+
 	rc = chip_read(chip);
+	if (rc < 0)
+		return 0;
+
+
 	/* lower 5 bits should be 0 */
 	return ((rc & 0x1f) == 0) ? 1 : 0;
 }
@@ -563,6 +590,9 @@ static int  tda985x_getrxsubchans(struct CHIPSTATE *chip)
 	/* Allows forced mono */
 	mode = V4L2_TUNER_SUB_MONO;
 	val = chip_read(chip);
+	if (val < 0)
+		return mode;
+
 	if (val & TDA985x_STP)
 		mode = V4L2_TUNER_SUB_STEREO;
 	if (val & TDA985x_SAPP)
@@ -720,8 +750,12 @@ static int tda9873_getrxsubchans(struct CHIPSTATE *chip)
 	struct v4l2_subdev *sd = &chip->sd;
 	int val,mode;
 
-	val = chip_read(chip);
 	mode = V4L2_TUNER_SUB_MONO;
+
+	val = chip_read(chip);
+	if (val < 0)
+		return mode;
+
 	if (val & TDA9873_STEREO)
 		mode = V4L2_TUNER_SUB_STEREO;
 	if (val & TDA9873_DUAL)
@@ -780,7 +814,8 @@ static int tda9873_checkit(struct CHIPSTATE *chip)
 {
 	int rc;
 
-	if (-1 == (rc = chip_read2(chip,254)))
+	rc = chip_read2(chip, 254);
+	if (rc < 0)
 		return 0;
 	return (rc & ~0x1f) == 0x80;
 }
@@ -926,11 +961,14 @@ static int tda9874a_getrxsubchans(struct CHIPSTATE *chip)
 
 	mode = V4L2_TUNER_SUB_MONO;
 
-	if(-1 == (dsr = chip_read2(chip,TDA9874A_DSR)))
+	dsr = chip_read2(chip, TDA9874A_DSR);
+	if (dsr < 0)
 		return mode;
-	if(-1 == (nsr = chip_read2(chip,TDA9874A_NSR)))
+	nsr = chip_read2(chip, TDA9874A_NSR);
+	if (nsr < 0)
 		return mode;
-	if(-1 == (necr = chip_read2(chip,TDA9874A_NECR)))
+	necr = chip_read2(chip, TDA9874A_NECR);
+	if (necr < 0)
 		return mode;
 
 	/* need to store dsr/nsr somewhere */
@@ -1059,9 +1097,11 @@ static int tda9874a_checkit(struct CHIPSTATE *chip)
 	struct v4l2_subdev *sd = &chip->sd;
 	int dic,sic;	/* device id. and software id. codes */
 
-	if(-1 == (dic = chip_read2(chip,TDA9874A_DIC)))
+	dic = chip_read2(chip, TDA9874A_DIC);
+	if (dic < 0)
 		return 0;
-	if(-1 == (sic = chip_read2(chip,TDA9874A_SIC)))
+	sic = chip_read2(chip, TDA9874A_SIC);
+	if (sic < 0)
 		return 0;
 
 	v4l2_dbg(1, debug, sd, "tda9874a_checkit(): DIC=0x%X, SIC=0x%X.\n", dic, sic);
@@ -1201,7 +1241,11 @@ static int tda9875_checkit(struct CHIPSTATE *chip)
 	int dic, rev;
 
 	dic = chip_read2(chip, 254);
+	if (dic < 0)
+		return 0;
 	rev = chip_read2(chip, 255);
+	if (rev < 0)
+		return 0;
 
 	if (dic == 0 || dic == 2) { /* tda9875 and tda9875A */
 		v4l2_info(sd, "found tda9875%s rev. %d.\n",
@@ -1377,8 +1421,12 @@ static int ta8874z_getrxsubchans(struct CHIPSTATE *chip)
 {
 	int val, mode;
 
-	val = chip_read(chip);
 	mode = V4L2_TUNER_SUB_MONO;
+
+	val = chip_read(chip);
+	if (val < 0)
+		return mode;
+
 	if (val & TA8874Z_B1){
 		mode |= V4L2_TUNER_SUB_LANG1 | V4L2_TUNER_SUB_LANG2;
 	}else if (!(val & TA8874Z_B0)){
@@ -1431,7 +1479,11 @@ static void ta8874z_setaudmode(struct CHIPSTATE *chip, int mode)
 static int ta8874z_checkit(struct CHIPSTATE *chip)
 {
 	int rc;
+
 	rc = chip_read(chip);
+	if (rc < 0)
+		return rc;
+
 	return ((rc & 0x1f) == 0x1f) ? 1 : 0;
 }
 
-- 
2.14.3

[PATCH 11/30] media: bttv-input: better handle errors at I2C transfer

[Mauro Carvalho Chehab]

The error handling logic at get_key_pv951() is a little bit
akward, with produces this false positive warning:

	drivers/media/pci/bt8xx/bttv-input.c:344 get_key_pv951() error: uninitialized symbol 'b'.

Do a cleanup. As a side effect, it also improves its coding
style.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/bt8xx/bttv-input.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c
index da49c5567db5..08266b23826e 100644
--- a/drivers/media/pci/bt8xx/bttv-input.c
+++ b/drivers/media/pci/bt8xx/bttv-input.c
@@ -332,11 +332,15 @@ static void bttv_ir_stop(struct bttv *btv)
 static int get_key_pv951(struct IR_i2c *ir, enum rc_proto *protocol,
 			 u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		dprintk("read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
-- 
2.14.3

[PATCH 12/30] media: solo6x10: simplify the logic at solo_p2m_dma_desc()

[Mauro Carvalho Chehab]

The logic with gets a p2m_id is more complex than needed,
causing false positives with static analyzers:

	drivers/media/pci/solo6x10/solo6x10-p2m.c:81 solo_p2m_dma_desc() error: buffer overflow 'solo_dev->p2m_dev' 4 <= s32max

Make it simpler and use unsigned int.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/solo6x10/solo6x10-p2m.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-p2m.c b/drivers/media/pci/solo6x10/solo6x10-p2m.c
index 8c8484674d2f..46c30430e30b 100644
--- a/drivers/media/pci/solo6x10/solo6x10-p2m.c
+++ b/drivers/media/pci/solo6x10/solo6x10-p2m.c
@@ -69,14 +69,11 @@ int solo_p2m_dma_desc(struct solo_dev *solo_dev,
 	unsigned int timeout;
 	unsigned int config = 0;
 	int ret = 0;
-	int p2m_id = 0;
+	unsigned int p2m_id = 0;
 
 	/* Get next ID. According to Softlogic, 6110 has problems on !=0 P2M */
-	if (solo_dev->type != SOLO_DEV_6110 && multi_p2m) {
+	if (solo_dev->type != SOLO_DEV_6110 && multi_p2m)
 		p2m_id = atomic_inc_return(&solo_dev->p2m_count) % SOLO_NR_P2M;
-		if (p2m_id < 0)
-			p2m_id = -p2m_id;
-	}
 
 	p2m_dev = &solo_dev->p2m_dev[p2m_id];
 
-- 
2.14.3

[PATCH 13/30] media: cx88: fix two warnings

[Mauro Carvalho Chehab]

drivers/media/pci/cx88/cx88-alsa.c:295 cx88_alsa_dma_init() warn: argument 3 to %08lx specifier is cast from pointer
drivers/media/pci/cx88/cx88-alsa.c:669 snd_cx88_wm8775_volume_put() warn: potential negative subtraction from max '65535 - (32768 * left) / right'

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

diff --git a/drivers/media/pci/cx88/cx88-alsa.c b/drivers/media/pci/cx88/cx88-alsa.c
index 9740326bc93f..ab09bb55cf45 100644
--- a/drivers/media/pci/cx88/cx88-alsa.c
+++ b/drivers/media/pci/cx88/cx88-alsa.c
@@ -292,8 +292,8 @@ static int cx88_alsa_dma_init(struct cx88_audio_dev *chip, int nr_pages)
 		return -ENOMEM;
 	}
 
-	dprintk(1, "vmalloc is at addr 0x%08lx, size=%d\n",
-		(unsigned long)buf->vaddr, nr_pages << PAGE_SHIFT);
+	dprintk(1, "vmalloc is at addr %p, size=%d\n",
+		buf->vaddr, nr_pages << PAGE_SHIFT);
 
 	memset(buf->vaddr, 0, nr_pages << PAGE_SHIFT);
 	buf->nr_pages = nr_pages;
@@ -656,8 +656,8 @@ static void snd_cx88_wm8775_volume_put(struct snd_kcontrol *kcontrol,
 {
 	struct cx88_audio_dev *chip = snd_kcontrol_chip(kcontrol);
 	struct cx88_core *core = chip->core;
-	int left = value->value.integer.value[0];
-	int right = value->value.integer.value[1];
+	u16 left = value->value.integer.value[0];
+	u16 right = value->value.integer.value[1];
 	int v, b;
 
 	/* Pass volume & balance onto any WM8775 */
-- 
2.14.3

[PATCH 14/30] media: cx23885: fix a warning

[Mauro Carvalho Chehab]

drivers/media/pci/cx23885/cx23885-alsa.c:92 cx23885_alsa_dma_init() warn: argument 3 to %08lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/cx23885/cx23885-alsa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-alsa.c b/drivers/media/pci/cx23885/cx23885-alsa.c
index d8c3637e492e..20b3cb17f97f 100644
--- a/drivers/media/pci/cx23885/cx23885-alsa.c
+++ b/drivers/media/pci/cx23885/cx23885-alsa.c
@@ -89,9 +89,8 @@ static int cx23885_alsa_dma_init(struct cx23885_audio_dev *chip, int nr_pages)
 		return -ENOMEM;
 	}
 
-	dprintk(1, "vmalloc is at addr 0x%08lx, size=%d\n",
-				(unsigned long)buf->vaddr,
-				nr_pages << PAGE_SHIFT);
+	dprintk(1, "vmalloc is at addr %p, size=%d\n",
+		buf->vaddr, nr_pages << PAGE_SHIFT);
 
 	memset(buf->vaddr, 0, nr_pages << PAGE_SHIFT);
 	buf->nr_pages = nr_pages;
-- 
2.14.3

[PATCH 15/30] soc_camera: fix a weird cast on printk

[Mauro Carvalho Chehab]

drivers/media/platform/soc_camera/soc_camera.c:790 soc_camera_mmap() warn: argument 4 to %08lx specifier is cast from pointer

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

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 1318512c8fe3..69f0d8e80bd8 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -787,7 +787,7 @@ static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	int err;
 
-	dev_dbg(icd->pdev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
+	dev_dbg(icd->pdev, "mmap called, vma=%p\n", vma);
 
 	if (icd->streamer != file)
 		return -EBUSY;
-- 
2.14.3

[PATCH 16/30] media: videobuf-dma-sg: Fix a weird cast

[Mauro Carvalho Chehab]

Just use %p. Fixes this warning:
	drivers/media/v4l2-core/videobuf-dma-sg.c:247 videobuf_dma_init_kernel() warn: argument 2 to %08lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/videobuf-dma-sg.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c
index f412429cf5ba..add2edb23eac 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -244,9 +244,8 @@ static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int direction,
 		goto out_free_pages;
 	}
 
-	dprintk(1, "vmalloc is at addr 0x%08lx, size=%d\n",
-				(unsigned long)dma->vaddr,
-				nr_pages << PAGE_SHIFT);
+	dprintk(1, "vmalloc is at addr %p, size=%d\n",
+		dma->vaddr, nr_pages << PAGE_SHIFT);
 
 	memset(dma->vaddr, 0, nr_pages << PAGE_SHIFT);
 	dma->nr_pages = nr_pages;
-- 
2.14.3

[PATCH 17/30] media: ivtvfb: Cleanup some warnings

[Mauro Carvalho Chehab]

drivers/media/pci/ivtv/ivtvfb.c:349 ivtvfb_prep_frame() warn: argument 3 to %08lx specifier is cast from pointer
drivers/media/pci/ivtv/ivtvfb.c:360 ivtvfb_prep_frame() warn: argument 3 to %08lx specifier is cast from pointer
drivers/media/pci/ivtv/ivtvfb.c:363 ivtvfb_prep_frame() warn: argument 4 to %08lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/ivtv/ivtvfb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 621b2f613d81..8e62b8be6529 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -346,8 +346,8 @@ static int ivtvfb_prep_frame(struct ivtv *itv, int cmd, void __user *source,
 
 	/* Not fatal, but will have undesirable results */
 	if ((unsigned long)source & 3)
-		IVTVFB_WARN("ivtvfb_prep_frame: Source address not 32 bit aligned (0x%08lx)\n",
-			(unsigned long)source);
+		IVTVFB_WARN("ivtvfb_prep_frame: Source address not 32 bit aligned (%p)\n",
+			    source);
 
 	if (dest_offset & 3)
 		IVTVFB_WARN("ivtvfb_prep_frame: Dest offset not 32 bit aligned (%ld)\n", dest_offset);
@@ -357,12 +357,10 @@ static int ivtvfb_prep_frame(struct ivtv *itv, int cmd, void __user *source,
 
 	/* Check Source */
 	if (!access_ok(VERIFY_READ, source + dest_offset, count)) {
-		IVTVFB_WARN("Invalid userspace pointer 0x%08lx\n",
-			(unsigned long)source);
+		IVTVFB_WARN("Invalid userspace pointer %p\n", source);
 
-		IVTVFB_DEBUG_WARN("access_ok() failed for offset 0x%08lx source 0x%08lx count %d\n",
-			dest_offset, (unsigned long)source,
-			count);
+		IVTVFB_DEBUG_WARN("access_ok() failed for offset 0x%08lx source %p count %d\n",
+				  dest_offset, source, count);
 		return -EINVAL;
 	}
 
-- 
2.14.3

[PATCH 18/30] media: s2255drv: fix a casting warning

[Mauro Carvalho Chehab]

drivers/media/usb/s2255/s2255drv.c:651 s2255_fillbuff() warn: argument 3 to %08lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/s2255/s2255drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
index a00a15f55d37..82927eb334c4 100644
--- a/drivers/media/usb/s2255/s2255drv.c
+++ b/drivers/media/usb/s2255/s2255drv.c
@@ -648,8 +648,8 @@ static void s2255_fillbuff(struct s2255_vc *vc,
 		pr_err("s2255: =======no frame\n");
 		return;
 	}
-	dprintk(dev, 2, "s2255fill at : Buffer 0x%08lx size= %d\n",
-		(unsigned long)vbuf, pos);
+	dprintk(dev, 2, "s2255fill at : Buffer %p size= %d\n",
+		vbuf, pos);
 }
 
 
-- 
2.14.3

[PATCH 19/30] media: saa7134-input: improve error handling

[Mauro Carvalho Chehab]

Currently, the code produces those false-positives:
	drivers/media/pci/saa7134/saa7134-input.c:203 get_key_msi_tvanywhere_plus() error: uninitialized symbol 'b'.
	drivers/media/pci/saa7134/saa7134-input.c:251 get_key_kworld_pc150u() error: uninitialized symbol 'b'.
	drivers/media/pci/saa7134/saa7134-input.c:275 get_key_purpletv() error: uninitialized symbol 'b'.

Improve the error handling code, making it to look like our
coding style.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/saa7134/saa7134-input.c | 46 +++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c
index 33ee8322895e..0e28c5021ac4 100644
--- a/drivers/media/pci/saa7134/saa7134-input.c
+++ b/drivers/media/pci/saa7134/saa7134-input.c
@@ -115,7 +115,7 @@ static int build_key(struct saa7134_dev *dev)
 static int get_key_flydvb_trio(struct IR_i2c *ir, enum rc_proto *protocol,
 			       u32 *scancode, u8 *toggle)
 {
-	int gpio;
+	int gpio, rc;
 	int attempt = 0;
 	unsigned char b;
 
@@ -153,8 +153,11 @@ static int get_key_flydvb_trio(struct IR_i2c *ir, enum rc_proto *protocol,
 		       attempt);
 		return -EIO;
 	}
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -169,7 +172,7 @@ static int get_key_msi_tvanywhere_plus(struct IR_i2c *ir,
 				       u32 *scancode, u8 *toggle)
 {
 	unsigned char b;
-	int gpio;
+	int gpio, rc;
 
 	/* <dev> is needed to access GPIO. Used by the saa_readl macro. */
 	struct saa7134_dev *dev = ir->c->adapter->algo_data;
@@ -193,8 +196,11 @@ static int get_key_msi_tvanywhere_plus(struct IR_i2c *ir,
 
 	/* GPIO says there is a button press. Get it. */
 
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -218,6 +224,7 @@ static int get_key_kworld_pc150u(struct IR_i2c *ir, enum rc_proto *protocol,
 {
 	unsigned char b;
 	unsigned int gpio;
+	int rc;
 
 	/* <dev> is needed to access GPIO. Used by the saa_readl macro. */
 	struct saa7134_dev *dev = ir->c->adapter->algo_data;
@@ -241,8 +248,11 @@ static int get_key_kworld_pc150u(struct IR_i2c *ir, enum rc_proto *protocol,
 
 	/* GPIO says there is a button press. Get it. */
 
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -263,11 +273,15 @@ static int get_key_kworld_pc150u(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_purpletv(struct IR_i2c *ir, enum rc_proto *protocol,
 			    u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -288,11 +302,17 @@ static int get_key_purpletv(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_hvr1110(struct IR_i2c *ir, enum rc_proto *protocol,
 			   u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char buf[5];
 
 	/* poll IR chip */
-	if (5 != i2c_master_recv(ir->c, buf, 5))
+	rc = i2c_master_recv(ir->c, buf, 5);
+	if (rc != 5) {
+		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
+	}
 
 	/* Check if some key were pressed */
 	if (!(buf[0] & 0x80))
@@ -319,6 +339,7 @@ static int get_key_hvr1110(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_beholdm6xx(struct IR_i2c *ir, enum rc_proto *protocol,
 			      u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char data[12];
 	u32 gpio;
 
@@ -335,8 +356,11 @@ static int get_key_beholdm6xx(struct IR_i2c *ir, enum rc_proto *protocol,
 
 	ir->c->addr = 0x5a >> 1;
 
-	if (12 != i2c_master_recv(ir->c, data, 12)) {
+	rc = i2c_master_recv(ir->c, data, 12);
+	if (rc != 12) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -356,12 +380,16 @@ static int get_key_pinnacle(struct IR_i2c *ir, enum rc_proto *protocol,
 			    u32 *scancode, u8 *toggle, int parity_offset,
 			    int marker, int code_modulo)
 {
+	int rc;
 	unsigned char b[4];
 	unsigned int start = 0,parity = 0,code = 0;
 
 	/* poll IR chip */
-	if (4 != i2c_master_recv(ir->c, b, 4)) {
+	rc = i2c_master_recv(ir->c, b, 4);
+	if (rc != 4) {
 		ir_dbg(ir, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
-- 
2.14.3

[PATCH 20/30] media: ir-kbd-i2c: improve error handling code

[Mauro Carvalho Chehab]

The current I2C error handling logic makes static analyzers
confused, and it doesn't follow the coding style we're using:

	drivers/media/i2c/ir-kbd-i2c.c:180 get_key_pixelview() error: uninitialized symbol 'b'.
	drivers/media/i2c/ir-kbd-i2c.c:224 get_key_knc1() error: uninitialized symbol 'b'.
	drivers/media/i2c/ir-kbd-i2c.c:226 get_key_knc1() error: uninitialized symbol 'b'.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/ir-kbd-i2c.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
index 193020d64e51..1d11aab1817a 100644
--- a/drivers/media/i2c/ir-kbd-i2c.c
+++ b/drivers/media/i2c/ir-kbd-i2c.c
@@ -168,11 +168,15 @@ static int get_key_haup_xvr(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_pixelview(struct IR_i2c *ir, enum rc_proto *protocol,
 			     u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		dev_dbg(&ir->rc->dev, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -185,11 +189,15 @@ static int get_key_pixelview(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_fusionhdtv(struct IR_i2c *ir, enum rc_proto *protocol,
 			      u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char buf[4];
 
 	/* poll IR chip */
-	if (4 != i2c_master_recv(ir->c, buf, 4)) {
+	rc = i2c_master_recv(ir->c, buf, 4);
+	if (rc != 4) {
 		dev_dbg(&ir->rc->dev, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
@@ -209,11 +217,15 @@ static int get_key_fusionhdtv(struct IR_i2c *ir, enum rc_proto *protocol,
 static int get_key_knc1(struct IR_i2c *ir, enum rc_proto *protocol,
 			u32 *scancode, u8 *toggle)
 {
+	int rc;
 	unsigned char b;
 
 	/* poll IR chip */
-	if (1 != i2c_master_recv(ir->c, &b, 1)) {
+	rc = i2c_master_recv(ir->c, &b, 1);
+	if (rc != 1) {
 		dev_dbg(&ir->rc->dev, "read error\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
-- 
2.14.3

[PATCH 21/30] media: ir-kbd-i2c: change the if logic to avoid a warning

[Mauro Carvalho Chehab]

While the code is correct, it produces this warning:
	drivers/media/i2c/ir-kbd-i2c.c:593 zilog_ir_format() error: buffer overflow 'code_block->codes' 61 <= 173

As static analyzers may be tricked by arithmetic expressions on
comparisions. So, change the order, in order to shut up this
false-positive warning.

That also makes easier for humans to understand that it won't
be trying to go past buffer size.

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

diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c
index 1d11aab1817a..a7e23bcf845c 100644
--- a/drivers/media/i2c/ir-kbd-i2c.c
+++ b/drivers/media/i2c/ir-kbd-i2c.c
@@ -583,7 +583,7 @@ static int zilog_ir_format(struct rc_dev *rcdev, unsigned int *txbuf,
 		/* first copy any leading non-repeating */
 		int leading = c - rep * 3;
 
-		if (leading + rep >= ARRAY_SIZE(code_block->codes) - 3) {
+		if (leading >= ARRAY_SIZE(code_block->codes) - 3 - rep) {
 			dev_warn(&rcdev->dev, "IR too long, cannot transmit\n");
 			return -EINVAL;
 		}
-- 
2.14.3

[PATCH 22/30] media: zoran: don't cast pointers to print them

[Mauro Carvalho Chehab]

drivers/media/pci/zoran/zoran_driver.c:242 v4l_fbuffer_alloc() warn: argument 5 to %lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/zoran/zoran_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/zoran/zoran_driver.c b/drivers/media/pci/zoran/zoran_driver.c
index 8d4e7d930a66..14f9c0e26a1c 100644
--- a/drivers/media/pci/zoran/zoran_driver.c
+++ b/drivers/media/pci/zoran/zoran_driver.c
@@ -241,8 +241,8 @@ static int v4l_fbuffer_alloc(struct zoran_fh *fh)
 			SetPageReserved(virt_to_page(mem + off));
 		dprintk(4,
 			KERN_INFO
-			"%s: %s - V4L frame %d mem 0x%lx (bus: 0x%llx)\n",
-			ZR_DEVNAME(zr), __func__, i, (unsigned long) mem,
+			"%s: %s - V4L frame %d mem %p (bus: 0x%llx)\n",
+			ZR_DEVNAME(zr), __func__, i, mem,
 			(unsigned long long)virt_to_bus(mem));
 	}
 
-- 
2.14.3

[PATCH 23/30] media: solo6x10: get rid of an address space warning

[Mauro Carvalho Chehab]

Instead of using an ancillary function to avoid duplicating
a small portion of code that copies data either to kernelspace
or between userspace-kernelspace, duplicate the code,
as it prevents static analyzers to complain about it:

	drivers/media/pci/solo6x10/solo6x10-g723.c:260:46: warning: cast removes address space of expression

The hole idea of using __user is to make sure that the code is
doing the right thing with address space, so there's no
sense on use casting.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/solo6x10/solo6x10-g723.c | 73 +++++++++++++++++-------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-g723.c b/drivers/media/pci/solo6x10/solo6x10-g723.c
index 81be1b8df758..2ac33b5cc454 100644
--- a/drivers/media/pci/solo6x10/solo6x10-g723.c
+++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
@@ -223,48 +223,57 @@ static snd_pcm_uframes_t snd_solo_pcm_pointer(struct snd_pcm_substream *ss)
 	return idx * G723_FRAMES_PER_PAGE;
 }
 
-static int __snd_solo_pcm_copy(struct snd_pcm_substream *ss,
-			       unsigned long pos, void *dst,
-			       unsigned long count, bool in_kernel)
-{
-	struct solo_snd_pcm *solo_pcm = snd_pcm_substream_chip(ss);
-	struct solo_dev *solo_dev = solo_pcm->solo_dev;
-	int err, i;
-
-	for (i = 0; i < (count / G723_FRAMES_PER_PAGE); i++) {
-		int page = (pos / G723_FRAMES_PER_PAGE) + i;
-
-		err = solo_p2m_dma_t(solo_dev, 0, solo_pcm->g723_dma,
-				     SOLO_G723_EXT_ADDR(solo_dev) +
-				     (page * G723_PERIOD_BLOCK) +
-				     (ss->number * G723_PERIOD_BYTES),
-				     G723_PERIOD_BYTES, 0, 0);
-		if (err)
-			return err;
-
-		if (in_kernel)
-			memcpy(dst, solo_pcm->g723_buf, G723_PERIOD_BYTES);
-		else if (copy_to_user((void __user *)dst,
-				      solo_pcm->g723_buf, G723_PERIOD_BYTES))
-			return -EFAULT;
-		dst += G723_PERIOD_BYTES;
-	}
-
-	return 0;
-}
-
 static int snd_solo_pcm_copy_user(struct snd_pcm_substream *ss, int channel,
 				  unsigned long pos, void __user *dst,
 				  unsigned long count)
 {
-	return __snd_solo_pcm_copy(ss, pos, (void *)dst, count, false);
+	struct solo_snd_pcm *solo_pcm = snd_pcm_substream_chip(ss);
+	struct solo_dev *solo_dev = solo_pcm->solo_dev;
+	int err, i;
+
+	for (i = 0; i < (count / G723_FRAMES_PER_PAGE); i++) {
+		int page = (pos / G723_FRAMES_PER_PAGE) + i;
+
+		err = solo_p2m_dma_t(solo_dev, 0, solo_pcm->g723_dma,
+				     SOLO_G723_EXT_ADDR(solo_dev) +
+				     (page * G723_PERIOD_BLOCK) +
+				     (ss->number * G723_PERIOD_BYTES),
+				     G723_PERIOD_BYTES, 0, 0);
+		if (err)
+			return err;
+
+		if (copy_to_user(dst, solo_pcm->g723_buf, G723_PERIOD_BYTES))
+			return -EFAULT;
+		dst += G723_PERIOD_BYTES;
+	}
+
+	return 0;
 }
 
 static int snd_solo_pcm_copy_kernel(struct snd_pcm_substream *ss, int channel,
 				    unsigned long pos, void *dst,
 				    unsigned long count)
 {
-	return __snd_solo_pcm_copy(ss, pos, dst, count, true);
+	struct solo_snd_pcm *solo_pcm = snd_pcm_substream_chip(ss);
+	struct solo_dev *solo_dev = solo_pcm->solo_dev;
+	int err, i;
+
+	for (i = 0; i < (count / G723_FRAMES_PER_PAGE); i++) {
+		int page = (pos / G723_FRAMES_PER_PAGE) + i;
+
+		err = solo_p2m_dma_t(solo_dev, 0, solo_pcm->g723_dma,
+				     SOLO_G723_EXT_ADDR(solo_dev) +
+				     (page * G723_PERIOD_BLOCK) +
+				     (ss->number * G723_PERIOD_BYTES),
+				     G723_PERIOD_BYTES, 0, 0);
+		if (err)
+			return err;
+
+		memcpy(dst, solo_pcm->g723_buf, G723_PERIOD_BYTES);
+		dst += G723_PERIOD_BYTES;
+	}
+
+	return 0;
 }
 
 static const struct snd_pcm_ops snd_solo_pcm_ops = {
-- 
2.14.3

[PATCH 24/30] media: saa7134-alsa: don't use casts to print a buffer address

[Mauro Carvalho Chehab]

Change the logic there to avoid casting, solving this warning:
	drivers/media/pci/saa7134/saa7134-alsa.c:276 saa7134_alsa_dma_init() warn: argument 3 to %08lx specifier is cast from pointer

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/saa7134/saa7134-alsa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c
index c59b69f1af9d..72311445d13d 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -273,9 +273,8 @@ static int saa7134_alsa_dma_init(struct saa7134_dev *dev, int nr_pages)
 		return -ENOMEM;
 	}
 
-	pr_debug("vmalloc is at addr 0x%08lx, size=%d\n",
-				(unsigned long)dma->vaddr,
-				nr_pages << PAGE_SHIFT);
+	pr_debug("vmalloc is at addr %p, size=%d\n",
+		 dma->vaddr, nr_pages << PAGE_SHIFT);
 
 	memset(dma->vaddr, 0, nr_pages << PAGE_SHIFT);
 	dma->nr_pages = nr_pages;
-- 
2.14.3

[PATCH 25/30] media: vivid-radio-rx: add a cast to avoid a warning

[Mauro Carvalho Chehab]

The logic at vivid_radio_rx_g_tuner() is producint an overflow
warning:

	drivers/media/platform/vivid/vivid-radio-rx.c:250 vivid_radio_rx_g_tuner() warn: potential negative subtraction from max '65535 - (__builtin_choose_expr( ==  ||  == , , __builtin_choose_expr( ==  ||  == , , __builtin_choose_expr( ==  ||  == , , __builtin_choose_expr( ==  ||  == , , __builtin_choose_expr( ==  ||  == , , __builtin_choose_expr( == , , (0))))))) * 65535) / delta'

Add a cast to prevent that.

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

diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c b/drivers/media/platform/vivid/vivid-radio-rx.c
index acbfea2cce76..1f86d7d4f72f 100644
--- a/drivers/media/platform/vivid/vivid-radio-rx.c
+++ b/drivers/media/platform/vivid/vivid-radio-rx.c
@@ -247,7 +247,7 @@ int vivid_radio_rx_g_tuner(struct file *file, void *fh, struct v4l2_tuner *vt)
 	vt->rangehigh = FM_FREQ_RANGE_HIGH;
 	sig_qual = dev->radio_rx_sig_qual;
 	vt->signal = abs(sig_qual) > delta ? 0 :
-		     0xffff - (abs(sig_qual) * 0xffff) / delta;
+		     0xffff - ((unsigned)abs(sig_qual) * 0xffff) / delta;
 	vt->afc = sig_qual > delta ? 0 : sig_qual;
 	if (abs(sig_qual) > delta)
 		vt->rxsubchans = 0;
-- 
2.14.3

[PATCH 27/30] media: em28xx-input: improve error handling code

[Mauro Carvalho Chehab]

The current I2C error handling logic makes static analyzers
confused:

	drivers/media/usb/em28xx/em28xx-input.c:96 em28xx_get_key_terratec() error: uninitialized symbol 'b'.

Change it to match the coding style we're using elsewhere.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/em28xx/em28xx-input.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index eb2ec0384b69..2dc1be00b8b8 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -82,11 +82,16 @@ struct em28xx_IR {
 static int em28xx_get_key_terratec(struct i2c_client *i2c_dev,
 				   enum rc_proto *protocol, u32 *scancode)
 {
+	int rc;
 	unsigned char b;
 
 	/* poll IR chip */
-	if (i2c_master_recv(i2c_dev, &b, 1) != 1)
+	rc = i2c_master_recv(i2c_dev, &b, 1);
+	if (rc != 1) {
+		if (rc < 0)
+			return rc;
 		return -EIO;
+	}
 
 	/*
 	 * it seems that 0xFE indicates that a button is still hold
-- 
2.14.3

[PATCH 28/30] media: tm6000: avoid casting just to print pointer address

[Mauro Carvalho Chehab]

Instead of casting, just use %p.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/tm6000/tm6000-video.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index 8314d3fa9241..b2399d4266da 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -1346,9 +1346,8 @@ static int __tm6000_open(struct file *file)
 	fh->width = dev->width;
 	fh->height = dev->height;
 
-	dprintk(dev, V4L2_DEBUG_OPEN, "Open: fh=0x%08lx, dev=0x%08lx, dev->vidq=0x%08lx\n",
-			(unsigned long)fh, (unsigned long)dev,
-			(unsigned long)&dev->vidq);
+	dprintk(dev, V4L2_DEBUG_OPEN, "Open: fh=%p, dev=%p, dev->vidq=%p\n",
+		fh, dev, &dev->vidq);
 	dprintk(dev, V4L2_DEBUG_OPEN, "Open: list_empty queued=%d\n",
 		list_empty(&dev->vidq.queued));
 	dprintk(dev, V4L2_DEBUG_OPEN, "Open: list_empty active=%d\n",
-- 
2.14.3

[PATCH 29/30] media: tda9840: cleanup a warning

[Mauro Carvalho Chehab]

There's a false positive warning there:
	drivers/media/i2c/tda9840.c:79 tda9840_status() error: uninitialized symbol 'byte'.

Change the code to match our coding style, in order to fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/i2c/tda9840.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tda9840.c b/drivers/media/i2c/tda9840.c
index f31e659588ac..0dd6ff3e6201 100644
--- a/drivers/media/i2c/tda9840.c
+++ b/drivers/media/i2c/tda9840.c
@@ -68,11 +68,15 @@ static void tda9840_write(struct v4l2_subdev *sd, u8 reg, u8 val)
 static int tda9840_status(struct v4l2_subdev *sd)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int rc;
 	u8 byte;
 
-	if (1 != i2c_master_recv(client, &byte, 1)) {
+	rc = i2c_master_recv(client, &byte, 1);
+	if (rc != 1) {
 		v4l2_dbg(1, debug, sd,
 			"i2c_master_recv() failed\n");
+		if (rc < 0)
+			return rc;
 		return -EIO;
 	}
 
-- 
2.14.3

[PATCH 30/30] media: cec-core: fix a bug at cec_error_inj_write()

[Mauro Carvalho Chehab]

If the adapter doesn't have error_inj_parse_line() ops, the
write() logic won't return -EINVAL, but, instead, it will keep
looping, because "count" is a non-negative number.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/cec/cec-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index ea3eccfdba15..b0c87f9ea08f 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -209,14 +209,14 @@ static ssize_t cec_error_inj_write(struct file *file,
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 	p = buf;
-	while (p && *p && count >= 0) {
+	while (p && *p) {
 		p = skip_spaces(p);
 		line = strsep(&p, "\n");
 		if (!*line || *line == '#')
 			continue;
 		if (!adap->ops->error_inj_parse_line(adap, line)) {
-			count = -EINVAL;
-			break;
+			kfree(buf);
+			return -EINVAL;
 		}
 	}
 	kfree(buf);
-- 
2.14.3

Re: [PATCH 30/30] media: cec-core: fix a bug at cec_error_inj_write()

[Hans Verkuil]

On 03/23/18 12:57, Mauro Carvalho Chehab wrote:
> If the adapter doesn't have error_inj_parse_line() ops, the
> write() logic won't return -EINVAL, but, instead, it will keep
> looping, because "count" is a non-negative number.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  drivers/media/cec/cec-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
> index ea3eccfdba15..b0c87f9ea08f 100644
> --- a/drivers/media/cec/cec-core.c
> +++ b/drivers/media/cec/cec-core.c
> @@ -209,14 +209,14 @@ static ssize_t cec_error_inj_write(struct file *file,
>  	if (IS_ERR(buf))
>  		return PTR_ERR(buf);
>  	p = buf;
> -	while (p && *p && count >= 0) {
> +	while (p && *p) {
>  		p = skip_spaces(p);
>  		line = strsep(&p, "\n");
>  		if (!*line || *line == '#')
>  			continue;
>  		if (!adap->ops->error_inj_parse_line(adap, line)) {
> -			count = -EINVAL;
> -			break;
> +			kfree(buf);
> +			return -EINVAL;
>  		}
>  	}
>  	kfree(buf);
> 

[26/30] media: zr364xx: avoid casting just to print pointer address

[Mauro Carvalho Chehab]

Instead of casting, just use %p.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/zr364xx/zr364xx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c
index 8b7c19943d46..b8886102c5ed 100644
--- a/drivers/media/usb/zr364xx/zr364xx.c
+++ b/drivers/media/usb/zr364xx/zr364xx.c
@@ -517,8 +517,7 @@ static void zr364xx_fillbuff(struct zr364xx_camera *cam,
 		printk(KERN_ERR KBUILD_MODNAME ": =======no frame\n");
 		return;
 	}
-	DBG("%s: Buffer 0x%08lx size= %d\n", __func__,
-		(unsigned long)vbuf, pos);
+	DBG("%s: Buffer %p size= %d\n", __func__, vbuf, pos);
 	/* tell v4l buffer was filled */
 
 	buf->vb.field_count = cam->frame_count * 2;
@@ -1277,7 +1276,7 @@ static int zr364xx_mmap(struct file *file, struct vm_area_struct *vma)
 		DBG("%s: cam == NULL\n", __func__);
 		return -ENODEV;
 	}
-	DBG("mmap called, vma=0x%08lx\n", (unsigned long)vma);
+	DBG("mmap called, vma=%p\n", vma);
 
 	ret = videobuf_mmap_mapper(&cam->vb_vidq, vma);
 

[PATCH 26/30] media: zr364xx: avoid casting just to print pointer address

[Mauro Carvalho Chehab]

Instead of casting, just use %p.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/usb/zr364xx/zr364xx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/zr364xx/zr364xx.c b/drivers/media/usb/zr364xx/zr364xx.c
index 8b7c19943d46..b8886102c5ed 100644
--- a/drivers/media/usb/zr364xx/zr364xx.c
+++ b/drivers/media/usb/zr364xx/zr364xx.c
@@ -517,8 +517,7 @@ static void zr364xx_fillbuff(struct zr364xx_camera *cam,
 		printk(KERN_ERR KBUILD_MODNAME ": =======no frame\n");
 		return;
 	}
-	DBG("%s: Buffer 0x%08lx size= %d\n", __func__,
-		(unsigned long)vbuf, pos);
+	DBG("%s: Buffer %p size= %d\n", __func__, vbuf, pos);
 	/* tell v4l buffer was filled */
 
 	buf->vb.field_count = cam->frame_count * 2;
@@ -1277,7 +1276,7 @@ static int zr364xx_mmap(struct file *file, struct vm_area_struct *vma)
 		DBG("%s: cam == NULL\n", __func__);
 		return -ENODEV;
 	}
-	DBG("mmap called, vma=0x%08lx\n", (unsigned long)vma);
+	DBG("mmap called, vma=%p\n", vma);
 
 	ret = videobuf_mmap_mapper(&cam->vb_vidq, vma);
 
-- 
2.14.3