[PATCH 01/17] media: staging: atomisp: fix number conversion

[PATCH 01/17] media: staging: atomisp: fix number conversion

[Mauro Carvalho Chehab]

smatch says that there's an issue with number
conversion:

   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_params.c:4154 sh_css_params_write_to_ddr_internal() warn: '((-(1 << ((14 - 1)))))' 4294959104 can't fit into 32767 'converted_macc_table.data[idx]'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_params.c:4157 sh_css_params_write_to_ddr_internal() warn: '((-(1 << ((14 - 1)))))' 4294959104 can't fit into 32767 'converted_macc_table.data[idx + 1]'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_params.c:4160 sh_css_params_write_to_ddr_internal() warn: '((-(1 << ((14 - 1)))))' 4294959104 can't fit into 32767 'converted_macc_table.data[idx + 2]'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_params.c:4163 sh_css_params_write_to_ddr_internal() warn: '((-(1 << ((14 - 1)))))' 4294959104 can't fit into 32767 'converted_macc_table.data[idx + 3]'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c:168 ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + j]'

That's probably because min() and max() definition used there
are really poor ones. So, replace by the in-kernel macro.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c      | 16 ++++++++++++----
 .../media/atomisp/pci/atomisp2/css2400/sh_css_frac.h     |  2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c
index 47bb5042381b..8f2178bf9e68 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/eed1_8/ia_css_eed1_8.host.c
@@ -160,17 +160,25 @@ ia_css_eed1_8_vmem_encode(
 		base = shuffle_block * i;
 
 		for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) {
-			to->e_dew_enh_x[0][base + j] = min(max(from->dew_enhance_seg_x[j], 0), 8191);
-			to->e_dew_enh_y[0][base + j] = min(max(from->dew_enhance_seg_y[j], -8192), 8191);
+			to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int,
+									from->dew_enhance_seg_x[j], 0),
+									8191);
+			to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int,
+									from->dew_enhance_seg_y[j], -8192),
+									8191);
 		}
 
 		for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) {
-			to->e_dew_enh_a[0][base + j] = min(max(from->dew_enhance_seg_slope[j], -8192), 8191);
+			to->e_dew_enh_a[0][base + j] = min_t(int, max_t(int,
+									from->dew_enhance_seg_slope[j],
+								        -8192), 8191);
 			/* Convert dew_enhance_seg_exp to flag:
 			 * 0 -> 0
 			 * 1...13 -> 1
 			 */
-			to->e_dew_enh_f[0][base + j] = (min(max(from->dew_enhance_seg_exp[j], 0), 13) > 0);
+			to->e_dew_enh_f[0][base + j] = (min_t(int, max_t(int,
+									 from->dew_enhance_seg_exp[j],
+									 0), 13) > 0);
 		}
 
 		/* Hard-coded to 0, in order to be able to handle out of
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_frac.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_frac.h
index 1d1771d71f3c..90a63b3921e6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_frac.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css_frac.h
@@ -30,7 +30,7 @@
 
 /* a:fraction bits for 16bit precision, b:fraction bits for ISP precision */
 #define sDIGIT_FITTING(v, a, b) \
-	min(max((((v)>>sSHIFT) >> max(sFRACTION_BITS_FITTING(a)-(b), 0)), \
+	min_t(int, max_t(int, (((v)>>sSHIFT) >> max(sFRACTION_BITS_FITTING(a)-(b), 0)), \
 	  sISP_VAL_MIN), sISP_VAL_MAX)
 #define uDIGIT_FITTING(v, a, b) \
 	min((unsigned)max((unsigned)(((v)>>uSHIFT) \
-- 
2.14.3

[PATCH 02/17] media: staging: atomisp: don't declare the same vars as both private and public

[Mauro Carvalho Chehab]

The mmu_private.h header is included at mmu.c, with duplicates the
already existing definitions at mmu_public.h.

Fix this by removing the erroneous header file.

Solve those issues:

    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:24:26: warning: function 'mmu_reg_store' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:35:30: warning: function 'mmu_reg_load' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:24:26: warning: function 'mmu_reg_store' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:35:30: warning: function 'mmu_reg_load' with external linkage has definition

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../css2400/hive_isp_css_common/host/mmu.c         |  4 --
 .../css2400/hive_isp_css_common/host/mmu_private.h | 44 ----------------------
 .../css2400/hive_isp_css_include/host/mmu_public.h |  4 +-
 .../css2400/hive_isp_css_include/mmu_device.h      |  8 ----
 4 files changed, 2 insertions(+), 58 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
index a28b67eb66ea..1a1719d3e745 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
@@ -15,10 +15,6 @@
 /* The name "mmu.h is already taken" */
 #include "mmu_device.h"
 
-#ifndef __INLINE_MMU__
-#include "mmu_private.h"
-#endif /* __INLINE_MMU__ */
-
 void mmu_set_page_table_base_index(
 	const mmu_ID_t		ID,
 	const hrt_data		base_index)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h
deleted file mode 100644
index 7377666f6eb7..000000000000
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * Support for Intel Camera Imaging ISP subsystem.
- * Copyright (c) 2010-2015, Intel Corporation.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- */
-
-#ifndef __MMU_PRIVATE_H_INCLUDED__
-#define __MMU_PRIVATE_H_INCLUDED__
-
-#include "mmu_public.h"
-
-#include "device_access.h"
-
-#include "assert_support.h"
-
-STORAGE_CLASS_MMU_H void mmu_reg_store(
-	const mmu_ID_t		ID,
-	const unsigned int	reg,
-	const hrt_data		value)
-{
-	assert(ID < N_MMU_ID);
-	assert(MMU_BASE[ID] != (hrt_address)-1);
-	ia_css_device_store_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data), value);
-	return;
-}
-
-STORAGE_CLASS_MMU_H hrt_data mmu_reg_load(
-	const mmu_ID_t		ID,
-	const unsigned int	reg)
-{
-	assert(ID < N_MMU_ID);
-	assert(MMU_BASE[ID] != (hrt_address)-1);
-	return ia_css_device_load_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data));
-}
-
-#endif /* __MMU_PRIVATE_H_INCLUDED__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
index 0a13eda73607..637e136c4fc5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
@@ -62,7 +62,7 @@ extern void mmu_invalidate_cache_all(void);
 
  \return none, MMU[ID].ctrl[reg] = value
  */
-STORAGE_CLASS_MMU_H void mmu_reg_store(
+extern void mmu_reg_store(
 	const mmu_ID_t		ID,
 	const unsigned int	reg,
 	const hrt_data		value);
@@ -75,7 +75,7 @@ STORAGE_CLASS_MMU_H void mmu_reg_store(
 
  \return MMU[ID].ctrl[reg]
  */
-STORAGE_CLASS_MMU_H hrt_data mmu_reg_load(
+extern hrt_data mmu_reg_load(
 	const mmu_ID_t		ID,
 	const unsigned int	reg);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
index 519e850ec390..8f6f1dc40095 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
@@ -35,14 +35,6 @@
 #include "system_local.h"
 #include "mmu_local.h"
 
-#ifndef __INLINE_MMU__
-#define STORAGE_CLASS_MMU_H extern
-#define STORAGE_CLASS_MMU_C 
 #include "mmu_public.h"
-#else  /* __INLINE_MMU__ */
-#define STORAGE_CLASS_MMU_H static inline
-#define STORAGE_CLASS_MMU_C static inline
-#include "mmu_private.h"
-#endif /* __INLINE_MMU__ */
 
 #endif /* __MMU_DEVICE_H_INCLUDED__ */
-- 
2.14.3

Re: [PATCH 02/17] media: staging: atomisp: don't declare the same vars as both private and public

[kbuild test robot]

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

Hi Mauro,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20180411]
[also build test ERROR on v4.16]
[cannot apply to linuxtv-media/master staging/staging-testing v4.16 v4.16-rc7 v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/media-staging-atomisp-fix-number-conversion/20180414-000700
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.o: In function `mmu_set_page_table_base_index':
>> mmu.c:(.text+0x10): undefined reference to `mmu_reg_store'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.o: In function `mmu_get_page_table_base_index':
>> mmu.c:(.text+0x2e): undefined reference to `mmu_reg_load'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.o: In function `mmu_invalidate_cache':
   mmu.c:(.text+0x50): undefined reference to `mmu_reg_store'
   drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.o: In function `mmu_invalidate_cache_all':
   mmu.c:(.text+0x72): undefined reference to `mmu_reg_store'
   mmu.c:(.text+0x83): undefined reference to `mmu_reg_store'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62294 bytes --]

[PATCH 03/17] media: atomisp: fix __user annotations

[Mauro Carvalho Chehab]

There are lots of troubles with atomisp __user annotations. Fix them.

drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c:357:49: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c:357:49:    expected void *userptr
drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c:357:49:    got void [noderef] <asn:1>*user_ptr
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:3302:43: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:3302:43:    expected void const [noderef] <asn:1>*from
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:3302:43:    got void const *from
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4070:58: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4070:58:    expected void const *from
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4070:58:    got unsigned short [noderef] <asn:1>*<noident>
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4082:58: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4082:58:    expected void const *from
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:4082:58:    got unsigned short [noderef] <asn:1>*<noident>
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:6179:62: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:6179:62:    expected void const [noderef] <asn:1>*from
drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c:6179:62:    got unsigned short [usertype] *<noident>

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../media/atomisp/pci/atomisp2/atomisp_acc.c       |  8 ++++----
 .../media/atomisp/pci/atomisp2/atomisp_cmd.c       |  9 +++++----
 .../media/atomisp/pci/atomisp2/atomisp_compat.h    |  2 +-
 .../atomisp/pci/atomisp2/atomisp_compat_css20.c    |  2 +-
 .../media/atomisp/pci/atomisp2/atomisp_ioctl.c     |  2 +-
 .../memory_access/memory_access.h                  |  2 +-
 .../pci/atomisp2/css2400/ia_css_frame_public.h     |  2 +-
 .../pci/atomisp2/css2400/ia_css_memory_access.c    |  4 ++--
 .../pci/atomisp2/css2400/runtime/frame/src/frame.c |  2 +-
 .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   |  2 +-
 .../media/atomisp/pci/atomisp2/hmm/hmm_bo.c        |  4 ++--
 .../atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.c | 22 ++++++++++++----------
 .../atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.h | 11 ++++++-----
 .../media/atomisp/pci/atomisp2/include/hmm/hmm.h   |  2 +-
 .../atomisp/pci/atomisp2/include/hmm/hmm_bo.h      |  2 +-
 15 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c
index a6638edee360..7ebcebd80b77 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_acc.c
@@ -353,10 +353,10 @@ int atomisp_acc_map(struct atomisp_sub_device *asd, struct atomisp_acc_map *map)
 		}
 
 		pgnr = DIV_ROUND_UP(map->length, PAGE_SIZE);
-		cssptr = hrt_isp_css_mm_alloc_user_ptr(
-				map->length, map->user_ptr,
-				pgnr, HRT_USR_PTR,
-				(map->flags & ATOMISP_MAP_FLAG_CACHED));
+		cssptr = hrt_isp_css_mm_alloc_user_ptr(map->length,
+						       map->user_ptr,
+						       pgnr, HRT_USR_PTR,
+						       (map->flags & ATOMISP_MAP_FLAG_CACHED));
 	} else {
 		/* Allocate private buffer. */
 		if (map->flags & ATOMISP_MAP_FLAG_CACHED)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index fa6ea506f8b1..874165654850 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -3299,7 +3299,7 @@ static unsigned int long copy_from_compatible(void *to, const void *from,
 					      unsigned long n, bool from_user)
 {
 	if (from_user)
-		return copy_from_user(to, from, n);
+		return copy_from_user(to, (void __user *)from, n);
 	else
 		memcpy(to, from, n);
 	return 0;
@@ -4067,7 +4067,7 @@ int atomisp_cp_morph_table(struct atomisp_sub_device *asd,
 
 	for (i = 0; i < CSS_MORPH_TABLE_NUM_PLANES; i++) {
 		if (copy_from_compatible(morph_table->coordinates_x[i],
-			source_morph_table->coordinates_x[i],
+			(__force void *)source_morph_table->coordinates_x[i],
 #ifndef ISP2401
 			source_morph_table->height * source_morph_table->width *
 			sizeof(*source_morph_table->coordinates_x[i]),
@@ -4079,7 +4079,7 @@ int atomisp_cp_morph_table(struct atomisp_sub_device *asd,
 			goto error;
 
 		if (copy_from_compatible(morph_table->coordinates_y[i],
-			source_morph_table->coordinates_y[i],
+			(__force void *)source_morph_table->coordinates_y[i],
 #ifndef ISP2401
 			source_morph_table->height * source_morph_table->width *
 			sizeof(*source_morph_table->coordinates_y[i]),
@@ -6176,7 +6176,8 @@ int atomisp_set_shading_table(struct atomisp_sub_device *asd,
 		    ATOMISP_SC_TYPE_SIZE;
 	for (i = 0; i < ATOMISP_NUM_SC_COLORS; i++) {
 		ret = copy_from_user(shading_table->data[i],
-				     user_shading_table->data[i], len_table);
+				     (void __user *)user_shading_table->data[i],
+				     len_table);
 		if (ret) {
 			free_table = shading_table;
 			ret = -EFAULT;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
index 6c829d0a1e4c..aac0eccee798 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
@@ -353,7 +353,7 @@ void atomisp_css_frame_free(struct atomisp_css_frame *frame);
 
 int atomisp_css_frame_map(struct atomisp_css_frame **frame,
 				const struct atomisp_css_frame_info *info,
-				const void *data, uint16_t attribute,
+				const void __user *data, uint16_t attribute,
 				void *context);
 
 int atomisp_css_set_black_frame(struct atomisp_sub_device *asd,
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
index f668c68dc33a..df88d9df2027 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
@@ -2189,7 +2189,7 @@ void atomisp_css_frame_free(struct atomisp_css_frame *frame)
 
 int atomisp_css_frame_map(struct atomisp_css_frame **frame,
 				const struct atomisp_css_frame_info *info,
-				const void *data, uint16_t attribute,
+				const void __user *data, uint16_t attribute,
 				void *context)
 {
 	if (ia_css_frame_map(frame, info, data, attribute, context)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
index 61bd550dafb9..6e7231243891 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
@@ -1253,7 +1253,7 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		attributes.type = HRT_USR_PTR;
 #endif
 		ret = atomisp_css_frame_map(&handle, &frame_info,
-				       (void *)buf->m.userptr,
+				       (void __user *)buf->m.userptr,
 				       0, &attributes);
 		if (ret) {
 			dev_err(isp->dev, "Failed to map user buffer\n");
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/memory_access/memory_access.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/memory_access/memory_access.h
index 195c4a5bceeb..d2387812f3a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/memory_access/memory_access.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/memory_access/memory_access.h
@@ -137,7 +137,7 @@ extern hrt_vaddress mmgr_alloc_attr(const size_t size, const uint16_t attribute)
  \return vaddress
  */
 extern hrt_vaddress mmgr_mmap(
-	const void *ptr,
+	const void __user *ptr,
 	const size_t size,
 	uint16_t attribute,
 	void *context);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
index 0beb7347a4f3..89943e8bf180 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
@@ -333,7 +333,7 @@ ia_css_frame_set_data(struct ia_css_frame *frame,
 enum ia_css_err
 ia_css_frame_map(struct ia_css_frame **frame,
 		 const struct ia_css_frame_info *info,
-		 const void *data,
+		 const void __user *data,
 		 uint16_t attribute,
 		 void *context);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_memory_access.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_memory_access.c
index 282075942ba6..8222dd0a41f2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_memory_access.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_memory_access.c
@@ -72,12 +72,12 @@ mmgr_store(const hrt_vaddress vaddr, const void *data, const size_t size)
 }
 
 hrt_vaddress
-mmgr_mmap(const void *ptr, const size_t size,
+mmgr_mmap(const void __user *ptr, const size_t size,
 	  uint16_t attribute, void *context)
 {
 	struct hrt_userbuffer_attr *userbuffer_attr = context;
 	return hrt_isp_css_mm_alloc_user_ptr(
-			size, (void *)ptr, userbuffer_attr->pgnr,
+			size, ptr, userbuffer_attr->pgnr,
 			userbuffer_attr->type,
 			attribute & HRT_BUF_FLAG_CACHED);
 }
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
index 7562beadabef..fd8e6fda5db4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/frame/src/frame.c
@@ -175,7 +175,7 @@ enum ia_css_err ia_css_frame_allocate(struct ia_css_frame **frame,
 
 enum ia_css_err ia_css_frame_map(struct ia_css_frame **frame,
 	const struct ia_css_frame_info *info,
-	const void *data,
+	const void __user *data,
 	uint16_t attribute,
 	void *context)
 {
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
index 4338b8a1309f..15bc10b5e9b1 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c
@@ -219,7 +219,7 @@ void hmm_cleanup(void)
 }
 
 ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
-		     int from_highmem, void *userptr, bool cached)
+		     int from_highmem, const void __user *userptr, bool cached)
 {
 	unsigned int pgnr;
 	struct hmm_buffer_object *bo;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
index 79bd540d7882..c888f9c809f9 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
@@ -977,7 +977,7 @@ static int get_pfnmap_pages(struct task_struct *tsk, struct mm_struct *mm,
  * Convert user space virtual address into pages list
  */
 static int alloc_user_pages(struct hmm_buffer_object *bo,
-			      void *userptr, bool cached)
+			    const void __user *userptr, bool cached)
 {
 	int page_nr;
 	int i;
@@ -1081,7 +1081,7 @@ static void free_user_pages(struct hmm_buffer_object *bo)
  */
 int hmm_bo_alloc_pages(struct hmm_buffer_object *bo,
 		       enum hmm_bo_type type, int from_highmem,
-		       void *userptr, bool cached)
+		       const void __user *userptr, bool cached)
 {
 	int ret = -EINVAL;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.c b/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.c
index a94958bde718..9b186517f20a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.c
@@ -24,11 +24,11 @@
 
 #define __page_align(size)	(((size) + (PAGE_SIZE-1)) & (~(PAGE_SIZE-1)))
 
-static void *my_userptr;
+static void __user *my_userptr;
 static unsigned my_num_pages;
 static enum hrt_userptr_type my_usr_type;
 
-void hrt_isp_css_mm_set_user_ptr(void *userptr,
+void hrt_isp_css_mm_set_user_ptr(void __user *userptr,
 				 unsigned int num_pages,
 				 enum hrt_userptr_type type)
 {
@@ -37,10 +37,11 @@ void hrt_isp_css_mm_set_user_ptr(void *userptr,
 	my_usr_type = type;
 }
 
-static ia_css_ptr __hrt_isp_css_mm_alloc(size_t bytes, void *userptr,
-				    unsigned int num_pages,
-				    enum hrt_userptr_type type,
-				    bool cached)
+static ia_css_ptr __hrt_isp_css_mm_alloc(size_t bytes,
+					 const void __user *userptr,
+					 unsigned int num_pages,
+					 enum hrt_userptr_type type,
+					 bool cached)
 {
 #ifdef CONFIG_ION
 	if (type == HRT_USR_ION)
@@ -78,10 +79,11 @@ ia_css_ptr hrt_isp_css_mm_alloc(size_t bytes)
 				      my_num_pages, my_usr_type, false);
 }
 
-ia_css_ptr hrt_isp_css_mm_alloc_user_ptr(size_t bytes, void *userptr,
-				    unsigned int num_pages,
-				    enum hrt_userptr_type type,
-				    bool cached)
+ia_css_ptr hrt_isp_css_mm_alloc_user_ptr(size_t bytes,
+					 const void __user *userptr,
+					 unsigned int num_pages,
+					 enum hrt_userptr_type type,
+					 bool cached)
 {
 	return __hrt_isp_css_mm_alloc(bytes, userptr, num_pages,
 				      type, cached);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.h b/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.h
index 15c2dfb6794e..93762e71b4ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hrt/hive_isp_css_mm_hrt.h
@@ -37,15 +37,16 @@ struct hrt_userbuffer_attr {
 	unsigned int		pgnr;
 };
 
-void hrt_isp_css_mm_set_user_ptr(void *userptr,
+void hrt_isp_css_mm_set_user_ptr(void __user *userptr,
 				unsigned int num_pages, enum hrt_userptr_type);
 
 /* Allocate memory, returns a virtual address */
 ia_css_ptr hrt_isp_css_mm_alloc(size_t bytes);
-ia_css_ptr hrt_isp_css_mm_alloc_user_ptr(size_t bytes, void *userptr,
-				    unsigned int num_pages,
-				    enum hrt_userptr_type,
-				    bool cached);
+ia_css_ptr hrt_isp_css_mm_alloc_user_ptr(size_t bytes,
+					 const void __user *userptr,
+					 unsigned int num_pages,
+					 enum hrt_userptr_type,
+					 bool cached);
 ia_css_ptr hrt_isp_css_mm_alloc_cached(size_t bytes);
 
 /* allocate memory and initialize with zeros,
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm.h b/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm.h
index 1e135c7c6d9b..7dcc73c9f49d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm.h
@@ -38,7 +38,7 @@ int hmm_init(void);
 void hmm_cleanup(void);
 
 ia_css_ptr hmm_alloc(size_t bytes, enum hmm_bo_type type,
-		int from_highmem, void *userptr, bool cached);
+		int from_highmem, const void __user *userptr, bool cached);
 void hmm_free(ia_css_ptr ptr);
 int hmm_load(ia_css_ptr virt, void *data, unsigned int bytes);
 int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm_bo.h
index bd44ebbc427c..508d6fd68f93 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm_bo.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/include/hmm/hmm_bo.h
@@ -244,7 +244,7 @@ int hmm_bo_allocated(struct hmm_buffer_object *bo);
  */
 int hmm_bo_alloc_pages(struct hmm_buffer_object *bo,
 		enum hmm_bo_type type, int from_highmem,
-		void *userptr, bool cached);
+		const void __user *userptr, bool cached);
 void hmm_bo_free_pages(struct hmm_buffer_object *bo);
 int hmm_bo_page_allocated(struct hmm_buffer_object *bo);
 
-- 
2.14.3

Re: [PATCH 03/17] media: atomisp: fix __user annotations

[kbuild test robot]

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

Hi Mauro,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20180411]
[also build test ERROR on v4.16]
[cannot apply to linuxtv-media/master staging/staging-testing v4.16 v4.16-rc7 v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mauro-Carvalho-Chehab/media-staging-atomisp-fix-number-conversion/20180414-000700
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Mauro-Carvalho-Chehab/media-staging-atomisp-fix-number-conversion/20180414-000700 HEAD 3b98aee6f72133addf14b8bc4fc71c2978bf81f9 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

>> ERROR: "mmu_reg_store" [drivers/staging/media/atomisp/pci/atomisp2/atomisp.ko] undefined!
>> ERROR: "mmu_reg_load" [drivers/staging/media/atomisp/pci/atomisp2/atomisp.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63138 bytes --]

[PATCHv2 02/17] media: staging: atomisp: don't declare the same vars as both private and public

[Mauro Carvalho Chehab]

The mmu_private.h header is included at mmu.c, with duplicates the
already existing definitions at mmu_public.h.

Fix this by removing the erroneous header file.

Solve those issues:

    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:24:26: warning: function 'mmu_reg_store' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:35:30: warning: function 'mmu_reg_load' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:24:26: warning: function 'mmu_reg_store' with external linkage has definition
    drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h:35:30: warning: function 'mmu_reg_load' with external linkage has definition

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---

v2: re-added two function definitions that were at the mmu_private.h

 .../css2400/hive_isp_css_common/host/mmu.c         |  4 --
 .../css2400/hive_isp_css_common/host/mmu_private.h | 44 ----------------------
 .../css2400/hive_isp_css_include/host/mmu_public.h | 22 +++++++++--
 .../css2400/hive_isp_css_include/mmu_device.h      |  8 ----
 4 files changed, 18 insertions(+), 60 deletions(-)
 delete mode 100644 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
index a28b67eb66ea..1a1719d3e745 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu.c
@@ -15,10 +15,6 @@
 /* The name "mmu.h is already taken" */
 #include "mmu_device.h"
 
-#ifndef __INLINE_MMU__
-#include "mmu_private.h"
-#endif /* __INLINE_MMU__ */
-
 void mmu_set_page_table_base_index(
 	const mmu_ID_t		ID,
 	const hrt_data		base_index)
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h
deleted file mode 100644
index 7377666f6eb7..000000000000
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/mmu_private.h
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * Support for Intel Camera Imaging ISP subsystem.
- * Copyright (c) 2010-2015, Intel Corporation.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- */
-
-#ifndef __MMU_PRIVATE_H_INCLUDED__
-#define __MMU_PRIVATE_H_INCLUDED__
-
-#include "mmu_public.h"
-
-#include "device_access.h"
-
-#include "assert_support.h"
-
-STORAGE_CLASS_MMU_H void mmu_reg_store(
-	const mmu_ID_t		ID,
-	const unsigned int	reg,
-	const hrt_data		value)
-{
-	assert(ID < N_MMU_ID);
-	assert(MMU_BASE[ID] != (hrt_address)-1);
-	ia_css_device_store_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data), value);
-	return;
-}
-
-STORAGE_CLASS_MMU_H hrt_data mmu_reg_load(
-	const mmu_ID_t		ID,
-	const unsigned int	reg)
-{
-	assert(ID < N_MMU_ID);
-	assert(MMU_BASE[ID] != (hrt_address)-1);
-	return ia_css_device_load_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data));
-}
-
-#endif /* __MMU_PRIVATE_H_INCLUDED__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
index 0a13eda73607..bbff4128603b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/host/mmu_public.h
@@ -16,6 +16,8 @@
 #define __MMU_PUBLIC_H_INCLUDED__
 
 #include "system_types.h"
+#include "device_access.h"
+#include "assert_support.h"
 
 /*! Set the page table base index of MMU[ID]
 
@@ -62,10 +64,17 @@ extern void mmu_invalidate_cache_all(void);
 
  \return none, MMU[ID].ctrl[reg] = value
  */
-STORAGE_CLASS_MMU_H void mmu_reg_store(
+static inline void mmu_reg_store(
 	const mmu_ID_t		ID,
 	const unsigned int	reg,
-	const hrt_data		value);
+	const hrt_data		value)
+{
+	assert(ID < N_MMU_ID);
+	assert(MMU_BASE[ID] != (hrt_address)-1);
+	ia_css_device_store_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data), value);
+	return;
+}
+
 
 /*! Read from a control register of MMU[ID]
 
@@ -75,8 +84,13 @@ STORAGE_CLASS_MMU_H void mmu_reg_store(
 
  \return MMU[ID].ctrl[reg]
  */
-STORAGE_CLASS_MMU_H hrt_data mmu_reg_load(
+static inline hrt_data mmu_reg_load(
 	const mmu_ID_t		ID,
-	const unsigned int	reg);
+	const unsigned int	reg)
+{
+	assert(ID < N_MMU_ID);
+	assert(MMU_BASE[ID] != (hrt_address)-1);
+	return ia_css_device_load_uint32(MMU_BASE[ID] + reg*sizeof(hrt_data));
+}
 
 #endif /* __MMU_PUBLIC_H_INCLUDED__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
index 519e850ec390..8f6f1dc40095 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/mmu_device.h
@@ -35,14 +35,6 @@
 #include "system_local.h"
 #include "mmu_local.h"
 
-#ifndef __INLINE_MMU__
-#define STORAGE_CLASS_MMU_H extern
-#define STORAGE_CLASS_MMU_C 
 #include "mmu_public.h"
-#else  /* __INLINE_MMU__ */
-#define STORAGE_CLASS_MMU_H static inline
-#define STORAGE_CLASS_MMU_C static inline
-#include "mmu_private.h"
-#endif /* __INLINE_MMU__ */
 
 #endif /* __MMU_DEVICE_H_INCLUDED__ */
-- 
2.14.3

[PATCH 04/17] media: staging: atomisp: fix string comparation logic

[Mauro Carvalho Chehab]

it makes no sense to use strncmp() with a size with is
bigger than the string we're comparing with.

Fix those warnings:

    drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c:776 atomisp_open() error: strncmp() '"ATOMISP ISP ACC"' too small (16 vs 32)
    drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c:913 atomisp_release() error: strncmp() '"ATOMISP ISP ACC"' too small (16 vs 32)
    drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:2751 atomisp_vidioc_default() error: strncmp() '"ATOMISP ISP ACC"' too small (16 vs 32)

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c  | 6 ++----
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
index 709137f25700..693b905547e4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
@@ -773,8 +773,7 @@ static int atomisp_open(struct file *file)
 
 	rt_mutex_lock(&isp->mutex);
 
-	acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
-			sizeof(vdev->name));
+	acc_node = !strcmp(vdev->name, "ATOMISP ISP ACC");
 	if (acc_node) {
 		acc_pipe = atomisp_to_acc_pipe(vdev);
 		asd = acc_pipe->asd;
@@ -910,8 +909,7 @@ static int atomisp_release(struct file *file)
 	rt_mutex_lock(&isp->mutex);
 
 	dev_dbg(isp->dev, "release device %s\n", vdev->name);
-	acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
-			sizeof(vdev->name));
+	acc_node = !strcmp(vdev->name, "ATOMISP ISP ACC");
 	if (acc_node) {
 		acc_pipe = atomisp_to_acc_pipe(vdev);
 		asd = acc_pipe->asd;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
index 6e7231243891..8c67aea67b6b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
@@ -2748,8 +2748,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 	bool acc_node;
 	int err;
 
-	acc_node = !strncmp(vdev->name, "ATOMISP ISP ACC",
-			sizeof(vdev->name));
+	acc_node = !strcmp(vdev->name, "ATOMISP ISP ACC");
 	if (acc_node)
 		asd = atomisp_to_acc_pipe(vdev)->asd;
 	else
-- 
2.14.3

[PATCH 05/17] dvb_frontend: fix locking issues at dvb_frontend_get_event()

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/dvb-core/dvb_frontend.c:314 dvb_frontend_get_event() warn: inconsistent returns 'sem:&fepriv->sem'.
	  Locked on:   line 288
	               line 295
	               line 306
	               line 314
	  Unlocked on: line 303

The lock implementation for get event is wrong, as, if an
interrupt occurs, down_interruptible() will fail, and the
routine will call up() twice when userspace calls the ioctl
again.

The bad code is there since when Linux migrated to git, in
2005.

Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e33414975065..a4ada1ccf0df 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -275,8 +275,20 @@ static void dvb_frontend_add_event(struct dvb_frontend *fe,
 	wake_up_interruptible (&events->wait_queue);
 }
 
+static int dvb_frontend_test_event(struct dvb_frontend_private *fepriv,
+				   struct dvb_fe_events *events)
+{
+	int ret;
+
+	up(&fepriv->sem);
+	ret = events->eventw != events->eventr;
+	down(&fepriv->sem);
+
+	return ret;
+}
+
 static int dvb_frontend_get_event(struct dvb_frontend *fe,
-			    struct dvb_frontend_event *event, int flags)
+			          struct dvb_frontend_event *event, int flags)
 {
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dvb_fe_events *events = &fepriv->events;
@@ -294,13 +306,8 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe,
 		if (flags & O_NONBLOCK)
 			return -EWOULDBLOCK;
 
-		up(&fepriv->sem);
-
-		ret = wait_event_interruptible (events->wait_queue,
-						events->eventw != events->eventr);
-
-		if (down_interruptible (&fepriv->sem))
-			return -ERESTARTSYS;
+		ret = wait_event_interruptible(events->wait_queue,
+					       dvb_frontend_test_event(fepriv, events));
 
 		if (ret < 0)
 			return ret;
-- 
2.14.3

[PATCH 05/17] dvb_frontend: fix locking issues at dvb_frontend_get_event()

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/dvb-core/dvb_frontend.c:314 dvb_frontend_get_event() warn: inconsistent returns 'sem:&fepriv->sem'.
	  Locked on:   line 288
	               line 295
	               line 306
	               line 314
	  Unlocked on: line 303

The lock implementation for get event is wrong, as, if an
interrupt occurs, down_interruptible() will fail, and the
routine will call up() twice when userspace calls the ioctl
again.

The bad code is there since when Linux migrated to git, in
2005.

Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/dvb-core/dvb_frontend.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index e33414975065..a4ada1ccf0df 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -275,8 +275,20 @@ static void dvb_frontend_add_event(struct dvb_frontend *fe,
 	wake_up_interruptible (&events->wait_queue);
 }
 
+static int dvb_frontend_test_event(struct dvb_frontend_private *fepriv,
+				   struct dvb_fe_events *events)
+{
+	int ret;
+
+	up(&fepriv->sem);
+	ret = events->eventw != events->eventr;
+	down(&fepriv->sem);
+
+	return ret;
+}
+
 static int dvb_frontend_get_event(struct dvb_frontend *fe,
-			    struct dvb_frontend_event *event, int flags)
+			          struct dvb_frontend_event *event, int flags)
 {
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	struct dvb_fe_events *events = &fepriv->events;
@@ -294,13 +306,8 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe,
 		if (flags & O_NONBLOCK)
 			return -EWOULDBLOCK;
 
-		up(&fepriv->sem);
-
-		ret = wait_event_interruptible (events->wait_queue,
-						events->eventw != events->eventr);
-
-		if (down_interruptible (&fepriv->sem))
-			return -ERESTARTSYS;
+		ret = wait_event_interruptible(events->wait_queue,
+					       dvb_frontend_test_event(fepriv, events));
 
 		if (ret < 0)
 			return ret;
-- 
2.14.3

[PATCH 06/17] media: v4l2-fwnode: simplify v4l2_fwnode_reference_parse_int_props()

[Mauro Carvalho Chehab]

The logic at v4l2_fwnode_reference_parse_int_props() is somewhat
complex and violates Linux coding style, as it does multiple
statements on a single line. That makes static analyzers to
be confused, as warned by smatch:

	drivers/media/v4l2-core/v4l2-fwnode.c:832 v4l2_fwnode_reference_parse_int_props() warn: passing zero to 'PTR_ERR'

Simplify the logic, in order to make clearer about what happens
when v4l2_fwnode_reference_get_int_prop() returns an error.

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

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d630640642ee..3f77aa318035 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -819,17 +819,25 @@ static int v4l2_fwnode_reference_parse_int_props(
 	unsigned int index;
 	int ret;
 
-	for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop(
-					 dev_fwnode(dev), prop, index, props,
-					 nprops))); index++)
+	index = 0;
+	do {
+		fwnode = v4l2_fwnode_reference_get_int_prop(dev_fwnode(dev),
+							    prop, index,
+							    props, nprops);
+		if (IS_ERR(fwnode)) {
+			/*
+			 * Note that right now both -ENODATA and -ENOENT may
+			 * signal out-of-bounds access. Return the error in
+			 * cases other than that.
+			 */
+			if (PTR_ERR(fwnode) != -ENOENT &&
+			    PTR_ERR(fwnode) != -ENODATA)
+				return PTR_ERR(fwnode);
+			break;
+		}
 		fwnode_handle_put(fwnode);
-
-	/*
-	 * Note that right now both -ENODATA and -ENOENT may signal
-	 * out-of-bounds access. Return the error in cases other than that.
-	 */
-	if (PTR_ERR(fwnode) != -ENOENT && PTR_ERR(fwnode) != -ENODATA)
-		return PTR_ERR(fwnode);
+		index++;
+	} while (1);
 
 	ret = v4l2_async_notifier_realloc(notifier,
 					  notifier->num_subdevs + index);
-- 
2.14.3

[PATCH 07/17] media: cec: fix smatch error

[Mauro Carvalho Chehab]

From: Hans Verkuil <hverkuil@xs4all.nl>

drivers/media/cec/cec-pin-error-inj.c:231
cec_pin_error_inj_parse_line() error: uninitialized symbol 'pos'.

The tx-add-bytes command didn't check for the presence of an argument, and
also didn't check that it was > 0.

This should fix this error.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/cec/cec-pin-error-inj.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/cec/cec-pin-error-inj.c b/drivers/media/cec/cec-pin-error-inj.c
index aaa899a175ce..7132a2758bd3 100644
--- a/drivers/media/cec/cec-pin-error-inj.c
+++ b/drivers/media/cec/cec-pin-error-inj.c
@@ -203,16 +203,18 @@ bool cec_pin_error_inj_parse_line(struct cec_adapter *adap, char *line)
 		mode_mask = CEC_ERROR_INJ_MODE_MASK << mode_offset;
 		arg_idx = cec_error_inj_cmds[i].arg_idx;
 
-		if (mode_offset == CEC_ERROR_INJ_RX_ARB_LOST_OFFSET ||
-		    mode_offset == CEC_ERROR_INJ_TX_ADD_BYTES_OFFSET)
-			is_bit_pos = false;
-
 		if (mode_offset == CEC_ERROR_INJ_RX_ARB_LOST_OFFSET) {
 			if (has_op)
 				return false;
 			if (!has_pos)
 				pos = 0x0f;
+			is_bit_pos = false;
+		} else if (mode_offset == CEC_ERROR_INJ_TX_ADD_BYTES_OFFSET) {
+			if (!has_pos || !pos)
+				return false;
+			is_bit_pos = false;
 		}
+
 		if (arg_idx >= 0 && is_bit_pos) {
 			if (!has_pos || pos >= 160)
 				return false;
-- 
2.14.3

[PATCH 08/17] atomisp: remove an impossible condition

[Mauro Carvalho Chehab]

Changeset dc9f65cf9aea ("media: staging: atomisp: avoid a warning if 32
bits build") was meant to solve an impossible condition when building
with 32 bits. It turns that this impossible condition also happens wit
64 bits:
	drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c:698 gmin_get_config_var() warn: impossible condition '(*out_len > (~0)) => (0-u64max > u64max)'

After a further analysis, this condition will always be false as, on
all architectures, size_t doesn't have more bits than unsigned long.

Also, the only two archs that really matter are x86 and x86_64, as this
driver doesn't build on other archs (as it depends on X86-specific UEFI
support).

So, just drop the useless code.

Fixes: dc9f65cf9aea ("media: staging: atomisp: avoid a warning if 32 bits build")
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../media/atomisp/platform/intel-mid/atomisp_gmin_platform.c        | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 3283c1b05d6a..70c34de98707 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -693,12 +693,6 @@ static int gmin_get_config_var(struct device *dev, const char *var,
 	for (i = 0; i < sizeof(var8) && var8[i]; i++)
 		var16[i] = var8[i];
 
-#ifdef CONFIG_64BIT
-	/* To avoid owerflows when calling the efivar API */
-	if (*out_len > ULONG_MAX)
-		return -EINVAL;
-#endif
-
 	/* Not sure this API usage is kosher; efivar_entry_get()'s
 	 * implementation simply uses VariableName and VendorGuid from
 	 * the struct and ignores the rest, but it seems like there
-- 
2.14.3

[PATCH 09/17] media: platform: fix some 64-bits warnings

[Mauro Carvalho Chehab]

The omap/omap3 and viu drivers are for 32 bit platforms only.
There, a pointer has 32 bits. Now that those drivers build
for 64 bits with COMPILE_TEST, they produce the following
warnings:

drivers/media/platform/omap/omap_vout_vrfb.c: In function 'omap_vout_allocate_vrfb_buffers':
drivers/media/platform/omap/omap_vout_vrfb.c:57:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   memset((void *) vout->smsshado_virt_addr[i], 0,
          ^
drivers/media/platform/fsl-viu.c: In function 'viu_setup_preview':
drivers/media/platform/fsl-viu.c:753:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  reg_val.field_base_addr = (u32)dev->ovbuf.base;
                            ^
drivers/media/platform/omap/omap_vout.c: In function 'omap_vout_get_userptr':
drivers/media/platform/omap/omap_vout.c:209:25: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   *physp = virt_to_phys((void *)virtp);
                         ^
drivers/media/platform/omap3isp/ispccdc.c: In function 'ccdc_config':
drivers/media/platform/omap3isp/ispccdc.c:738:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
         (__force void __user *)fpc.fpcaddr,
         ^

Add some typecasts to remove those warnings when building for
64 bits.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/platform/fsl-viu.c             | 2 +-
 drivers/media/platform/omap/omap_vout.c      | 2 +-
 drivers/media/platform/omap/omap_vout_vrfb.c | 4 ++--
 drivers/media/platform/omap3isp/ispccdc.c    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/fsl-viu.c b/drivers/media/platform/fsl-viu.c
index 5b6bfcafc2a4..e41510ce69a4 100644
--- a/drivers/media/platform/fsl-viu.c
+++ b/drivers/media/platform/fsl-viu.c
@@ -750,7 +750,7 @@ static int viu_setup_preview(struct viu_dev *dev, struct viu_fh *fh)
 	reg_val.status_cfg |= DMA_ACT | INT_DMA_END_EN | INT_FIELD_EN;
 
 	/* setup the base address of the overlay buffer */
-	reg_val.field_base_addr = (u32)dev->ovbuf.base;
+	reg_val.field_base_addr = (u32)(long)dev->ovbuf.base;
 
 	return 0;
 }
diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c
index e2723fedac8d..5700b7818621 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -198,7 +198,7 @@ static int omap_vout_try_format(struct v4l2_pix_format *pix)
  * omap_vout_get_userptr: Convert user space virtual address to physical
  * address.
  */
-static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp,
+static int omap_vout_get_userptr(struct videobuf_buffer *vb, long virtp,
 				 u32 *physp)
 {
 	struct frame_vector *vec;
diff --git a/drivers/media/platform/omap/omap_vout_vrfb.c b/drivers/media/platform/omap/omap_vout_vrfb.c
index 1d8508237220..29e3f5da59c1 100644
--- a/drivers/media/platform/omap/omap_vout_vrfb.c
+++ b/drivers/media/platform/omap/omap_vout_vrfb.c
@@ -54,8 +54,8 @@ static int omap_vout_allocate_vrfb_buffers(struct omap_vout_device *vout,
 			*count = 0;
 			return -ENOMEM;
 		}
-		memset((void *) vout->smsshado_virt_addr[i], 0,
-				vout->smsshado_size);
+		memset((void *)(long)vout->smsshado_virt_addr[i], 0,
+		       vout->smsshado_size);
 	}
 	return 0;
 }
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index b66276ab5765..77b73e27a274 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -735,7 +735,7 @@ static int ccdc_config(struct isp_ccdc_device *ccdc,
 				return -ENOMEM;
 
 			if (copy_from_user(fpc_new.addr,
-					   (__force void __user *)fpc.fpcaddr,
+					   (__force void __user *)(long)fpc.fpcaddr,
 					   size)) {
 				dma_free_coherent(isp->dev, size, fpc_new.addr,
 						  fpc_new.dma);
-- 
2.14.3

[PATCH 10/17] media: v4l2-compat-ioctl32: prevent go past max size

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/v4l2-core/v4l2-compat-ioctl32.c:879 put_v4l2_ext_controls32() warn: check for integer overflow 'count'

The access_ok() logic should check for too big arrays too.

Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 4312935f1dfc..d03a44d89649 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -871,7 +871,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 	    get_user(kcontrols, &kp->controls))
 		return -EFAULT;
 
-	if (!count)
+	if (!count || count > (U32_MAX/sizeof(*ucontrols)))
 		return 0;
 	if (get_user(p, &up->controls))
 		return -EFAULT;
-- 
2.14.3

[PATCH 10/17] media: v4l2-compat-ioctl32: prevent go past max size

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/v4l2-core/v4l2-compat-ioctl32.c:879 put_v4l2_ext_controls32() warn: check for integer overflow 'count'

The access_ok() logic should check for too big arrays too.

Cc: stable@vger.kernel.org
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 4312935f1dfc..d03a44d89649 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -871,7 +871,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 	    get_user(kcontrols, &kp->controls))
 		return -EFAULT;
 
-	if (!count)
+	if (!count || count > (U32_MAX/sizeof(*ucontrols)))
 		return 0;
 	if (get_user(p, &up->controls))
 		return -EFAULT;
-- 
2.14.3

[PATCH 11/17] media: atomisp: compat32: use get_user() before referencing user data

[Mauro Carvalho Chehab]

The logic at get_atomisp_parameters32() is broken, as pointed by
smatch:

	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:737:21: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:744:60: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:763:21: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:770:60: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:788:21: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:795:60: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:812:21: warning: dereference of noderef expression
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:819:60: warning: dereference of noderef expression

It tries to access userspace data directly, without calling
get_user(). That should generate OOPS. Thankfully, the right
logic is already there (although commented out).

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../atomisp/pci/atomisp2/atomisp_compat_ioctl32.c  | 38 ----------------------
 1 file changed, 38 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
index 44c21813a06e..d7c0ef1f9584 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
@@ -691,10 +691,8 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 				sizeof(compat_uptr_t);
 	unsigned int size, offset = 0;
 	void  __user *user_ptr;
-#ifdef ISP2401
 	unsigned int stp, mtp, dcp, dscp = 0;
 
-#endif
 	if (!access_ok(VERIFY_READ, up, sizeof(struct atomisp_parameters32)))
 			return -EFAULT;
 
@@ -707,15 +705,11 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 		n--;
 	}
 	if (get_user(kp->isp_config_id, &up->isp_config_id) ||
-#ifndef ISP2401
-	    get_user(kp->per_frame_setting, &up->per_frame_setting))
-#else
 	    get_user(kp->per_frame_setting, &up->per_frame_setting) ||
 	    get_user(stp, &up->shading_table) ||
 	    get_user(mtp, &up->morph_table) ||
 	    get_user(dcp, &up->dvs2_coefs) ||
 	    get_user(dscp, &up->dvs_6axis_config))
-#endif
 		return -EFAULT;
 
 	{
@@ -733,18 +727,10 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 		user_ptr = compat_alloc_user_space(size);
 
 		/* handle shading table */
-#ifndef ISP2401
-		if (up->shading_table != 0) {
-#else
 		if (stp != 0) {
-#endif
 			if (get_atomisp_shading_table32(&karg.shading_table,
 				(struct atomisp_shading_table32 __user *)
-#ifndef ISP2401
-						(uintptr_t)up->shading_table))
-#else
 						(uintptr_t)stp))
-#endif
 				return -EFAULT;
 
 			kp->shading_table = user_ptr + offset;
@@ -759,18 +745,10 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 		}
 
 		/* handle morph table */
-#ifndef ISP2401
-		if (up->morph_table != 0) {
-#else
 		if (mtp != 0) {
-#endif
 			if (get_atomisp_morph_table32(&karg.morph_table,
 					(struct atomisp_morph_table32 __user *)
-#ifndef ISP2401
-						(uintptr_t)up->morph_table))
-#else
 						(uintptr_t)mtp))
-#endif
 				return -EFAULT;
 
 			kp->morph_table = user_ptr + offset;
@@ -784,18 +762,10 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 		}
 
 		/* handle dvs2 coefficients */
-#ifndef ISP2401
-		if (up->dvs2_coefs != 0) {
-#else
 		if (dcp != 0) {
-#endif
 			if (get_atomisp_dis_coefficients32(&karg.dvs2_coefs,
 				(struct atomisp_dis_coefficients32 __user *)
-#ifndef ISP2401
-						(uintptr_t)up->dvs2_coefs))
-#else
 						(uintptr_t)dcp))
-#endif
 				return -EFAULT;
 
 			kp->dvs2_coefs = user_ptr + offset;
@@ -808,18 +778,10 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 				return -EFAULT;
 		}
 		/* handle dvs 6axis configuration */
-#ifndef ISP2401
-		if (up->dvs_6axis_config != 0) {
-#else
 		if (dscp != 0) {
-#endif
 			if (get_atomisp_dvs_6axis_config32(&karg.dvs_6axis_config,
 				(struct atomisp_dvs_6axis_config32 __user *)
-#ifndef ISP2401
-						(uintptr_t)up->dvs_6axis_config))
-#else
 						(uintptr_t)dscp))
-#endif
 				return -EFAULT;
 
 			kp->dvs_6axis_config = user_ptr + offset;
-- 
2.14.3

[PATCH 12/17] media: staging: atomisp: add missing include

[Mauro Carvalho Chehab]

There are two functions used externally:
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:866:6: warning: symbol 'atomisp_do_compat_ioctl' was not declared. Should it be static?
	drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:1110:6: warning: symbol 'atomisp_compat_ioctl32' was not declared. Should it be static?

whose include header is missing. Add it.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
index d7c0ef1f9584..15546b1f843d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
@@ -21,6 +21,7 @@
 
 #include "atomisp_internal.h"
 #include "atomisp_compat.h"
+#include "atomisp_ioctl.h"
 #include "atomisp_compat_ioctl32.h"
 
 static int get_atomisp_histogram32(struct atomisp_histogram *kp,
@@ -863,8 +864,8 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return ret;
 }
 
-long atomisp_do_compat_ioctl(struct file *file,
-			    unsigned int cmd, unsigned long arg)
+static long atomisp_do_compat_ioctl(struct file *file,
+				unsigned int cmd, unsigned long arg)
 {
 	union {
 		struct atomisp_histogram his;
-- 
2.14.3

[PATCH 13/17] media: atomisp: compat32: fix __user annotations

[Mauro Carvalho Chehab]

The __user annotations at the compat32 code is not right:

   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:81:18: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:81:18:    expected void *base
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:81:18:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:232:23: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:232:23:    expected unsigned int [usertype] *xcoords_y
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:232:23:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:233:23: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:233:23:    expected unsigned int [usertype] *ycoords_y
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:233:23:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:234:24: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:234:24:    expected unsigned int [usertype] *xcoords_uv
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:234:24:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:235:24: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:235:24:    expected unsigned int [usertype] *ycoords_uv
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:235:24:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:296:29: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:296:29:    expected unsigned int [usertype] *effective_width
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:296:29:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:360:29: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:360:29:    expected unsigned int [usertype] *effective_width
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:360:29:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:437:19: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:437:19:    expected struct v4l2_framebuffer *frame
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:437:19:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:481:29: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:481:29:    expected unsigned short *calb_grp_values
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:481:29:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:701:39: warning: cast removes address space of expression
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:704:21: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:704:21:    expected void const volatile [noderef] <asn:1>*<noident>
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:704:21:    got unsigned int [usertype] *src
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:737:43: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:737:43:    expected struct atomisp_shading_table *shading_table
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:737:43:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:742:44: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:742:44:    expected void [noderef] <asn:1>*to
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:742:44:    got struct atomisp_shading_table *shading_table
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:755:41: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:755:41:    expected struct atomisp_morph_table *morph_table
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:755:41:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:760:44: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:760:44:    expected void [noderef] <asn:1>*to
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:760:44:    got struct atomisp_morph_table *morph_table
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:772:40: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:772:40:    expected struct atomisp_dvs2_coefficients *dvs2_coefs
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:772:40:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:777:44: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:777:44:    expected void [noderef] <asn:1>*to
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:777:44:    got struct atomisp_dvs2_coefficients *dvs2_coefs
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:788:46: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:788:46:    expected struct atomisp_dvs_6axis_config *dvs_6axis_config
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:788:46:    got void [noderef] <asn:1>*
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:793:44: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:793:44:    expected void [noderef] <asn:1>*to
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:793:44:    got struct atomisp_dvs_6axis_config *dvs_6axis_config
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:853:17: warning: incorrect type in assignment (different address spaces)
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:853:17:    expected struct atomisp_sensor_ae_bracketing_lut_entry *lut
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c:853:17:    got void [noderef] <asn:1>*

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 .../atomisp/pci/atomisp2/atomisp_compat_ioctl32.c  | 49 ++++++++++++----------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
index 15546b1f843d..8992d60ee27e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_ioctl32.c
@@ -78,7 +78,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer *kp,
 		get_user(kp->flags, &up->flags))
 			return -EFAULT;
 
-	kp->base = compat_ptr(tmp);
+	kp->base = (void __force *)compat_ptr(tmp);
 	get_v4l2_pix_format((struct v4l2_pix_format *)&kp->fmt, &up->fmt);
 	return 0;
 }
@@ -229,10 +229,10 @@ static int get_atomisp_dvs_6axis_config32(struct atomisp_dvs_6axis_config *kp,
 		get_user(ycoords_uv, &up->ycoords_uv))
 			return -EFAULT;
 
-	kp->xcoords_y = compat_ptr(xcoords_y);
-	kp->ycoords_y = compat_ptr(ycoords_y);
-	kp->xcoords_uv = compat_ptr(xcoords_uv);
-	kp->ycoords_uv = compat_ptr(ycoords_uv);
+	kp->xcoords_y = (void __force *)compat_ptr(xcoords_y);
+	kp->ycoords_y = (void __force *)compat_ptr(ycoords_y);
+	kp->xcoords_uv = (void __force *)compat_ptr(xcoords_uv);
+	kp->ycoords_uv = (void __force *)compat_ptr(ycoords_uv);
 	return 0;
 }
 
@@ -293,7 +293,7 @@ static int get_atomisp_metadata_stat32(struct atomisp_metadata *kp,
 			return -EFAULT;
 
 	kp->data = compat_ptr(data);
-	kp->effective_width = compat_ptr(effective_width);
+	kp->effective_width = (void __force *)compat_ptr(effective_width);
 	return 0;
 }
 
@@ -357,7 +357,7 @@ static int get_atomisp_metadata_by_type_stat32(
 			return -EFAULT;
 
 	kp->data = compat_ptr(data);
-	kp->effective_width = compat_ptr(effective_width);
+	kp->effective_width = (void __force *)compat_ptr(effective_width);
 	return 0;
 }
 
@@ -434,7 +434,7 @@ static int get_atomisp_overlay32(struct atomisp_overlay *kp,
 		get_user(kp->overlay_start_x, &up->overlay_start_y))
 			return -EFAULT;
 
-	kp->frame = compat_ptr(frame);
+	kp->frame = (void __force *)compat_ptr(frame);
 	return 0;
 }
 
@@ -478,7 +478,7 @@ static int get_atomisp_calibration_group32(
 		get_user(calb_grp_values, &up->calb_grp_values))
 			return -EFAULT;
 
-	kp->calb_grp_values = compat_ptr(calb_grp_values);
+	kp->calb_grp_values = (void __force *)compat_ptr(calb_grp_values);
 	return 0;
 }
 
@@ -698,8 +698,8 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 			return -EFAULT;
 
 	while (n >= 0) {
-		compat_uptr_t *src = (compat_uptr_t *)up + n;
-		uintptr_t *dst = (uintptr_t *)kp + n;
+		compat_uptr_t __user *src = ((compat_uptr_t __user *)up) + n;
+		uintptr_t *dst = ((uintptr_t *)kp) + n;
 
 		if (get_user((*dst), src))
 			return -EFAULT;
@@ -734,12 +734,12 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 						(uintptr_t)stp))
 				return -EFAULT;
 
-			kp->shading_table = user_ptr + offset;
+			kp->shading_table = (void __force *)user_ptr + offset;
 			offset = sizeof(struct atomisp_shading_table);
 			if (!kp->shading_table)
 				return -EFAULT;
 
-			if (copy_to_user(kp->shading_table,
+			if (copy_to_user((void __user *)kp->shading_table,
 					 &karg.shading_table,
 					 sizeof(struct atomisp_shading_table)))
 				return -EFAULT;
@@ -752,13 +752,14 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 						(uintptr_t)mtp))
 				return -EFAULT;
 
-			kp->morph_table = user_ptr + offset;
+			kp->morph_table = (void __force *)user_ptr + offset;
 			offset += sizeof(struct atomisp_morph_table);
 			if (!kp->morph_table)
 				return -EFAULT;
 
-			if (copy_to_user(kp->morph_table, &karg.morph_table,
-					   sizeof(struct atomisp_morph_table)))
+			if (copy_to_user((void __user *)kp->morph_table,
+				         &karg.morph_table,
+					 sizeof(struct atomisp_morph_table)))
 				return -EFAULT;
 		}
 
@@ -769,13 +770,14 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 						(uintptr_t)dcp))
 				return -EFAULT;
 
-			kp->dvs2_coefs = user_ptr + offset;
+			kp->dvs2_coefs = (void __force *)user_ptr + offset;
 			offset += sizeof(struct atomisp_dis_coefficients);
 			if (!kp->dvs2_coefs)
 				return -EFAULT;
 
-			if (copy_to_user(kp->dvs2_coefs, &karg.dvs2_coefs,
-				sizeof(struct atomisp_dis_coefficients)))
+			if (copy_to_user((void __user *)kp->dvs2_coefs,
+					 &karg.dvs2_coefs,
+					 sizeof(struct atomisp_dis_coefficients)))
 				return -EFAULT;
 		}
 		/* handle dvs 6axis configuration */
@@ -785,13 +787,14 @@ static int get_atomisp_parameters32(struct atomisp_parameters *kp,
 						(uintptr_t)dscp))
 				return -EFAULT;
 
-			kp->dvs_6axis_config = user_ptr + offset;
+			kp->dvs_6axis_config = (void __force *)user_ptr + offset;
 			offset += sizeof(struct atomisp_dvs_6axis_config);
 			if (!kp->dvs_6axis_config)
 				return -EFAULT;
 
-			if (copy_to_user(kp->dvs_6axis_config, &karg.dvs_6axis_config,
-				sizeof(struct atomisp_dvs_6axis_config)))
+			if (copy_to_user((void __user *)kp->dvs_6axis_config,
+					 &karg.dvs_6axis_config,
+					 sizeof(struct atomisp_dvs_6axis_config)))
 				return -EFAULT;
 		}
 	}
@@ -850,7 +853,7 @@ static int get_atomisp_sensor_ae_bracketing_lut(
 		get_user(lut, &up->lut))
 			return -EFAULT;
 
-	kp->lut = compat_ptr(lut);
+	kp->lut = (void __force *)compat_ptr(lut);
 	return 0;
 }
 
-- 
2.14.3

[PATCH 14/17] media: atomisp: get rid of a warning

[Mauro Carvalho Chehab]

On smatch, this warning is trigged:

	drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c:324 __bo_take_off_handling() error: we previously assumed 'bo->prev' could be null (see line 314)

Because it can't properly analize the truth table for the above
function. So, add an explicit check for the final condition there.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
index c888f9c809f9..a6620d2c9f50 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
@@ -319,7 +319,7 @@ static void __bo_take_off_handling(struct hmm_buffer_object *bo)
 	 *	to take off this bo, we just set take the "prev/next" pointers
 	 *	to NULL, the free rbtree stays unchaged
 	 */
-	} else {
+	} else if (bo->prev != NULL && bo->next != NULL) {
 		bo->next->prev = bo->prev;
 		bo->prev->next = bo->next;
 		bo->next = NULL;
-- 
2.14.3

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding

If something goes wrong at readl(), the logic will stay there
inside an IRQ code forever. This is not the nicest thing to
do :-)

So, add a timeout there, preventing staying inside the IRQ
for more than 10ms.

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

diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
index d2efd7b2c3bc..c855b177103c 100644
--- a/drivers/media/rc/st_rc.c
+++ b/drivers/media/rc/st_rc.c
@@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
 
 static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
 {
+	unsigned long timeout;
 	unsigned int symbol, mark = 0;
 	struct st_rc_device *dev = data;
 	int last_symbol = 0;
-	u32 status;
+	u32 status, int_status;
 	DEFINE_IR_RAW_EVENT(ev);
 
 	if (dev->irq_wake)
 		pm_wakeup_event(dev->dev, 0);
 
-	status  = readl(dev->rx_base + IRB_RX_STATUS);
+	/* FIXME: is 10ms good enough ? */
+	timeout = jiffies +  msecs_to_jiffies(10);
+	do {
+		status  = readl(dev->rx_base + IRB_RX_STATUS);
+		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
+			break;
 
-	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
-		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
+		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
 		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
 			/* discard the entire collection in case of errors!  */
 			ir_raw_event_reset(dev->rdev);
@@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
 
 		}
 		last_symbol = 0;
-		status  = readl(dev->rx_base + IRB_RX_STATUS);
-	}
+	} while (time_is_after_jiffies(timeout));
 
 	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
 
-- 
2.14.3

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding

If something goes wrong at readl(), the logic will stay there
inside an IRQ code forever. This is not the nicest thing to
do :-)

So, add a timeout there, preventing staying inside the IRQ
for more than 10ms.

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

diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
index d2efd7b2c3bc..c855b177103c 100644
--- a/drivers/media/rc/st_rc.c
+++ b/drivers/media/rc/st_rc.c
@@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
 
 static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
 {
+	unsigned long timeout;
 	unsigned int symbol, mark = 0;
 	struct st_rc_device *dev = data;
 	int last_symbol = 0;
-	u32 status;
+	u32 status, int_status;
 	DEFINE_IR_RAW_EVENT(ev);
 
 	if (dev->irq_wake)
 		pm_wakeup_event(dev->dev, 0);
 
-	status  = readl(dev->rx_base + IRB_RX_STATUS);
+	/* FIXME: is 10ms good enough ? */
+	timeout = jiffies +  msecs_to_jiffies(10);
+	do {
+		status  = readl(dev->rx_base + IRB_RX_STATUS);
+		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
+			break;
 
-	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
-		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
+		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
 		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
 			/* discard the entire collection in case of errors!  */
 			ir_raw_event_reset(dev->rdev);
@@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
 
 		}
 		last_symbol = 0;
-		status  = readl(dev->rx_base + IRB_RX_STATUS);
-	}
+	} while (time_is_after_jiffies(timeout));
 
 	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
 
-- 
2.14.3

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.

If we knew how large the fifo was, then we could limit the loop to that many
iterations (maybe a few extra in case IR arrives while we a reading, but
IR is much slower than a cpu executing this loop of course).

Patrice is something you could help with?

Thanks

Sean

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>  
>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  {
> +	unsigned long timeout;
>  	unsigned int symbol, mark = 0;
>  	struct st_rc_device *dev = data;
>  	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>  	DEFINE_IR_RAW_EVENT(ev);
>  
>  	if (dev->irq_wake)
>  		pm_wakeup_event(dev->dev, 0);
>  
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>  
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>  			/* discard the entire collection in case of errors!  */
>  			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  
>  		}
>  		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>  
>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>  
> -- 
> 2.14.3

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.

If we knew how large the fifo was, then we could limit the loop to that many
iterations (maybe a few extra in case IR arrives while we a reading, but
IR is much slower than a cpu executing this loop of course).

Patrice is something you could help with?

Thanks

Sean

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>  
>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  {
> +	unsigned long timeout;
>  	unsigned int symbol, mark = 0;
>  	struct st_rc_device *dev = data;
>  	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>  	DEFINE_IR_RAW_EVENT(ev);
>  
>  	if (dev->irq_wake)
>  		pm_wakeup_event(dev->dev, 0);
>  
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>  
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>  			/* discard the entire collection in case of errors!  */
>  			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  
>  		}
>  		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>  
>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>  
> -- 
> 2.14.3

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Patrice CHOTARD]

Hi Mauro, Sean

On 04/13/2018 12:21 AM, Sean Young wrote:
> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
>> As warned by smatch:
>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
>>
>> If something goes wrong at readl(), the logic will stay there
>> inside an IRQ code forever. This is not the nicest thing to
>> do :-)
>>
>> So, add a timeout there, preventing staying inside the IRQ
>> for more than 10ms.
> 
> If we knew how large the fifo was, then we could limit the loop to that many
> iterations (maybe a few extra in case IR arrives while we a reading, but
> IR is much slower than a cpu executing this loop of course).
> 
> Patrice is something you could help with?

Unfortunately, i will not be able to give you an answer regarding the Rx 
fifo size.

For information, currently, none of upstreamed ST boards are using this 
driver. It was used in the past internally and by customers.

Regards

> 
> Thanks
> 
> Sean
> 
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> ---
>>   drivers/media/rc/st_rc.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
>> index d2efd7b2c3bc..c855b177103c 100644
>> --- a/drivers/media/rc/st_rc.c
>> +++ b/drivers/media/rc/st_rc.c
>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>>   
>>   static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>   {
>> +	unsigned long timeout;
>>   	unsigned int symbol, mark = 0;
>>   	struct st_rc_device *dev = data;
>>   	int last_symbol = 0;
>> -	u32 status;
>> +	u32 status, int_status;
>>   	DEFINE_IR_RAW_EVENT(ev);
>>   
>>   	if (dev->irq_wake)
>>   		pm_wakeup_event(dev->dev, 0);
>>   
>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
>> +	/* FIXME: is 10ms good enough ? */
>> +	timeout = jiffies +  msecs_to_jiffies(10);
>> +	do {
>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
>> +			break;
>>   
>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>   		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>>   			/* discard the entire collection in case of errors!  */
>>   			ir_raw_event_reset(dev->rdev);
>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>   
>>   		}
>>   		last_symbol = 0;
>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
>> -	}
>> +	} while (time_is_after_jiffies(timeout));
>>   
>>   	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>>   
>> -- 
>> 2.14.3

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Patrice CHOTARD]

Hi Mauro, Sean

On 04/13/2018 12:21 AM, Sean Young wrote:
> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
>> As warned by smatch:
>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
>>
>> If something goes wrong at readl(), the logic will stay there
>> inside an IRQ code forever. This is not the nicest thing to
>> do :-)
>>
>> So, add a timeout there, preventing staying inside the IRQ
>> for more than 10ms.
> 
> If we knew how large the fifo was, then we could limit the loop to that many
> iterations (maybe a few extra in case IR arrives while we a reading, but
> IR is much slower than a cpu executing this loop of course).
> 
> Patrice is something you could help with?

Unfortunately, i will not be able to give you an answer regarding the Rx 
fifo size.

For information, currently, none of upstreamed ST boards are using this 
driver. It was used in the past internally and by customers.

Regards

> 
> Thanks
> 
> Sean
> 
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> ---
>>   drivers/media/rc/st_rc.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
>> index d2efd7b2c3bc..c855b177103c 100644
>> --- a/drivers/media/rc/st_rc.c
>> +++ b/drivers/media/rc/st_rc.c
>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>>   
>>   static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>   {
>> +	unsigned long timeout;
>>   	unsigned int symbol, mark = 0;
>>   	struct st_rc_device *dev = data;
>>   	int last_symbol = 0;
>> -	u32 status;
>> +	u32 status, int_status;
>>   	DEFINE_IR_RAW_EVENT(ev);
>>   
>>   	if (dev->irq_wake)
>>   		pm_wakeup_event(dev->dev, 0);
>>   
>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
>> +	/* FIXME: is 10ms good enough ? */
>> +	timeout = jiffies +  msecs_to_jiffies(10);
>> +	do {
>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
>> +			break;
>>   
>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>   		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>>   			/* discard the entire collection in case of errors!  */
>>   			ir_raw_event_reset(dev->rdev);
>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>   
>>   		}
>>   		last_symbol = 0;
>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
>> -	}
>> +	} while (time_is_after_jiffies(timeout));
>>   
>>   	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>>   
>> -- 
>> 2.14.3

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Hi Sean,

Em Thu, 12 Apr 2018 23:21:32 +0100
Sean Young <sean@mess.org> escreveu:

> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> > As warned by smatch:
> > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > 
> > If something goes wrong at readl(), the logic will stay there
> > inside an IRQ code forever. This is not the nicest thing to
> > do :-)
> > 
> > So, add a timeout there, preventing staying inside the IRQ
> > for more than 10ms.  
> 
> If we knew how large the fifo was, then we could limit the loop to that many
> iterations (maybe a few extra in case IR arrives while we a reading, but
> IR is much slower than a cpu executing this loop of course).

IR is slower, but this code is called at IRQ time, e. g. when the
controller already received the IR data. Also, it reads directly
via a memory mapped register, with should be fast. I suspect that
10ms is a lot more time than what would be required to go though
all the FIFO data.

> 
> Patrice is something you could help with?
> 
> Thanks
> 
> Sean
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> > index d2efd7b2c3bc..c855b177103c 100644
> > --- a/drivers/media/rc/st_rc.c
> > +++ b/drivers/media/rc/st_rc.c
> > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >  
> >  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  {
> > +	unsigned long timeout;
> >  	unsigned int symbol, mark = 0;
> >  	struct st_rc_device *dev = data;
> >  	int last_symbol = 0;
> > -	u32 status;
> > +	u32 status, int_status;
> >  	DEFINE_IR_RAW_EVENT(ev);
> >  
> >  	if (dev->irq_wake)
> >  		pm_wakeup_event(dev->dev, 0);
> >  
> > -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +	/* FIXME: is 10ms good enough ? */
> > +	timeout = jiffies +  msecs_to_jiffies(10);
> > +	do {
> > +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> > +			break;
> >  
> > -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> > -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> > +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >  			/* discard the entire collection in case of errors!  */
> >  			ir_raw_event_reset(dev->rdev);
> > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  
> >  		}
> >  		last_symbol = 0;
> > -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > -	}
> > +	} while (time_is_after_jiffies(timeout));
> >  
> >  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >  
> > -- 
> > 2.14.3  



Thanks,
Mauro

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Hi Sean,

Em Thu, 12 Apr 2018 23:21:32 +0100
Sean Young <sean@mess.org> escreveu:

> On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> > As warned by smatch:
> > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > 
> > If something goes wrong at readl(), the logic will stay there
> > inside an IRQ code forever. This is not the nicest thing to
> > do :-)
> > 
> > So, add a timeout there, preventing staying inside the IRQ
> > for more than 10ms.  
> 
> If we knew how large the fifo was, then we could limit the loop to that many
> iterations (maybe a few extra in case IR arrives while we a reading, but
> IR is much slower than a cpu executing this loop of course).

IR is slower, but this code is called at IRQ time, e. g. when the
controller already received the IR data. Also, it reads directly
via a memory mapped register, with should be fast. I suspect that
10ms is a lot more time than what would be required to go though
all the FIFO data.

> 
> Patrice is something you could help with?
> 
> Thanks
> 
> Sean
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> > index d2efd7b2c3bc..c855b177103c 100644
> > --- a/drivers/media/rc/st_rc.c
> > +++ b/drivers/media/rc/st_rc.c
> > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >  
> >  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  {
> > +	unsigned long timeout;
> >  	unsigned int symbol, mark = 0;
> >  	struct st_rc_device *dev = data;
> >  	int last_symbol = 0;
> > -	u32 status;
> > +	u32 status, int_status;
> >  	DEFINE_IR_RAW_EVENT(ev);
> >  
> >  	if (dev->irq_wake)
> >  		pm_wakeup_event(dev->dev, 0);
> >  
> > -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +	/* FIXME: is 10ms good enough ? */
> > +	timeout = jiffies +  msecs_to_jiffies(10);
> > +	do {
> > +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> > +			break;
> >  
> > -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> > -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> > +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >  			/* discard the entire collection in case of errors!  */
> >  			ir_raw_event_reset(dev->rdev);
> > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  
> >  		}
> >  		last_symbol = 0;
> > -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > -	}
> > +	} while (time_is_after_jiffies(timeout));
> >  
> >  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >  
> > -- 
> > 2.14.3  



Thanks,
Mauro

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote:
> Hi Sean,
> 
> Em Thu, 12 Apr 2018 23:21:32 +0100
> Sean Young <sean@mess.org> escreveu:
> 
> > On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> > > As warned by smatch:
> > > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > > 
> > > If something goes wrong at readl(), the logic will stay there
> > > inside an IRQ code forever. This is not the nicest thing to
> > > do :-)
> > > 
> > > So, add a timeout there, preventing staying inside the IRQ
> > > for more than 10ms.  
> > 
> > If we knew how large the fifo was, then we could limit the loop to that many
> > iterations (maybe a few extra in case IR arrives while we a reading, but
> > IR is much slower than a cpu executing this loop of course).
> 
> IR is slower, but this code is called at IRQ time, e. g. when the
> controller already received the IR data. Also, it reads directly
> via a memory mapped register, with should be fast.

There is a chance that a new IR edge occurs whilst reading the fifo. All of
this is academic since we don't know the size of the fifo.

> I suspect that
> 10ms is a lot more time than what would be required to go though
> all the FIFO data.

10ms seems far too much. The serial_ir prints a warning if it loops more
than 255 times (and breaks out of the loop). Since we don't know the size
of the fifo, that will be more than enough. We could also limit it to 
512 times, since the raw IR kfifo is 512 and any more than that would
not fit in the kfifo anyway. This would exit much quicker than 10ms.

At least winbond-cir has a similar loop, there might be other drivers.

Thanks
Sean

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote:
> Hi Sean,
> 
> Em Thu, 12 Apr 2018 23:21:32 +0100
> Sean Young <sean@mess.org> escreveu:
> 
> > On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:
> > > As warned by smatch:
> > > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > > 
> > > If something goes wrong at readl(), the logic will stay there
> > > inside an IRQ code forever. This is not the nicest thing to
> > > do :-)
> > > 
> > > So, add a timeout there, preventing staying inside the IRQ
> > > for more than 10ms.  
> > 
> > If we knew how large the fifo was, then we could limit the loop to that many
> > iterations (maybe a few extra in case IR arrives while we a reading, but
> > IR is much slower than a cpu executing this loop of course).
> 
> IR is slower, but this code is called at IRQ time, e. g. when the
> controller already received the IR data. Also, it reads directly
> via a memory mapped register, with should be fast.

There is a chance that a new IR edge occurs whilst reading the fifo. All of
this is academic since we don't know the size of the fifo.

> I suspect that
> 10ms is a lot more time than what would be required to go though
> all the FIFO data.

10ms seems far too much. The serial_ir prints a warning if it loops more
than 255 times (and breaks out of the loop). Since we don't know the size
of the fifo, that will be more than enough. We could also limit it to 
512 times, since the raw IR kfifo is 512 and any more than that would
not fit in the kfifo anyway. This would exit much quicker than 10ms.

At least winbond-cir has a similar loop, there might be other drivers.

Thanks
Sean

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 10:40:05 +0100
Sean Young <sean@mess.org> escreveu:

> On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote:
> > Hi Sean,
> > 
> > Em Thu, 12 Apr 2018 23:21:32 +0100
> > Sean Young <sean@mess.org> escreveu:
> >   
> > > On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:  
> > > > As warned by smatch:
> > > > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > > > 
> > > > If something goes wrong at readl(), the logic will stay there
> > > > inside an IRQ code forever. This is not the nicest thing to
> > > > do :-)
> > > > 
> > > > So, add a timeout there, preventing staying inside the IRQ
> > > > for more than 10ms.    
> > > 
> > > If we knew how large the fifo was, then we could limit the loop to that many
> > > iterations (maybe a few extra in case IR arrives while we a reading, but
> > > IR is much slower than a cpu executing this loop of course).  
> > 
> > IR is slower, but this code is called at IRQ time, e. g. when the
> > controller already received the IR data. Also, it reads directly
> > via a memory mapped register, with should be fast.  
> 
> There is a chance that a new IR edge occurs whilst reading the fifo. All of
> this is academic since we don't know the size of the fifo.
> 
> > I suspect that
> > 10ms is a lot more time than what would be required to go though
> > all the FIFO data.  
> 
> 10ms seems far too much. The serial_ir prints a warning if it loops more
> than 255 times (and breaks out of the loop). Since we don't know the size
> of the fifo, that will be more than enough. We could also limit it to 
> 512 times, since the raw IR kfifo is 512 and any more than that would
> not fit in the kfifo anyway. This would exit much quicker than 10ms.
> 
> At least winbond-cir has a similar loop, there might be other drivers.

Yeah, we could limit it to run only 512 times (or some other reasonable
quantity), but in order to do that, we need to be sure that, on each read(),
the FIFO will shift - e. g. no risk of needing to do more than one read
to get the next element. That would work if the FIFO is implemented via
flip-flops. But if it is implemented via some slow memory, or if the
shift logic is implemented via some software on a micro-controller, it
may need a few interactions to get the next value.

Without knowing about the hardware implementation, I'd say that setting
a max time for the whole FIFO interaction is safer.

Thanks,
Mauro

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 10:40:05 +0100
Sean Young <sean@mess.org> escreveu:

> On Fri, Apr 13, 2018 at 06:06:46AM -0300, Mauro Carvalho Chehab wrote:
> > Hi Sean,
> > 
> > Em Thu, 12 Apr 2018 23:21:32 +0100
> > Sean Young <sean@mess.org> escreveu:
> >   
> > > On Thu, Apr 12, 2018 at 11:24:07AM -0400, Mauro Carvalho Chehab wrote:  
> > > > As warned by smatch:
> > > > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > > > 
> > > > If something goes wrong at readl(), the logic will stay there
> > > > inside an IRQ code forever. This is not the nicest thing to
> > > > do :-)
> > > > 
> > > > So, add a timeout there, preventing staying inside the IRQ
> > > > for more than 10ms.    
> > > 
> > > If we knew how large the fifo was, then we could limit the loop to that many
> > > iterations (maybe a few extra in case IR arrives while we a reading, but
> > > IR is much slower than a cpu executing this loop of course).  
> > 
> > IR is slower, but this code is called at IRQ time, e. g. when the
> > controller already received the IR data. Also, it reads directly
> > via a memory mapped register, with should be fast.  
> 
> There is a chance that a new IR edge occurs whilst reading the fifo. All of
> this is academic since we don't know the size of the fifo.
> 
> > I suspect that
> > 10ms is a lot more time than what would be required to go though
> > all the FIFO data.  
> 
> 10ms seems far too much. The serial_ir prints a warning if it loops more
> than 255 times (and breaks out of the loop). Since we don't know the size
> of the fifo, that will be more than enough. We could also limit it to 
> 512 times, since the raw IR kfifo is 512 and any more than that would
> not fit in the kfifo anyway. This would exit much quicker than 10ms.
> 
> At least winbond-cir has a similar loop, there might be other drivers.

Yeah, we could limit it to run only 512 times (or some other reasonable
quantity), but in order to do that, we need to be sure that, on each read(),
the FIFO will shift - e. g. no risk of needing to do more than one read
to get the next element. That would work if the FIFO is implemented via
flip-flops. But if it is implemented via some slow memory, or if the
shift logic is implemented via some software on a micro-controller, it
may need a few interactions to get the next value.

Without knowing about the hardware implementation, I'd say that setting
a max time for the whole FIFO interaction is safer.

Thanks,
Mauro

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote:
> Yeah, we could limit it to run only 512 times (or some other reasonable
> quantity), but in order to do that, we need to be sure that, on each read(),
> the FIFO will shift - e. g. no risk of needing to do more than one read
> to get the next element. That would work if the FIFO is implemented via
> flip-flops. But if it is implemented via some slow memory, or if the
> shift logic is implemented via some software on a micro-controller, it
> may need a few interactions to get the next value.
> 
> Without knowing about the hardware implementation, I'd say that setting
> a max time for the whole FIFO interaction is safer.

Ok. If the 10ms timeout is reached, there really is a problem; should we
report an error in this case?

Thanks

Sean

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Sean Young]

On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote:
> Yeah, we could limit it to run only 512 times (or some other reasonable
> quantity), but in order to do that, we need to be sure that, on each read(),
> the FIFO will shift - e. g. no risk of needing to do more than one read
> to get the next element. That would work if the FIFO is implemented via
> flip-flops. But if it is implemented via some slow memory, or if the
> shift logic is implemented via some software on a micro-controller, it
> may need a few interactions to get the next value.
> 
> Without knowing about the hardware implementation, I'd say that setting
> a max time for the whole FIFO interaction is safer.

Ok. If the 10ms timeout is reached, there really is a problem; should we
report an error in this case?

Thanks

Sean

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 14:20:52 +0100
Sean Young <sean@mess.org> escreveu:

> On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote:
> > Yeah, we could limit it to run only 512 times (or some other reasonable
> > quantity), but in order to do that, we need to be sure that, on each read(),
> > the FIFO will shift - e. g. no risk of needing to do more than one read
> > to get the next element. That would work if the FIFO is implemented via
> > flip-flops. But if it is implemented via some slow memory, or if the
> > shift logic is implemented via some software on a micro-controller, it
> > may need a few interactions to get the next value.
> > 
> > Without knowing about the hardware implementation, I'd say that setting
> > a max time for the whole FIFO interaction is safer.  
> 
> Ok. If the 10ms timeout is reached, there really is a problem; should we
> report an error in this case?

Maybe, but then it should likely warn only once.


Thanks,
Mauro

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 14:20:52 +0100
Sean Young <sean@mess.org> escreveu:

> On Fri, Apr 13, 2018 at 07:00:50AM -0300, Mauro Carvalho Chehab wrote:
> > Yeah, we could limit it to run only 512 times (or some other reasonable
> > quantity), but in order to do that, we need to be sure that, on each read(),
> > the FIFO will shift - e. g. no risk of needing to do more than one read
> > to get the next element. That would work if the FIFO is implemented via
> > flip-flops. But if it is implemented via some slow memory, or if the
> > shift logic is implemented via some software on a micro-controller, it
> > may need a few interactions to get the next value.
> > 
> > Without knowing about the hardware implementation, I'd say that setting
> > a max time for the whole FIFO interaction is safer.  
> 
> Ok. If the 10ms timeout is reached, there really is a problem; should we
> report an error in this case?

Maybe, but then it should likely warn only once.


Thanks,
Mauro

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Patrice CHOTARD]

Hi Mauro

On 04/12/2018 05:24 PM, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>   drivers/media/rc/st_rc.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>   
>   static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>   {
> +	unsigned long timeout;
>   	unsigned int symbol, mark = 0;
>   	struct st_rc_device *dev = data;
>   	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>   	DEFINE_IR_RAW_EVENT(ev);
>   
>   	if (dev->irq_wake)
>   		pm_wakeup_event(dev->dev, 0);
>   
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>   
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>   		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>   			/* discard the entire collection in case of errors!  */
>   			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>   
>   		}
>   		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>   
>   	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>   
> 

Acked-by: Patrice Chotard <patrice.chotard@st.com>

Thanks

Patrice

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Patrice CHOTARD]

Hi Mauro

On 04/12/2018 05:24 PM, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>   drivers/media/rc/st_rc.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>   
>   static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>   {
> +	unsigned long timeout;
>   	unsigned int symbol, mark = 0;
>   	struct st_rc_device *dev = data;
>   	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>   	DEFINE_IR_RAW_EVENT(ev);
>   
>   	if (dev->irq_wake)
>   		pm_wakeup_event(dev->dev, 0);
>   
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>   
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>   		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>   			/* discard the entire collection in case of errors!  */
>   			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>   
>   		}
>   		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>   
>   	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>   
> 

Acked-by: Patrice Chotard <patrice.chotard@st.com>

Thanks

Patrice

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mason]

On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:

> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>  
>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  {
> +	unsigned long timeout;
>  	unsigned int symbol, mark = 0;
>  	struct st_rc_device *dev = data;
>  	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>  	DEFINE_IR_RAW_EVENT(ev);
>  
>  	if (dev->irq_wake)
>  		pm_wakeup_event(dev->dev, 0);
>  
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>  
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>  			/* discard the entire collection in case of errors!  */
>  			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  
>  		}
>  		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>  
>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>  

Isn't this a place where the iopoll.h helpers might be useful?

e.g. readl_poll_timeout()

https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114

Regards.

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mason]

On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:

> As warned by smatch:
> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> 
> If something goes wrong at readl(), the logic will stay there
> inside an IRQ code forever. This is not the nicest thing to
> do :-)
> 
> So, add a timeout there, preventing staying inside the IRQ
> for more than 10ms.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> index d2efd7b2c3bc..c855b177103c 100644
> --- a/drivers/media/rc/st_rc.c
> +++ b/drivers/media/rc/st_rc.c
> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>  
>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  {
> +	unsigned long timeout;
>  	unsigned int symbol, mark = 0;
>  	struct st_rc_device *dev = data;
>  	int last_symbol = 0;
> -	u32 status;
> +	u32 status, int_status;
>  	DEFINE_IR_RAW_EVENT(ev);
>  
>  	if (dev->irq_wake)
>  		pm_wakeup_event(dev->dev, 0);
>  
> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> +	/* FIXME: is 10ms good enough ? */
> +	timeout = jiffies +  msecs_to_jiffies(10);
> +	do {
> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> +			break;
>  
> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>  			/* discard the entire collection in case of errors!  */
>  			ir_raw_event_reset(dev->rdev);
> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>  
>  		}
>  		last_symbol = 0;
> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> -	}
> +	} while (time_is_after_jiffies(timeout));
>  
>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>  

Isn't this a place where the iopoll.h helpers might be useful?

e.g. readl_poll_timeout()

https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114

Regards.

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 11:15:16 +0200
Mason <slash.tmp@free.fr> escreveu:

> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
> 
> > As warned by smatch:
> > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > 
> > If something goes wrong at readl(), the logic will stay there
> > inside an IRQ code forever. This is not the nicest thing to
> > do :-)
> > 
> > So, add a timeout there, preventing staying inside the IRQ
> > for more than 10ms.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> > index d2efd7b2c3bc..c855b177103c 100644
> > --- a/drivers/media/rc/st_rc.c
> > +++ b/drivers/media/rc/st_rc.c
> > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >  
> >  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  {
> > +	unsigned long timeout;
> >  	unsigned int symbol, mark = 0;
> >  	struct st_rc_device *dev = data;
> >  	int last_symbol = 0;
> > -	u32 status;
> > +	u32 status, int_status;
> >  	DEFINE_IR_RAW_EVENT(ev);
> >  
> >  	if (dev->irq_wake)
> >  		pm_wakeup_event(dev->dev, 0);
> >  
> > -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +	/* FIXME: is 10ms good enough ? */
> > +	timeout = jiffies +  msecs_to_jiffies(10);
> > +	do {
> > +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> > +			break;
> >  
> > -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> > -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> > +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >  			/* discard the entire collection in case of errors!  */
> >  			ir_raw_event_reset(dev->rdev);
> > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  
> >  		}
> >  		last_symbol = 0;
> > -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > -	}
> > +	} while (time_is_after_jiffies(timeout));
> >  
> >  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >    
> 
> Isn't this a place where the iopoll.h helpers might be useful?
> 
> e.g. readl_poll_timeout()
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114

That won't work. Internally[1], readx_poll_timeout() calls
usleep_range().

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43

It can't be called here, as this loop happens at the irq
handler.

Thanks,
Mauro

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 11:15:16 +0200
Mason <slash.tmp@free.fr> escreveu:

> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
> 
> > As warned by smatch:
> > 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> > 
> > If something goes wrong at readl(), the logic will stay there
> > inside an IRQ code forever. This is not the nicest thing to
> > do :-)
> > 
> > So, add a timeout there, preventing staying inside the IRQ
> > for more than 10ms.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> > index d2efd7b2c3bc..c855b177103c 100644
> > --- a/drivers/media/rc/st_rc.c
> > +++ b/drivers/media/rc/st_rc.c
> > @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >  
> >  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  {
> > +	unsigned long timeout;
> >  	unsigned int symbol, mark = 0;
> >  	struct st_rc_device *dev = data;
> >  	int last_symbol = 0;
> > -	u32 status;
> > +	u32 status, int_status;
> >  	DEFINE_IR_RAW_EVENT(ev);
> >  
> >  	if (dev->irq_wake)
> >  		pm_wakeup_event(dev->dev, 0);
> >  
> > -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +	/* FIXME: is 10ms good enough ? */
> > +	timeout = jiffies +  msecs_to_jiffies(10);
> > +	do {
> > +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> > +			break;
> >  
> > -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> > -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> > +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >  			/* discard the entire collection in case of errors!  */
> >  			ir_raw_event_reset(dev->rdev);
> > @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >  
> >  		}
> >  		last_symbol = 0;
> > -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> > -	}
> > +	} while (time_is_after_jiffies(timeout));
> >  
> >  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >    
> 
> Isn't this a place where the iopoll.h helpers might be useful?
> 
> e.g. readl_poll_timeout()
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114

That won't work. Internally[1], readx_poll_timeout() calls
usleep_range().

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43

It can't be called here, as this loop happens at the irq
handler.

Thanks,
Mauro

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mason]

On 13/04/2018 11:25, Mauro Carvalho Chehab wrote:
> Em Fri, 13 Apr 2018 11:15:16 +0200
> Mason <slash.tmp@free.fr> escreveu:
> 
>> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
>>
>>> As warned by smatch:
>>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
>>>
>>> If something goes wrong at readl(), the logic will stay there
>>> inside an IRQ code forever. This is not the nicest thing to
>>> do :-)
>>>
>>> So, add a timeout there, preventing staying inside the IRQ
>>> for more than 10ms.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>> ---
>>>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
>>> index d2efd7b2c3bc..c855b177103c 100644
>>> --- a/drivers/media/rc/st_rc.c
>>> +++ b/drivers/media/rc/st_rc.c
>>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>>>  
>>>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>>  {
>>> +	unsigned long timeout;
>>>  	unsigned int symbol, mark = 0;
>>>  	struct st_rc_device *dev = data;
>>>  	int last_symbol = 0;
>>> -	u32 status;
>>> +	u32 status, int_status;
>>>  	DEFINE_IR_RAW_EVENT(ev);
>>>  
>>>  	if (dev->irq_wake)
>>>  		pm_wakeup_event(dev->dev, 0);
>>>  
>>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> +	/* FIXME: is 10ms good enough ? */
>>> +	timeout = jiffies +  msecs_to_jiffies(10);
>>> +	do {
>>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
>>> +			break;
>>>  
>>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
>>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>>>  			/* discard the entire collection in case of errors!  */
>>>  			ir_raw_event_reset(dev->rdev);
>>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>>  
>>>  		}
>>>  		last_symbol = 0;
>>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> -	}
>>> +	} while (time_is_after_jiffies(timeout));
>>>  
>>>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>>>    
>>
>> Isn't this a place where the iopoll.h helpers might be useful?
>>
>> e.g. readl_poll_timeout()
>>
>> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114
> 
> That won't work. Internally[1], readx_poll_timeout() calls
> usleep_range().
> 
> [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43
> 
> It can't be called here, as this loop happens at the irq
> handler.

Sorry, I meant readl_poll_timeout_atomic()

But it might have to be open-coded because of the check for overruns.

Regards.

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mason]

On 13/04/2018 11:25, Mauro Carvalho Chehab wrote:
> Em Fri, 13 Apr 2018 11:15:16 +0200
> Mason <slash.tmp@free.fr> escreveu:
> 
>> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
>>
>>> As warned by smatch:
>>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
>>>
>>> If something goes wrong at readl(), the logic will stay there
>>> inside an IRQ code forever. This is not the nicest thing to
>>> do :-)
>>>
>>> So, add a timeout there, preventing staying inside the IRQ
>>> for more than 10ms.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>> ---
>>>  drivers/media/rc/st_rc.c | 16 ++++++++++------
>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
>>> index d2efd7b2c3bc..c855b177103c 100644
>>> --- a/drivers/media/rc/st_rc.c
>>> +++ b/drivers/media/rc/st_rc.c
>>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
>>>  
>>>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>>  {
>>> +	unsigned long timeout;
>>>  	unsigned int symbol, mark = 0;
>>>  	struct st_rc_device *dev = data;
>>>  	int last_symbol = 0;
>>> -	u32 status;
>>> +	u32 status, int_status;
>>>  	DEFINE_IR_RAW_EVENT(ev);
>>>  
>>>  	if (dev->irq_wake)
>>>  		pm_wakeup_event(dev->dev, 0);
>>>  
>>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> +	/* FIXME: is 10ms good enough ? */
>>> +	timeout = jiffies +  msecs_to_jiffies(10);
>>> +	do {
>>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
>>> +			break;
>>>  
>>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
>>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
>>>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
>>>  			/* discard the entire collection in case of errors!  */
>>>  			ir_raw_event_reset(dev->rdev);
>>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
>>>  
>>>  		}
>>>  		last_symbol = 0;
>>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
>>> -	}
>>> +	} while (time_is_after_jiffies(timeout));
>>>  
>>>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
>>>    
>>
>> Isn't this a place where the iopoll.h helpers might be useful?
>>
>> e.g. readl_poll_timeout()
>>
>> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114
> 
> That won't work. Internally[1], readx_poll_timeout() calls
> usleep_range().
> 
> [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43
> 
> It can't be called here, as this loop happens at the irq
> handler.

Sorry, I meant readl_poll_timeout_atomic()

But it might have to be open-coded because of the check for overruns.

Regards.

Re: [PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 11:36:56 +0200
Mason <slash.tmp@free.fr> escreveu:

> On 13/04/2018 11:25, Mauro Carvalho Chehab wrote:
> > Em Fri, 13 Apr 2018 11:15:16 +0200
> > Mason <slash.tmp@free.fr> escreveu:
> >   
> >> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
> >>  
> >>> As warned by smatch:
> >>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> >>>
> >>> If something goes wrong at readl(), the logic will stay there
> >>> inside an IRQ code forever. This is not the nicest thing to
> >>> do :-)
> >>>
> >>> So, add a timeout there, preventing staying inside the IRQ
> >>> for more than 10ms.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >>> ---
> >>>  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> >>> index d2efd7b2c3bc..c855b177103c 100644
> >>> --- a/drivers/media/rc/st_rc.c
> >>> +++ b/drivers/media/rc/st_rc.c
> >>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >>>  
> >>>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >>>  {
> >>> +	unsigned long timeout;
> >>>  	unsigned int symbol, mark = 0;
> >>>  	struct st_rc_device *dev = data;
> >>>  	int last_symbol = 0;
> >>> -	u32 status;
> >>> +	u32 status, int_status;
> >>>  	DEFINE_IR_RAW_EVENT(ev);
> >>>  
> >>>  	if (dev->irq_wake)
> >>>  		pm_wakeup_event(dev->dev, 0);
> >>>  
> >>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> +	/* FIXME: is 10ms good enough ? */
> >>> +	timeout = jiffies +  msecs_to_jiffies(10);
> >>> +	do {
> >>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> >>> +			break;
> >>>  
> >>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> >>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >>>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >>>  			/* discard the entire collection in case of errors!  */
> >>>  			ir_raw_event_reset(dev->rdev);
> >>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >>>  
> >>>  		}
> >>>  		last_symbol = 0;
> >>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> -	}
> >>> +	} while (time_is_after_jiffies(timeout));
> >>>  
> >>>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >>>      
> >>
> >> Isn't this a place where the iopoll.h helpers might be useful?
> >>
> >> e.g. readl_poll_timeout()
> >>
> >> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114  
> > 
> > That won't work. Internally[1], readx_poll_timeout() calls
> > usleep_range().
> > 
> > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43
> > 
> > It can't be called here, as this loop happens at the irq
> > handler.  
> 
> Sorry, I meant readl_poll_timeout_atomic()

Ah, ok!

> But it might have to be open-coded because of the check for overruns.

Yeah, readl_poll_timeout_atomic() works fine if we wanted to read just
one value, but in this case, we need a loop to read from a FIFO, and
we want to ensure that the total time spent there won't be bigger than
a reasonable limit. So, we would need to open-code it, with is what the
patch I proposed actually did (except that it uses jiffies instead of
the high-res clock).

If we take Sean's suggestion of limiting the loop by the FIFO size,
then readl_poll_timeout_atomic() could indeed be an interesting
alternative.

Thanks,
Mauro

[PATCH 15/17] media: st_rc: Don't stay on an IRQ handler forever

[Mauro Carvalho Chehab]

Em Fri, 13 Apr 2018 11:36:56 +0200
Mason <slash.tmp@free.fr> escreveu:

> On 13/04/2018 11:25, Mauro Carvalho Chehab wrote:
> > Em Fri, 13 Apr 2018 11:15:16 +0200
> > Mason <slash.tmp@free.fr> escreveu:
> >   
> >> On 12/04/2018 17:24, Mauro Carvalho Chehab wrote:
> >>  
> >>> As warned by smatch:
> >>> 	drivers/media/rc/st_rc.c:110 st_rc_rx_interrupt() warn: this loop depends on readl() succeeding
> >>>
> >>> If something goes wrong at readl(), the logic will stay there
> >>> inside an IRQ code forever. This is not the nicest thing to
> >>> do :-)
> >>>
> >>> So, add a timeout there, preventing staying inside the IRQ
> >>> for more than 10ms.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >>> ---
> >>>  drivers/media/rc/st_rc.c | 16 ++++++++++------
> >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c
> >>> index d2efd7b2c3bc..c855b177103c 100644
> >>> --- a/drivers/media/rc/st_rc.c
> >>> +++ b/drivers/media/rc/st_rc.c
> >>> @@ -96,19 +96,24 @@ static void st_rc_send_lirc_timeout(struct rc_dev *rdev)
> >>>  
> >>>  static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >>>  {
> >>> +	unsigned long timeout;
> >>>  	unsigned int symbol, mark = 0;
> >>>  	struct st_rc_device *dev = data;
> >>>  	int last_symbol = 0;
> >>> -	u32 status;
> >>> +	u32 status, int_status;
> >>>  	DEFINE_IR_RAW_EVENT(ev);
> >>>  
> >>>  	if (dev->irq_wake)
> >>>  		pm_wakeup_event(dev->dev, 0);
> >>>  
> >>> -	status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> +	/* FIXME: is 10ms good enough ? */
> >>> +	timeout = jiffies +  msecs_to_jiffies(10);
> >>> +	do {
> >>> +		status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> +		if (!(status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)))
> >>> +			break;
> >>>  
> >>> -	while (status & (IRB_FIFO_NOT_EMPTY | IRB_OVERFLOW)) {
> >>> -		u32 int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >>> +		int_status = readl(dev->rx_base + IRB_RX_INT_STATUS);
> >>>  		if (unlikely(int_status & IRB_RX_OVERRUN_INT)) {
> >>>  			/* discard the entire collection in case of errors!  */
> >>>  			ir_raw_event_reset(dev->rdev);
> >>> @@ -148,8 +153,7 @@ static irqreturn_t st_rc_rx_interrupt(int irq, void *data)
> >>>  
> >>>  		}
> >>>  		last_symbol = 0;
> >>> -		status  = readl(dev->rx_base + IRB_RX_STATUS);
> >>> -	}
> >>> +	} while (time_is_after_jiffies(timeout));
> >>>  
> >>>  	writel(IRB_RX_INTS, dev->rx_base + IRB_RX_INT_CLEAR);
> >>>      
> >>
> >> Isn't this a place where the iopoll.h helpers might be useful?
> >>
> >> e.g. readl_poll_timeout()
> >>
> >> https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L114  
> > 
> > That won't work. Internally[1], readx_poll_timeout() calls
> > usleep_range().
> > 
> > [1] https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L43
> > 
> > It can't be called here, as this loop happens at the irq
> > handler.  
> 
> Sorry, I meant readl_poll_timeout_atomic()

Ah, ok!

> But it might have to be open-coded because of the check for overruns.

Yeah, readl_poll_timeout_atomic() works fine if we wanted to read just
one value, but in this case, we need a loop to read from a FIFO, and
we want to ensure that the total time spent there won't be bigger than
a reasonable limit. So, we would need to open-code it, with is what the
patch I proposed actually did (except that it uses jiffies instead of
the high-res clock).

If we take Sean's suggestion of limiting the loop by the FIFO size,
then readl_poll_timeout_atomic() could indeed be an interesting
alternative.

Thanks,
Mauro

[PATCH 16/17] media: mantis: prevent staying forever in a loop at IRQ

[Mauro Carvalho Chehab]

As warned by smatch:
	drivers/media/pci/mantis/mantis_uart.c:105 mantis_uart_work() warn: this loop depends on readl() succeeding

If something goes wrong at readl(), the logic will stay there
inside an IRQ code forever. This is not the nicest thing to
do :-)

So, add a timeout there, preventing staying inside the IRQ
for more than 10ms.

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

diff --git a/drivers/media/pci/mantis/mantis_uart.c b/drivers/media/pci/mantis/mantis_uart.c
index 18f81c135996..b7765687e0c3 100644
--- a/drivers/media/pci/mantis/mantis_uart.c
+++ b/drivers/media/pci/mantis/mantis_uart.c
@@ -92,6 +92,7 @@ static void mantis_uart_work(struct work_struct *work)
 {
 	struct mantis_pci *mantis = container_of(work, struct mantis_pci, uart_work);
 	u32 stat;
+	unsigned long timeout;
 
 	stat = mmread(MANTIS_UART_STAT);
 
@@ -102,9 +103,15 @@ static void mantis_uart_work(struct work_struct *work)
 	 * MANTIS_UART_RXFIFO_DATA is only set if at least
 	 * config->bytes + 1 bytes are in the FIFO.
 	 */
+
+	/* FIXME: is 10ms good enough ? */
+	timeout = jiffies +  msecs_to_jiffies(10);
 	while (stat & MANTIS_UART_RXFIFO_DATA) {
 		mantis_uart_read(mantis);
 		stat = mmread(MANTIS_UART_STAT);
+
+		if (!time_is_after_jiffies(timeout))
+			break;
 	}
 
 	/* re-enable UART (RX) interrupt */
-- 
2.14.3

[PATCH 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab]

Smatch report several issues with bad __user annotations:

  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid

Fix them.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 35 +++++++++++++++------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d03a44d89649..1057ab8ce2b6 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
 			return -EFAULT;
 		break;
 	case V4L2_MEMORY_USERPTR:
-		if (get_user(p, &up->m.userptr) ||
-		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
+		if (get_user(p, &up->m.userptr)||
+		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
 			     &up32->m.userptr))
 			return -EFAULT;
 		break;
@@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	u32 length;
 	enum v4l2_memory memory;
 	struct v4l2_plane32 __user *uplane32;
-	struct v4l2_plane __user *uplane;
+	struct v4l2_plane *uplane;
 	compat_caddr_t p;
 	int ret;
 
@@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 
 		if (num_planes == 0)
 			return 0;
-
-		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
+		if (get_user(uplane, &kp->m.planes))
 			return -EFAULT;
 		if (get_user(p, &up->m.planes))
 			return -EFAULT;
 		uplane32 = compat_ptr(p);
 
 		while (num_planes--) {
-			ret = put_v4l2_plane32(uplane, uplane32, memory);
+			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
 			if (ret)
 				return ret;
 			++uplane;
@@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
 	    get_user(tmp, &up->base) ||
-	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
 	    assign_in_user(&kp->capability, &up->capability) ||
 	    assign_in_user(&kp->flags, &up->flags) ||
 	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
@@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
 	    get_user(base, &kp->base) ||
-	    put_user(ptr_to_compat(base), &up->base) ||
+	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
 	    assign_in_user(&up->capability, &kp->capability) ||
 	    assign_in_user(&up->flags, &kp->flags) ||
 	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
@@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 				   struct v4l2_ext_controls32 __user *up)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
-	struct v4l2_ext_control __user *kcontrols;
+	struct v4l2_ext_control *kcontrols;
 	u32 count;
 	u32 n;
 	compat_caddr_t p;
@@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, &kcontrols->id) ||
+		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
-		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
-		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
+		    assign_in_user(&ucontrols->size,
+				   (unsigned int __user *)&kcontrols->size) ||
+		    copy_in_user(&ucontrols->reserved2,
+				 (unsigned int __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
 			return -EFAULT;
 
@@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
 		if (ctrl_is_pointer(file, id))
 			size -= sizeof(ucontrols->value64);
 
-		if (copy_in_user(ucontrols, kcontrols, size))
+		if (copy_in_user(ucontrols,
+			         (unsigned int __user *)kcontrols, size))
 			return -EFAULT;
 
 		ucontrols++;
@@ -952,9 +954,10 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
 	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
 	    assign_in_user(&kp->pad, &up->pad) ||
 	    assign_in_user(&kp->start_block, &up->start_block) ||
-	    assign_in_user(&kp->blocks, &up->blocks) ||
+	    assign_in_user(&kp->blocks,
+			   (unsigned char __user *)&up->blocks) ||
 	    get_user(tmp, &up->edid) ||
-	    put_user(compat_ptr(tmp), &kp->edid) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
 	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
 		return -EFAULT;
 	return 0;
@@ -970,7 +973,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
 	    assign_in_user(&up->start_block, &kp->start_block) ||
 	    assign_in_user(&up->blocks, &kp->blocks) ||
 	    get_user(edid, &kp->edid) ||
-	    put_user(ptr_to_compat(edid), &up->edid) ||
+	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
 	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
 		return -EFAULT;
 	return 0;
-- 
2.14.3

[PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab]

Smatch report several issues with bad __user annotations:

  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid

Fix them.

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

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d03a44d89649..0b9dfe7dbfe7 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
 			return -EFAULT;
 		break;
 	case V4L2_MEMORY_USERPTR:
-		if (get_user(p, &up->m.userptr) ||
-		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
+		if (get_user(p, &up->m.userptr)||
+		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
 			     &up32->m.userptr))
 			return -EFAULT;
 		break;
@@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	u32 length;
 	enum v4l2_memory memory;
 	struct v4l2_plane32 __user *uplane32;
-	struct v4l2_plane __user *uplane;
+	struct v4l2_plane *uplane;
 	compat_caddr_t p;
 	int ret;
 
@@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 
 		if (num_planes == 0)
 			return 0;
-
-		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
+		if (get_user(uplane, &kp->m.planes))
 			return -EFAULT;
 		if (get_user(p, &up->m.planes))
 			return -EFAULT;
 		uplane32 = compat_ptr(p);
 
 		while (num_planes--) {
-			ret = put_v4l2_plane32(uplane, uplane32, memory);
+			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
 			if (ret)
 				return ret;
 			++uplane;
@@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
 	    get_user(tmp, &up->base) ||
-	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
 	    assign_in_user(&kp->capability, &up->capability) ||
 	    assign_in_user(&kp->flags, &up->flags) ||
 	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
@@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
 	    get_user(base, &kp->base) ||
-	    put_user(ptr_to_compat(base), &up->base) ||
+	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
 	    assign_in_user(&up->capability, &kp->capability) ||
 	    assign_in_user(&up->flags, &kp->flags) ||
 	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
@@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file,
 				   struct v4l2_ext_controls32 __user *up)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
-	struct v4l2_ext_control __user *kcontrols;
+	struct v4l2_ext_control *kcontrols;
 	u32 count;
 	u32 n;
 	compat_caddr_t p;
@@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, &kcontrols->id) ||
+		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
-		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
-		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
+		    assign_in_user(&ucontrols->size,
+				   (unsigned int __user *)&kcontrols->size) ||
+		    copy_in_user(&ucontrols->reserved2,
+				 (unsigned int __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
 			return -EFAULT;
 
@@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
 		if (ctrl_is_pointer(file, id))
 			size -= sizeof(ucontrols->value64);
 
-		if (copy_in_user(ucontrols, kcontrols, size))
+		if (copy_in_user(ucontrols,
+			         (unsigned int __user *)kcontrols, size))
 			return -EFAULT;
 
 		ucontrols++;
@@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
 	    assign_in_user(&kp->start_block, &up->start_block) ||
 	    assign_in_user(&kp->blocks, &up->blocks) ||
 	    get_user(tmp, &up->edid) ||
-	    put_user(compat_ptr(tmp), &kp->edid) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
 	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
 		return -EFAULT;
 	return 0;
@@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
 	    assign_in_user(&up->start_block, &kp->start_block) ||
 	    assign_in_user(&up->blocks, &kp->blocks) ||
 	    get_user(edid, &kp->edid) ||
-	    put_user(ptr_to_compat(edid), &up->edid) ||
+	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
 	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
 		return -EFAULT;
 	return 0;
-- 
2.14.3

Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Hans Verkuil]

On 04/13/2018 08:07 PM, Mauro Carvalho Chehab wrote:
> Smatch report several issues with bad __user annotations:
> 
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
> 
> Fix them.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++++-------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..0b9dfe7dbfe7 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
>  			return -EFAULT;
>  		break;
>  	case V4L2_MEMORY_USERPTR:
> -		if (get_user(p, &up->m.userptr) ||
> -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> +		if (get_user(p, &up->m.userptr)||
> +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>  			     &up32->m.userptr))
>  			return -EFAULT;
>  		break;
> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  	u32 length;
>  	enum v4l2_memory memory;
>  	struct v4l2_plane32 __user *uplane32;
> -	struct v4l2_plane __user *uplane;
> +	struct v4l2_plane *uplane;

This needs a comment (either here or before the get_user below). It really is a
pointer to userspace, but since videodev2.h has it without __user (since it is
copied to kernel space in v4l2-ioctl.c) we need to define it as a regular pointer
here and cast it to a __user pointer in the put_v4l2_plane32() call.

This is not trivially obvious, so a comment would help a lot.

>  	compat_caddr_t p;
>  	int ret;
>  
> @@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  
>  		if (num_planes == 0)
>  			return 0;
> -
> -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
> +		if (get_user(uplane, &kp->m.planes))
>  			return -EFAULT;
>  		if (get_user(p, &up->m.planes))
>  			return -EFAULT;
>  		uplane32 = compat_ptr(p);
>  
>  		while (num_planes--) {
> -			ret = put_v4l2_plane32(uplane, uplane32, memory);
> +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
>  			if (ret)
>  				return ret;
>  			++uplane;
> @@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>  	    get_user(tmp, &up->base) ||
> -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
>  	    assign_in_user(&kp->capability, &up->capability) ||
>  	    assign_in_user(&kp->flags, &up->flags) ||
>  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> @@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    get_user(base, &kp->base) ||
> -	    put_user(ptr_to_compat(base), &up->base) ||
> +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
>  	    assign_in_user(&up->capability, &kp->capability) ||
>  	    assign_in_user(&up->flags, &kp->flags) ||
>  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> @@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file,
>  				   struct v4l2_ext_controls32 __user *up)
>  {
>  	struct v4l2_ext_control32 __user *ucontrols;
> -	struct v4l2_ext_control __user *kcontrols;
> +	struct v4l2_ext_control *kcontrols;
>  	u32 count;
>  	u32 n;
>  	compat_caddr_t p;
> @@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		unsigned int size = sizeof(*ucontrols);
>  		u32 id;
>  
> -		if (get_user(id, &kcontrols->id) ||
> +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
>  		    put_user(id, &ucontrols->id) ||
> -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
> -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> +		    assign_in_user(&ucontrols->size,
> +				   (unsigned int __user *)&kcontrols->size) ||
> +		    copy_in_user(&ucontrols->reserved2,
> +				 (unsigned int __user *)&kcontrols->reserved2,
>  				 sizeof(ucontrols->reserved2)))
>  			return -EFAULT;
>  
> @@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		if (ctrl_is_pointer(file, id))
>  			size -= sizeof(ucontrols->value64);
>  
> -		if (copy_in_user(ucontrols, kcontrols, size))
> +		if (copy_in_user(ucontrols,
> +			         (unsigned int __user *)kcontrols, size))

This is rather ugly. Would it be better to do something like this:

	struct v4l2_ext_control __user *kcontrols
	struct v4l2_ext_control *kcontrols_tmp;

	get_user(kcontrols_tmp, &kp->controls);
	kcontrols = (void __user __force *)kcontrols_tmp;

And then there is no need to change anything else.

Regardless of the chosen solution, this needs comments to explain what is going
on here, just as with v4l2_buffer above.

Note: the whole 'u<foo>' and 'k<foo>' naming is now hopelessly out of date and
confusing. It should really be '<foo>32' and '<foo>64' to denote 32 bit vs
64 bit layout. The pointers are now always in userspace, so 'k<foo>' no longer
makes sense.

>  			return -EFAULT;
>  
>  		ucontrols++;
> @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
>  	    assign_in_user(&kp->start_block, &up->start_block) ||
>  	    assign_in_user(&kp->blocks, &up->blocks) ||
>  	    get_user(tmp, &up->edid) ||
> -	    put_user(compat_ptr(tmp), &kp->edid) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>  		return -EFAULT;
>  	return 0;
> @@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
>  	    assign_in_user(&up->start_block, &kp->start_block) ||
>  	    assign_in_user(&up->blocks, &kp->blocks) ||
>  	    get_user(edid, &kp->edid) ||
> -	    put_user(ptr_to_compat(edid), &up->edid) ||
> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>  		return -EFAULT;
>  	return 0;
> 

Otherwise this patch looks good.

Regards,

	Hans

Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 14:03:45 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/13/2018 08:07 PM, Mauro Carvalho Chehab wrote:
> > Smatch report several issues with bad __user annotations:
> > 
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
> >   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
> > 
> > Fix them.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index d03a44d89649..0b9dfe7dbfe7 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
> >  			return -EFAULT;
> >  		break;
> >  	case V4L2_MEMORY_USERPTR:
> > -		if (get_user(p, &up->m.userptr) ||
> > -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> > +		if (get_user(p, &up->m.userptr)||
> > +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
> >  			     &up32->m.userptr))
> >  			return -EFAULT;
> >  		break;
> > @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >  	u32 length;
> >  	enum v4l2_memory memory;
> >  	struct v4l2_plane32 __user *uplane32;
> > -	struct v4l2_plane __user *uplane;
> > +	struct v4l2_plane *uplane;  
> 
> This needs a comment (either here or before the get_user below). It really is a
> pointer to userspace, but since videodev2.h has it without __user (since it is
> copied to kernel space in v4l2-ioctl.c) we need to define it as a regular pointer
> here and cast it to a __user pointer in the put_v4l2_plane32() call.
> 
> This is not trivially obvious, so a comment would help a lot.



> 
> >  	compat_caddr_t p;
> >  	int ret;
> >  
> > @@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >  
> >  		if (num_planes == 0)
> >  			return 0;
> > -
> > -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
> > +		if (get_user(uplane, &kp->m.planes))
> >  			return -EFAULT;
> >  		if (get_user(p, &up->m.planes))
> >  			return -EFAULT;
> >  		uplane32 = compat_ptr(p);
> >  
> >  		while (num_planes--) {
> > -			ret = put_v4l2_plane32(uplane, uplane32, memory);
> > +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
> >  			if (ret)
> >  				return ret;
> >  			++uplane;
> > @@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >  
> >  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
> >  	    get_user(tmp, &up->base) ||
> > -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> > +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
> >  	    assign_in_user(&kp->capability, &up->capability) ||
> >  	    assign_in_user(&kp->flags, &up->flags) ||
> >  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> > @@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
> >  
> >  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
> >  	    get_user(base, &kp->base) ||
> > -	    put_user(ptr_to_compat(base), &up->base) ||
> > +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
> >  	    assign_in_user(&up->capability, &kp->capability) ||
> >  	    assign_in_user(&up->flags, &kp->flags) ||
> >  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> > @@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file,
> >  				   struct v4l2_ext_controls32 __user *up)
> >  {
> >  	struct v4l2_ext_control32 __user *ucontrols;
> > -	struct v4l2_ext_control __user *kcontrols;
> > +	struct v4l2_ext_control *kcontrols;
> >  	u32 count;
> >  	u32 n;
> >  	compat_caddr_t p;
> > @@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file,
> >  		unsigned int size = sizeof(*ucontrols);
> >  		u32 id;
> >  
> > -		if (get_user(id, &kcontrols->id) ||
> > +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
> >  		    put_user(id, &ucontrols->id) ||
> > -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
> > -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> > +		    assign_in_user(&ucontrols->size,
> > +				   (unsigned int __user *)&kcontrols->size) ||
> > +		    copy_in_user(&ucontrols->reserved2,
> > +				 (unsigned int __user *)&kcontrols->reserved2,
> >  				 sizeof(ucontrols->reserved2)))
> >  			return -EFAULT;
> >  
> > @@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
> >  		if (ctrl_is_pointer(file, id))
> >  			size -= sizeof(ucontrols->value64);
> >  
> > -		if (copy_in_user(ucontrols, kcontrols, size))
> > +		if (copy_in_user(ucontrols,
> > +			         (unsigned int __user *)kcontrols, size))  
> 
> This is rather ugly. Would it be better to do something like this:
> 
> 	struct v4l2_ext_control __user *kcontrols
> 	struct v4l2_ext_control *kcontrols_tmp;
> 
> 	get_user(kcontrols_tmp, &kp->controls);
> 	kcontrols = (void __user __force *)kcontrols_tmp;

Nah, having two pointers with the same value, one with __user and another
one without it will make it really hard for people to review.

> 
> And then there is no need to change anything else.
> 
> Regardless of the chosen solution, this needs comments to explain what is going
> on here, just as with v4l2_buffer above.

Actually, the problem here happens before that. The problem
here (and on the previous one) is that get_user() expects that
the second argument to be a Kernelspace pointer.

So, in this routine, it does (simplified):

        struct v4l2_ext_control32 __user *ucontrols;
        struct v4l2_ext_control *kcontrols;

...
	get_user(kcontrols, &kp->controls);

Then, every time it needs to get something related to it,
it needs the __user, like here:

	if (get_user(id, (unsigned int __user *)&kcontrols->id)
	...

In this specific case, only copy_in_user() was missing it; all the other
__user casts are there already.


> Note: the whole 'u<foo>' and 'k<foo>' naming is now hopelessly out of date and
> confusing. It should really be '<foo>32' and '<foo>64' to denote 32 bit vs
> 64 bit layout. The pointers are now always in userspace, so 'k<foo>' no longer
> makes sense.

Yes, we need this change, but this should be at a separate patch.

I can do it, after we cleanup v4l2-compat-ioctl32.c from their namespace
mess.


> 
> >  			return -EFAULT;
> >  
> >  		ucontrols++;
> > @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
> >  	    assign_in_user(&kp->start_block, &up->start_block) ||
> >  	    assign_in_user(&kp->blocks, &up->blocks) ||
> >  	    get_user(tmp, &up->edid) ||
> > -	    put_user(compat_ptr(tmp), &kp->edid) ||
> > +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
> >  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >  		return -EFAULT;
> >  	return 0;
> > @@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
> >  	    assign_in_user(&up->start_block, &kp->start_block) ||
> >  	    assign_in_user(&up->blocks, &kp->blocks) ||
> >  	    get_user(edid, &kp->edid) ||
> > -	    put_user(ptr_to_compat(edid), &up->edid) ||
> > +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
> >  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >  		return -EFAULT;
> >  	return 0;
> >   
> 
> Otherwise this patch looks good.
> 
> Regards,
> 
> 	Hans

Would be ok if I fold (or add as a separate patch) the enclosed diff?

Thanks,
Mauro


diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 1057ab8ce2b6..5c3408bdfd89 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -617,6 +617,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 
 		if (num_planes == 0)
 			return 0;
+		/*
+		 * We needed to define uplane without user.
+		 * The reason is that v4l2-ioctl.c copies it from userspace
+		 * into Kernelspace, so it's definition at videodev2.h doesn't
+		 * have an __user markup. That makes get_user() to do wrong
+		 * casts, as pointed by smatch.
+		 * So, instead, declare it as ks, and pass it as an userspace
+		 * pointer to put_v4l2_plane32().
+		 */
 		if (get_user(uplane, &kp->m.planes))
 			return -EFAULT;
 		if (get_user(p, &up->m.planes))
@@ -861,6 +870,15 @@ static int put_v4l2_ext_controls32(struct file *file,
 	u32 n;
 	compat_caddr_t p;
 
+	/*
+	 * We needed to define kcontrols without user.
+	 * The reason is that v4l2-ioctl.c copies it from userspace
+	 * into Kernelspace, so it's definition at videodev2.h doesn't
+	 * have an __user markup. That makes get_user() & friends to do
+	 * wrong casts, as pointed by smatch.
+	 * So, instead, declare it as ks, and pass it as an userspace
+	 * pointer where needed.
+	 */
 	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
 	    assign_in_user(&up->which, &kp->which) ||
 	    get_user(count, &kp->count) ||

Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Hans Verkuil]

On 04/16/2018 04:50 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 14:03:45 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/13/2018 08:07 PM, Mauro Carvalho Chehab wrote:
>>> Smatch report several issues with bad __user annotations:
>>>
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
>>>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
>>>
>>> Fix them.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++++-------------
>>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>>> index d03a44d89649..0b9dfe7dbfe7 100644
>>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>>> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
>>>  			return -EFAULT;
>>>  		break;
>>>  	case V4L2_MEMORY_USERPTR:
>>> -		if (get_user(p, &up->m.userptr) ||
>>> -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
>>> +		if (get_user(p, &up->m.userptr)||
>>> +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>>>  			     &up32->m.userptr))
>>>  			return -EFAULT;
>>>  		break;
>>> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>>>  	u32 length;
>>>  	enum v4l2_memory memory;
>>>  	struct v4l2_plane32 __user *uplane32;
>>> -	struct v4l2_plane __user *uplane;
>>> +	struct v4l2_plane *uplane;  
>>
>> This needs a comment (either here or before the get_user below). It really is a
>> pointer to userspace, but since videodev2.h has it without __user (since it is
>> copied to kernel space in v4l2-ioctl.c) we need to define it as a regular pointer
>> here and cast it to a __user pointer in the put_v4l2_plane32() call.
>>
>> This is not trivially obvious, so a comment would help a lot.
> 
> 
> 
>>
>>>  	compat_caddr_t p;
>>>  	int ret;
>>>  
>>> @@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>>>  
>>>  		if (num_planes == 0)
>>>  			return 0;
>>> -
>>> -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
>>> +		if (get_user(uplane, &kp->m.planes))
>>>  			return -EFAULT;
>>>  		if (get_user(p, &up->m.planes))
>>>  			return -EFAULT;
>>>  		uplane32 = compat_ptr(p);
>>>  
>>>  		while (num_planes--) {
>>> -			ret = put_v4l2_plane32(uplane, uplane32, memory);
>>> +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
>>>  			if (ret)
>>>  				return ret;
>>>  			++uplane;
>>> @@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>>>  
>>>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>>>  	    get_user(tmp, &up->base) ||
>>> -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
>>> +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
>>>  	    assign_in_user(&kp->capability, &up->capability) ||
>>>  	    assign_in_user(&kp->flags, &up->flags) ||
>>>  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
>>> @@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>>>  
>>>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>>>  	    get_user(base, &kp->base) ||
>>> -	    put_user(ptr_to_compat(base), &up->base) ||
>>> +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
>>>  	    assign_in_user(&up->capability, &kp->capability) ||
>>>  	    assign_in_user(&up->flags, &kp->flags) ||
>>>  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
>>> @@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file,
>>>  				   struct v4l2_ext_controls32 __user *up)
>>>  {
>>>  	struct v4l2_ext_control32 __user *ucontrols;
>>> -	struct v4l2_ext_control __user *kcontrols;
>>> +	struct v4l2_ext_control *kcontrols;
>>>  	u32 count;
>>>  	u32 n;
>>>  	compat_caddr_t p;
>>> @@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file,
>>>  		unsigned int size = sizeof(*ucontrols);
>>>  		u32 id;
>>>  
>>> -		if (get_user(id, &kcontrols->id) ||
>>> +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
>>>  		    put_user(id, &ucontrols->id) ||
>>> -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
>>> -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
>>> +		    assign_in_user(&ucontrols->size,
>>> +				   (unsigned int __user *)&kcontrols->size) ||
>>> +		    copy_in_user(&ucontrols->reserved2,
>>> +				 (unsigned int __user *)&kcontrols->reserved2,
>>>  				 sizeof(ucontrols->reserved2)))
>>>  			return -EFAULT;
>>>  
>>> @@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
>>>  		if (ctrl_is_pointer(file, id))
>>>  			size -= sizeof(ucontrols->value64);
>>>  
>>> -		if (copy_in_user(ucontrols, kcontrols, size))
>>> +		if (copy_in_user(ucontrols,
>>> +			         (unsigned int __user *)kcontrols, size))  
>>
>> This is rather ugly. Would it be better to do something like this:
>>
>> 	struct v4l2_ext_control __user *kcontrols
>> 	struct v4l2_ext_control *kcontrols_tmp;
>>
>> 	get_user(kcontrols_tmp, &kp->controls);
>> 	kcontrols = (void __user __force *)kcontrols_tmp;
> 
> Nah, having two pointers with the same value, one with __user and another
> one without it will make it really hard for people to review.
> 
>>
>> And then there is no need to change anything else.
>>
>> Regardless of the chosen solution, this needs comments to explain what is going
>> on here, just as with v4l2_buffer above.
> 
> Actually, the problem here happens before that. The problem
> here (and on the previous one) is that get_user() expects that
> the second argument to be a Kernelspace pointer.

You mean the first argument? I'm actually not entirely sure if this is
correct since __get_user calls __typeof__, but that might not know about
__user. Anyway, it does not really matter for this patch and the uaccess.h
code gives me a headache :-)

> 
> So, in this routine, it does (simplified):
> 
>         struct v4l2_ext_control32 __user *ucontrols;
>         struct v4l2_ext_control *kcontrols;
> 
> ...
> 	get_user(kcontrols, &kp->controls);
> 
> Then, every time it needs to get something related to it,
> it needs the __user, like here:
> 
> 	if (get_user(id, (unsigned int __user *)&kcontrols->id)
> 	...
> 
> In this specific case, only copy_in_user() was missing it; all the other
> __user casts are there already.
> 
> 
>> Note: the whole 'u<foo>' and 'k<foo>' naming is now hopelessly out of date and
>> confusing. It should really be '<foo>32' and '<foo>64' to denote 32 bit vs
>> 64 bit layout. The pointers are now always in userspace, so 'k<foo>' no longer
>> makes sense.
> 
> Yes, we need this change, but this should be at a separate patch.
> 
> I can do it, after we cleanup v4l2-compat-ioctl32.c from their namespace
> mess.

Of course, this is a separate patch.

> 
> 
>>
>>>  			return -EFAULT;
>>>  
>>>  		ucontrols++;
>>> @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
>>>  	    assign_in_user(&kp->start_block, &up->start_block) ||
>>>  	    assign_in_user(&kp->blocks, &up->blocks) ||
>>>  	    get_user(tmp, &up->edid) ||
>>> -	    put_user(compat_ptr(tmp), &kp->edid) ||
>>> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
>>>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>>>  		return -EFAULT;
>>>  	return 0;
>>> @@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
>>>  	    assign_in_user(&up->start_block, &kp->start_block) ||
>>>  	    assign_in_user(&up->blocks, &kp->blocks) ||
>>>  	    get_user(edid, &kp->edid) ||
>>> -	    put_user(ptr_to_compat(edid), &up->edid) ||
>>> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
>>>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>>>  		return -EFAULT;
>>>  	return 0;
>>>   
>>
>> Otherwise this patch looks good.
>>
>> Regards,
>>
>> 	Hans
> 
> Would be ok if I fold (or add as a separate patch) the enclosed diff?
> 
> Thanks,
> Mauro
> 
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 1057ab8ce2b6..5c3408bdfd89 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -617,6 +617,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  
>  		if (num_planes == 0)
>  			return 0;
> +		/*
> +		 * We needed to define uplane without user.
> +		 * The reason is that v4l2-ioctl.c copies it from userspace
> +		 * into Kernelspace, so it's definition at videodev2.h doesn't
> +		 * have an __user markup. That makes get_user() to do wrong
> +		 * casts, as pointed by smatch.
> +		 * So, instead, declare it as ks, and pass it as an userspace
> +		 * pointer to put_v4l2_plane32().
> +		 */

I would propose this text:

"We need to define uplane without __user, even though it does point
to data in userspace here. The reason is that v4l2-ioctl.c copies it from
userspace to kernelspace, so its definition in videodev2.h doesn't have a
__user markup. Defining uplane with __user causes smatch warnings, so
instead declare it without __user and cast it as a userspace pointer to
put_v4l2_plane32()."

(yes, it's 'a user'. See https://ell.stackexchange.com/questions/26986/why-a-user-instead-of-an-user)

>  		if (get_user(uplane, &kp->m.planes))
>  			return -EFAULT;
>  		if (get_user(p, &up->m.planes))
> @@ -861,6 +870,15 @@ static int put_v4l2_ext_controls32(struct file *file,
>  	u32 n;
>  	compat_caddr_t p;
>  
> +	/*
> +	 * We needed to define kcontrols without user.
> +	 * The reason is that v4l2-ioctl.c copies it from userspace
> +	 * into Kernelspace, so it's definition at videodev2.h doesn't
> +	 * have an __user markup. That makes get_user() & friends to do
> +	 * wrong casts, as pointed by smatch.
> +	 * So, instead, declare it as ks, and pass it as an userspace
> +	 * pointer where needed.
> +	 */

"We need to define kcontrols without __user, even though it does point
to data in userspace here. The reason is that v4l2-ioctl.c copies it from
userspace to kernelspace, so its definition in videodev2.h doesn't have a
__user markup. Defining kcontrols with __user causes smatch warnings, so
instead declare it without __user and cast it as a userspace pointer where
needed."

>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    assign_in_user(&up->which, &kp->which) ||
>  	    get_user(count, &kp->count) ||
> 
> 

Regards,

	Hans

Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab]

> >>> @@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file,
> >>>  		if (ctrl_is_pointer(file, id))
> >>>  			size -= sizeof(ucontrols->value64);
> >>>  
> >>> -		if (copy_in_user(ucontrols, kcontrols, size))
> >>> +		if (copy_in_user(ucontrols,
> >>> +			         (unsigned int __user *)kcontrols, size))    
> >>
> >> This is rather ugly. Would it be better to do something like this:
> >>
> >> 	struct v4l2_ext_control __user *kcontrols
> >> 	struct v4l2_ext_control *kcontrols_tmp;
> >>
> >> 	get_user(kcontrols_tmp, &kp->controls);
> >> 	kcontrols = (void __user __force *)kcontrols_tmp;  
> > 
> > Nah, having two pointers with the same value, one with __user and another
> > one without it will make it really hard for people to review.
> >   
> >>
> >> And then there is no need to change anything else.
> >>
> >> Regardless of the chosen solution, this needs comments to explain what is going
> >> on here, just as with v4l2_buffer above.  
> > 
> > Actually, the problem here happens before that. The problem
> > here (and on the previous one) is that get_user() expects that
> > the second argument to be a Kernelspace pointer.  
> 
> You mean the first argument? I'm actually not entirely sure if this is
> correct since __get_user calls __typeof__, but that might not know about
> __user. Anyway, it does not really matter for this patch and the uaccess.h
> code gives me a headache :-)

__typeof__ gets the type of the var, but __user is not a type. it is
something else (see include/linux/compiler_types.h):

	#ifdef __CHECKER__
	# define __user		__attribute__((noderef, address_space(1)))
	...
	#else /* __CHECKER__ */
	# ifdef STRUCTLEAK_PLUGIN
	#  define __user __attribute__((user))
	# else
	#  define __user
	# endif
	...
	#endif /* __CHECKER__ */

It is only built for sparse/smatch and uses __attribute__ definition,
with I guess __type_of__() won't parse.

Also, on almost all places where get_user() is called, what you expect
is to fill a Kernelspace var with something that came from userspace.

So, it expects that the first argument to be a pointer to an area
that is at Kernelspace.

> 
> > 
> > So, in this routine, it does (simplified):
> > 
> >         struct v4l2_ext_control32 __user *ucontrols;
> >         struct v4l2_ext_control *kcontrols;
> > 
> > ...
> > 	get_user(kcontrols, &kp->controls);
> > 
> > Then, every time it needs to get something related to it,
> > it needs the __user, like here:
> > 
> > 	if (get_user(id, (unsigned int __user *)&kcontrols->id)
> > 	...
> > 
> > In this specific case, only copy_in_user() was missing it; all the other
> > __user casts are there already.
> > 
> >   
> >> Note: the whole 'u<foo>' and 'k<foo>' naming is now hopelessly out of date and
> >> confusing. It should really be '<foo>32' and '<foo>64' to denote 32 bit vs
> >> 64 bit layout. The pointers are now always in userspace, so 'k<foo>' no longer
> >> makes sense.  
> > 
> > Yes, we need this change, but this should be at a separate patch.
> > 
> > I can do it, after we cleanup v4l2-compat-ioctl32.c from their namespace
> > mess.  
> 
> Of course, this is a separate patch.
> 
> > 
> >   
> >>  
> >>>  			return -EFAULT;
> >>>  
> >>>  		ucontrols++;
> >>> @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
> >>>  	    assign_in_user(&kp->start_block, &up->start_block) ||
> >>>  	    assign_in_user(&kp->blocks, &up->blocks) ||
> >>>  	    get_user(tmp, &up->edid) ||
> >>> -	    put_user(compat_ptr(tmp), &kp->edid) ||
> >>> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
> >>>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> >>>  		return -EFAULT;
> >>>  	return 0;
> >>> @@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
> >>>  	    assign_in_user(&up->start_block, &kp->start_block) ||
> >>>  	    assign_in_user(&up->blocks, &kp->blocks) ||
> >>>  	    get_user(edid, &kp->edid) ||
> >>> -	    put_user(ptr_to_compat(edid), &up->edid) ||
> >>> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
> >>>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
> >>>  		return -EFAULT;
> >>>  	return 0;
> >>>     
> >>
> >> Otherwise this patch looks good.
> >>
> >> Regards,
> >>
> >> 	Hans  
> > 
> > Would be ok if I fold (or add as a separate patch) the enclosed diff?
> > 
> > Thanks,
> > Mauro
> > 
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index 1057ab8ce2b6..5c3408bdfd89 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -617,6 +617,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
> >  
> >  		if (num_planes == 0)
> >  			return 0;
> > +		/*
> > +		 * We needed to define uplane without user.
> > +		 * The reason is that v4l2-ioctl.c copies it from userspace
> > +		 * into Kernelspace, so it's definition at videodev2.h doesn't
> > +		 * have an __user markup. That makes get_user() to do wrong
> > +		 * casts, as pointed by smatch.
> > +		 * So, instead, declare it as ks, and pass it as an userspace
> > +		 * pointer to put_v4l2_plane32().
> > +		 */  
> 
> I would propose this text:
> 
> "We need to define uplane without __user, even though it does point
> to data in userspace here. The reason is that v4l2-ioctl.c copies it from
> userspace to kernelspace, so its definition in videodev2.h doesn't have a
> __user markup. Defining uplane with __user causes smatch warnings, so
> instead declare it without __user and cast it as a userspace pointer to
> put_v4l2_plane32()."
> 
> (yes, it's 'a user'. See https://ell.stackexchange.com/questions/26986/why-a-user-instead-of-an-user)
> 
> >  		if (get_user(uplane, &kp->m.planes))
> >  			return -EFAULT;
> >  		if (get_user(p, &up->m.planes))
> > @@ -861,6 +870,15 @@ static int put_v4l2_ext_controls32(struct file *file,
> >  	u32 n;
> >  	compat_caddr_t p;
> >  
> > +	/*
> > +	 * We needed to define kcontrols without user.
> > +	 * The reason is that v4l2-ioctl.c copies it from userspace
> > +	 * into Kernelspace, so it's definition at videodev2.h doesn't
> > +	 * have an __user markup. That makes get_user() & friends to do
> > +	 * wrong casts, as pointed by smatch.
> > +	 * So, instead, declare it as ks, and pass it as an userspace
> > +	 * pointer where needed.
> > +	 */  
> 
> "We need to define kcontrols without __user, even though it does point
> to data in userspace here. The reason is that v4l2-ioctl.c copies it from
> userspace to kernelspace, so its definition in videodev2.h doesn't have a
> __user markup. Defining kcontrols with __user causes smatch warnings, so
> instead declare it without __user and cast it as a userspace pointer where
> needed."

Works for me. I'll send patch 17/17 as a separate series, with the above
merged on the first patch.

Regards,
Mauro