[PATCH 0/7] fix IS_ERR_VALUE usage

[PATCH 0/7] fix IS_ERR_VALUE usage

[Andrzej Hajda]

Hi,

This small set of independent patches tries to fix incorrect
IS_ERR_VALUE macro usage. It fixes most usages leading to errors
as described in [1]. It also follows conclusion from the discussion
[1][2] - IS_ERR_VALUE should be used only with unsigned long type,
signed types should use comparison 'ret < 0'.

The patchset does not fix errors present in net/ethernet/freescale
and soc/fsq/qe drivers - these drivers mixes different types:
dma_addr_t, u32, unsigned long, fixing it properly seems to me more
challenging, maybe maintainers or brave volunteers can look it.

The list of missing fixes:
drivers/net/ethernet/freescale/fs_enet/mac-scc.c:149:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(fep -> ring_mem_addr)
drivers/net/ethernet/freescale/ucc_geth.c:2237:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2314:48-49: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_ring_offset [ j ])
drivers/net/ethernet/freescale/ucc_geth.c:2524:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2544:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_tx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2571:46-47: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> send_q_mem_reg_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2612:42-43: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> scheduler_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2659:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> tx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2696:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_glbl_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2715:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> thread_dat_rx_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2736:54-55: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_fw_statistics_pram_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2756:53-54: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_irq_coalescing_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2822:44-45: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> rx_bd_qs_tbl_offset)
drivers/net/ethernet/freescale/ucc_geth.c:2908:47-48: WARNING: incorrect argument type in IS_ERR_VALUE(ugeth -> exf_glbl_param_offset)
drivers/net/ethernet/freescale/ucc_geth.c:292:36-37: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_offset)
drivers/net/ethernet/freescale/ucc_geth.c:3042:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(init_enet_pram_offset)
drivers/soc/fsl/qe/ucc_fast.c:271:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_tx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_fast.c:284:60-61: WARNING: incorrect argument type in IS_ERR_VALUE(uccf -> ucc_fast_rx_virtual_fifo_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:186:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> us_pram_offset)
drivers/soc/fsl/qe/ucc_slow.c:213:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> rx_base_offset)
drivers/soc/fsl/qe/ucc_slow.c:224:38-39: WARNING: incorrect argument type in IS_ERR_VALUE(uccs -> tx_base_offset)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Regards
Andrzej


Andrzej Hajda (7):
  netfilter: fix IS_ERR_VALUE usage
  MIPS: module: fix incorrect IS_ERR_VALUE macro usages
  drivers: char: mem: fix IS_ERROR_VALUE usage
  atmel-isi: fix IS_ERR_VALUE usage
  serial: clps711x: fix IS_ERR_VALUE usage
  fbdev: exynos: fix IS_ERR_VALUE usage
  usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage

 arch/mips/kernel/module-rela.c                |  2 +-
 arch/mips/kernel/module.c                     |  2 +-
 drivers/char/mem.c                            |  2 +-
 drivers/media/platform/soc_camera/atmel-isi.c |  4 ++--
 drivers/tty/serial/clps711x.c                 | 14 ++++++++------
 drivers/usb/gadget/udc/fsl_qe_udc.c           |  2 +-
 drivers/video/fbdev/exynos/exynos_mipi_dsi.c  |  6 +++---
 include/linux/netfilter/x_tables.h            |  6 +++---
 net/ipv4/netfilter/arp_tables.c               | 11 +++++++----
 net/ipv4/netfilter/ip_tables.c                | 12 ++++++++----
 net/ipv6/netfilter/ip6_tables.c               | 13 +++++++++----
 11 files changed, 44 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH 1/7] netfilter: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE should be used only with unsigned long type.
Otherwise it can work incorrectly. To achieve this function
xt_percpu_counter_alloc is modified to return unsigned long,
and its result is assigned to temporary variable to perform
error checking, before assigning to .pcnt field.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 include/linux/netfilter/x_tables.h |  6 +++---
 net/ipv4/netfilter/arp_tables.c    | 11 +++++++----
 net/ipv4/netfilter/ip_tables.c     | 12 ++++++++----
 net/ipv6/netfilter/ip6_tables.c    | 13 +++++++++----
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index c557741..79d4306 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -370,16 +370,16 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
  * allows us to return 0 for single core systems without forcing
  * callers to deal with SMP vs. NONSMP issues.
  */
-static inline u64 xt_percpu_counter_alloc(void)
+static inline unsigned long xt_percpu_counter_alloc(void)
 {
 	if (nr_cpu_ids > 1) {
 		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
 						    sizeof(struct xt_counters));
 
 		if (res == NULL)
-			return (u64) -ENOMEM;
+			return -ENOMEM;
 
-		return (u64) (__force unsigned long) res;
+		return (__force unsigned long) res;
 	}
 
 	return 0;
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac..6dcc208 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -521,14 +521,16 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 	struct xt_entry_target *t;
 	struct xt_target *target;
 	int ret;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	t = arpt_get_target(e);
 	target = xt_request_find_target(NFPROTO_ARP, t->u.user.name,
@@ -1423,11 +1425,12 @@ static int translate_compat_table(const char *name,
 
 	i = 0;
 	xt_entry_foreach(iter1, entry1, newinfo->size) {
-		iter1->counters.pcnt = xt_percpu_counter_alloc();
-		if (IS_ERR_VALUE(iter1->counters.pcnt)) {
+		unsigned long pcnt = xt_percpu_counter_alloc();
+		if (IS_ERR_VALUE(pcnt)) {
 			ret = -ENOMEM;
 			break;
 		}
+		iter1->counters.pcnt = pcnt;
 
 		ret = check_target(iter1, name);
 		if (ret != 0) {
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..ad57c78 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -665,14 +665,16 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1609,10 +1611,12 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 	struct xt_mtchk_param mtpar;
 	unsigned int j;
 	int ret = 0;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf..4291c8d 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -678,14 +678,16 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 	unsigned int j;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
 	ret = check_entry(e, name);
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1619,10 +1621,13 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 	int ret = 0;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
+	unsigned long pcnt;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	pcnt = xt_percpu_counter_alloc();
+	if (IS_ERR_VALUE(pcnt))
 		return -ENOMEM;
+	e->counters.pcnt = pcnt;
+
 	j = 0;
 	mtpar.net	= net;
 	mtpar.table     = name;
-- 
1.9.1

[PATCH 2/7] MIPS: module: fix incorrect IS_ERR_VALUE macro usages

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
Specifically it works incorrectly with longer types.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 arch/mips/kernel/module-rela.c | 2 +-
 arch/mips/kernel/module.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/module-rela.c b/arch/mips/kernel/module-rela.c
index 2b70723..08100dc 100644
--- a/arch/mips/kernel/module-rela.c
+++ b/arch/mips/kernel/module-rela.c
@@ -125,7 +125,7 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		/* This is the symbol it is referring to */
 		sym = (Elf_Sym *)sechdrs[symindex].sh_addr
 			+ ELF_MIPS_R_SYM(rel[i]);
-		if (IS_ERR_VALUE(sym->st_value)) {
+		if (sym->st_value >= -MAX_ERRNO) {
 			/* Ignore unresolved weak symbol */
 			if (ELF_ST_BIND(sym->st_info) == STB_WEAK)
 				continue;
diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 1833f51..2ba73bf4 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -214,7 +214,7 @@ int apply_relocate(Elf_Shdr *sechdrs, const char *strtab,
 		/* This is the symbol it is referring to */
 		sym = (Elf_Sym *)sechdrs[symindex].sh_addr
 			+ ELF_MIPS_R_SYM(rel[i]);
-		if (IS_ERR_VALUE(sym->st_value)) {
+		if (sym->st_value >= -MAX_ERRNO) {
 			/* Ignore unresolved weak symbol */
 			if (ELF_ST_BIND(sym->st_info) == STB_WEAK)
 				continue;
-- 
1.9.1

[PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
Specifically it works incorrectly with longer types.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/char/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 4f6f94c..71025c2 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -695,7 +695,7 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 		offset += file->f_pos;
 	case SEEK_SET:
 		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
-		if (IS_ERR_VALUE((unsigned long long)offset)) {
+		if ((unsigned long long)offset >= -MAX_ERRNO) {
 			ret = -EOVERFLOW;
 			break;
 		}
-- 
1.9.1

[PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
For signed types comparison 'ret < 0' should be used.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/media/platform/soc_camera/atmel-isi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 1af779e..ab2d9b9 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -1026,7 +1026,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
 
 static int atmel_isi_probe(struct platform_device *pdev)
 {
-	unsigned int irq;
+	int irq;
 	struct atmel_isi *isi;
 	struct resource *regs;
 	int ret, i;
@@ -1086,7 +1086,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
 		isi->width_flags |= 1 << 9;
 
 	irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(irq)) {
+	if (irq < 0) {
 		ret = irq;
 		goto err_req_irq;
 	}
-- 
1.9.1

[PATCH 5/7] serial: clps711x: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
For signed types comparison 'ret < 0' should be used.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/tty/serial/clps711x.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index b3a4e0c..0096ebe 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -467,13 +467,15 @@ static int uart_clps711x_probe(struct platform_device *pdev)
 	if (IS_ERR(s->port.membase))
 		return PTR_ERR(s->port.membase);
 
-	s->port.irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(s->port.irq))
-		return s->port.irq;
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		return ret;
+	s->port.irq = ret;
 
-	s->rx_irq = platform_get_irq(pdev, 1);
-	if (IS_ERR_VALUE(s->rx_irq))
-		return s->rx_irq;
+	ret = platform_get_irq(pdev, 1);
+	if (ret < 0)
+		return ret;
+	s->rx_irq = ret;
 
 	if (!np) {
 		char syscon_name[9];
-- 
1.9.1

[PATCH 6/7] fbdev: exynos: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
For signed types comparison 'ret < 0' should be used.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
index b527fe4..951b592 100644
--- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
@@ -402,12 +402,12 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
 		goto error;
 	}
 
-	dsim->irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(dsim->irq)) {
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to request dsim irq resource\n");
-		ret = -EINVAL;
 		goto error;
 	}
+	dsim->irq = ret;
 
 	init_completion(&dsim_wr_comp);
 	init_completion(&dsim_rd_comp);
-- 
1.9.1

[PATCH 7/7] usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE macro should be used only with unsigned long type.
Otherwise it can work incorrectly.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/usb/gadget/udc/fsl_qe_udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 53c0692..93d28cb 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -2340,7 +2340,7 @@ static struct qe_udc *qe_udc_config(struct platform_device *ofdev)
 {
 	struct qe_udc *udc;
 	struct device_node *np = ofdev->dev.of_node;
-	unsigned int tmp_addr = 0;
+	unsigned long tmp_addr = 0;
 	struct usb_device_para __iomem *usbpram;
 	unsigned int i;
 	u64 size;
-- 
1.9.1

Re: [PATCH 6/7] fbdev: exynos: fix IS_ERR_VALUE usage

[Tomi Valkeinen]

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



On 15/02/16 16:35, Andrzej Hajda wrote:
> IS_ERR_VALUE macro should be used only with unsigned long type.
> For signed types comparison 'ret < 0' should be used.
> 
> The patch follows conclusion from discussion on LKML [1][2].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
> index b527fe4..951b592 100644
> --- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
> @@ -402,12 +402,12 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
>  		goto error;
>  	}
>  
> -	dsim->irq = platform_get_irq(pdev, 0);
> -	if (IS_ERR_VALUE(dsim->irq)) {
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to request dsim irq resource\n");
> -		ret = -EINVAL;
>  		goto error;
>  	}
> +	dsim->irq = ret;
>  
>  	init_completion(&dsim_wr_comp);
>  	init_completion(&dsim_rd_comp);
> 

Thanks, queued for 4.6.

 Tomi


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

Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage

[Al Viro]

On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote:
> IS_ERR_VALUE should be used only with unsigned long type.
> Otherwise it can work incorrectly. To achieve this function
> xt_percpu_counter_alloc is modified to return unsigned long,
> and its result is assigned to temporary variable to perform
> error checking, before assigning to .pcnt field.

	Wrong fix, IMO.  Just have an anon union of u64 pcnt and
struct xt_counters __percpu *pcpu in there.  And make this

> +static inline unsigned long xt_percpu_counter_alloc(void)
>  {
>  	if (nr_cpu_ids > 1) {
>  		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
>  						    sizeof(struct xt_counters));
>  
>  		if (res == NULL)
> -			return (u64) -ENOMEM;
> +			return -ENOMEM;
>  
> -		return (u64) (__force unsigned long) res;
> +		return (__force unsigned long) res;
>  	}
>  
>  	return 0;

take struct xt_counters * and return 0 or -ENOMEM.  Storing the result of
allocation in ->pcpu of passed structure.

I mean, look at the callers -

> -	e->counters.pcnt = xt_percpu_counter_alloc();
> -	if (IS_ERR_VALUE(e->counters.pcnt))
> +	pcnt = xt_percpu_counter_alloc();
> +	if (IS_ERR_VALUE(pcnt))
>  		return -ENOMEM;
> +	e->counters.pcnt = pcnt;

should be
	if (xt_percpu_counter_alloc(&e->counters) < 0)
		return -ENOMEM;

and similar for the rest of callers.  Moreover, if you look at the _users_
of that fields, you'll see that a bunch of those actually want to use
->pcpu instead of doing all those casts.

Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need
to figure out what's going on in that place", which does include reading
through the code.  Mechanical "solutions" like that only hide the problem.

Re: [PATCH 3/7] drivers: char: mem: fix IS_ERROR_VALUE usage

[Al Viro]

On Mon, Feb 15, 2016 at 03:35:21PM +0100, Andrzej Hajda wrote:
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 4f6f94c..71025c2 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -695,7 +695,7 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>  		offset += file->f_pos;
>  	case SEEK_SET:
>  		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
> -		if (IS_ERR_VALUE((unsigned long long)offset)) {
> +		if ((unsigned long long)offset >= -MAX_ERRNO) {
>  			ret = -EOVERFLOW;
>  			break;
>  		}

ACK

Re: [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage

[Al Viro]

On Mon, Feb 15, 2016 at 03:35:22PM +0100, Andrzej Hajda wrote:
> IS_ERR_VALUE macro should be used only with unsigned long type.
> For signed types comparison 'ret < 0' should be used.
> 
> The patch follows conclusion from discussion on LKML [1][2].

ACK

Re: [PATCH 5/7] serial: clps711x: fix IS_ERR_VALUE usage

[Al Viro]

On Mon, Feb 15, 2016 at 03:35:23PM +0100, Andrzej Hajda wrote:
> IS_ERR_VALUE macro should be used only with unsigned long type.
> For signed types comparison 'ret < 0' should be used.

ACK

Re: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage

[Andrzej Hajda]

On 02/17/2016 03:31 AM, Al Viro wrote:
> On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote:
>> IS_ERR_VALUE should be used only with unsigned long type.
>> Otherwise it can work incorrectly. To achieve this function
>> xt_percpu_counter_alloc is modified to return unsigned long,
>> and its result is assigned to temporary variable to perform
>> error checking, before assigning to .pcnt field.
> 	Wrong fix, IMO.  Just have an anon union of u64 pcnt and
> struct xt_counters __percpu *pcpu in there.  And make this
>
>> +static inline unsigned long xt_percpu_counter_alloc(void)
>>  {
>>  	if (nr_cpu_ids > 1) {
>>  		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
>>  						    sizeof(struct xt_counters));
>>  
>>  		if (res == NULL)
>> -			return (u64) -ENOMEM;
>> +			return -ENOMEM;
>>  
>> -		return (u64) (__force unsigned long) res;
>> +		return (__force unsigned long) res;
>>  	}
>>  
>>  	return 0;
> take struct xt_counters * and return 0 or -ENOMEM.  Storing the result of
> allocation in ->pcpu of passed structure.
>
> I mean, look at the callers -
>
>> -	e->counters.pcnt = xt_percpu_counter_alloc();
>> -	if (IS_ERR_VALUE(e->counters.pcnt))
>> +	pcnt = xt_percpu_counter_alloc();
>> +	if (IS_ERR_VALUE(pcnt))
>>  		return -ENOMEM;
>> +	e->counters.pcnt = pcnt;
> should be
> 	if (xt_percpu_counter_alloc(&e->counters) < 0)
> 		return -ENOMEM;
>
> and similar for the rest of callers.  Moreover, if you look at the _users_
> of that fields, you'll see that a bunch of those actually want to use
> ->pcpu instead of doing all those casts.
>
> Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need
> to figure out what's going on in that place", which does include reading
> through the code.  Mechanical "solutions" like that only hide the problem.
>
>
I just tried to make the patch the least invasive :)

The problem with your proposition is that struct xt_counters is exposed
to userspace as well as the structs containing it:
struct arpt_entry,
struct ipt_entry,
struct ip6t_entry

Mixing __percpu pointer into these structures seems problematic.
Maybe it would be better to skip adding union and do ugly casting
in xt_percpu_counter_alloc(struct xt_counters *) and friends.

Regards
Andrzej

Re: [PATCH 0/7] fix IS_ERR_VALUE usage

[Arnd Bergmann]

On Monday 15 February 2016 15:35:18 Andrzej Hajda wrote:
> 
> Andrzej Hajda (7):
>   netfilter: fix IS_ERR_VALUE usage
>   MIPS: module: fix incorrect IS_ERR_VALUE macro usages
>   drivers: char: mem: fix IS_ERROR_VALUE usage
>   atmel-isi: fix IS_ERR_VALUE usage
>   serial: clps711x: fix IS_ERR_VALUE usage
>   fbdev: exynos: fix IS_ERR_VALUE usage
>   usb: gadget: fsl_qe_udc: fix IS_ERR_VALUE usage
> 

Can you Cc me the next time on all of the patches? I only got
three of them this time.

	Arnd

[PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

[Andrzej Hajda]

IS_ERR_VALUE should be used only with unsigned long type. Otherwise
it can work incorrectly. To achieve this function xt_percpu_counter_alloc
is modified to return only error code, pointer to counters is passed as an
argument. Helper union have been created to avoid ugly typecasting and
make code more readable.

The patch follows conclusion from discussion on LKML [1][2].

[1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
[2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi Al,

This is prettier version, at least in my opinion :)
It uses external union to avoid touching uapi structures.

Regards
Andrzej

 include/linux/netfilter/x_tables.h | 41 ++++++++++++++++++++------------------
 net/ipv4/netfilter/arp_tables.c    | 18 ++++++++---------
 net/ipv4/netfilter/ip_tables.c     | 20 +++++++++----------
 net/ipv6/netfilter/ip6_tables.c    | 21 +++++++++----------
 4 files changed, 51 insertions(+), 49 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index c557741..82faecb 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -357,44 +357,47 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
 	return ret;
 }
 
-
-/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
- * real (percpu) counter.  On !SMP, its just the packet count,
- * so nothing needs to be done there.
- *
- * xt_percpu_counter_alloc returns the address of the percpu
- * counter, or 0 on !SMP. We force an alignment of 16 bytes
- * so that bytes/packets share a common cache line.
- *
- * Hence caller must use IS_ERR_VALUE to check for error, this
- * allows us to return 0 for single core systems without forcing
- * callers to deal with SMP vs. NONSMP issues.
+/*
+ * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
+ * counter.  On !SMP, it is just the packet count. union ext_counters is used
+ * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
+ * structures as these are exposed to userspace.
  */
-static inline u64 xt_percpu_counter_alloc(void)
+union xt_smp_counters {
+	struct xt_counters counters;
+	struct xt_counters __percpu *smp_counters;
+};
+
+static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
+{
+	return container_of(cnt, union xt_smp_counters, counters);
+}
+
+static inline int xt_percpu_counter_alloc(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1) {
 		void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
 						    sizeof(struct xt_counters));
 
 		if (res == NULL)
-			return (u64) -ENOMEM;
+			return -ENOMEM;
 
-		return (u64) (__force unsigned long) res;
+		to_xt_smp_counters(cnt)->smp_counters = res;
 	}
 
 	return 0;
 }
-static inline void xt_percpu_counter_free(u64 pcnt)
+static inline void xt_percpu_counter_free(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1)
-		free_percpu((void __percpu *) (unsigned long) pcnt);
+		free_percpu(to_xt_smp_counters(cnt)->smp_counters);
 }
 
 static inline struct xt_counters *
 xt_get_this_cpu_counter(struct xt_counters *cnt)
 {
 	if (nr_cpu_ids > 1)
-		return this_cpu_ptr((void __percpu *) (unsigned long) cnt->pcnt);
+		return this_cpu_ptr(to_xt_smp_counters(cnt)->smp_counters);
 
 	return cnt;
 }
@@ -403,7 +406,7 @@ static inline struct xt_counters *
 xt_get_per_cpu_counter(struct xt_counters *cnt, unsigned int cpu)
 {
 	if (nr_cpu_ids > 1)
-		return per_cpu_ptr((void __percpu *) (unsigned long) cnt->pcnt, cpu);
+		return per_cpu_ptr(to_xt_smp_counters(cnt)->smp_counters, cpu);
 
 	return cnt;
 }
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac..be589e5 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -526,9 +526,9 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	t = arpt_get_target(e);
 	target = xt_request_find_target(NFPROTO_ARP, t->u.user.name,
@@ -547,7 +547,7 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 err:
 	module_put(t->u.kernel.target->me);
 out:
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -625,7 +625,7 @@ static inline void cleanup_entry(struct arpt_entry *e)
 	if (par.target->destroy != NULL)
 		par.target->destroy(&par);
 	module_put(par.target->me);
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1423,15 +1423,13 @@ static int translate_compat_table(const char *name,
 
 	i = 0;
 	xt_entry_foreach(iter1, entry1, newinfo->size) {
-		iter1->counters.pcnt = xt_percpu_counter_alloc();
-		if (IS_ERR_VALUE(iter1->counters.pcnt)) {
-			ret = -ENOMEM;
+		ret = xt_percpu_counter_alloc(&iter1->counters);
+		if (ret < 0)
 			break;
-		}
 
 		ret = check_target(iter1, name);
 		if (ret != 0) {
-			xt_percpu_counter_free(iter1->counters.pcnt);
+			xt_percpu_counter_free(iter1->counters);
 			break;
 		}
 		++i;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..5f3f96b 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -670,9 +670,9 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -711,7 +711,7 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -797,7 +797,7 @@ cleanup_entry(struct ipt_entry *e, struct net *net)
 	if (par.target->destroy != NULL)
 		par.target->destroy(&par);
 	module_put(par.target->me);
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1608,11 +1608,11 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 	struct xt_entry_match *ematch;
 	struct xt_mtchk_param mtpar;
 	unsigned int j;
-	int ret = 0;
+	int ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -1639,7 +1639,7 @@ compat_check_entry(struct ipt_entry *e, struct net *net, const char *name)
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf..25b6a90 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -683,9 +683,9 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 	if (ret)
 		return ret;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
 
 	j = 0;
 	mtpar.net	= net;
@@ -723,7 +723,7 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
@@ -809,7 +809,7 @@ static void cleanup_entry(struct ip6t_entry *e, struct net *net)
 		par.target->destroy(&par);
 	module_put(par.target->me);
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 }
 
 /* Checks and translates the user-supplied table segment (held in
@@ -1616,13 +1616,14 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 			      const char *name)
 {
 	unsigned int j;
-	int ret = 0;
+	int ret;
 	struct xt_mtchk_param mtpar;
 	struct xt_entry_match *ematch;
 
-	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
-		return -ENOMEM;
+	ret = xt_percpu_counter_alloc(&e->counters);
+	if (ret < 0)
+		return ret;
+
 	j = 0;
 	mtpar.net	= net;
 	mtpar.table     = name;
@@ -1648,7 +1649,7 @@ static int compat_check_entry(struct ip6t_entry *e, struct net *net,
 		cleanup_match(ematch, net);
 	}
 
-	xt_percpu_counter_free(e->counters.pcnt);
+	xt_percpu_counter_free(&e->counters);
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

[Arnd Bergmann]

On Wednesday 17 February 2016 13:41:29 Andrzej Hajda wrote:
> IS_ERR_VALUE should be used only with unsigned long type. Otherwise
> it can work incorrectly. To achieve this function xt_percpu_counter_alloc
> is modified to return only error code, pointer to counters is passed as an
> argument. Helper union have been created to avoid ugly typecasting and
> make code more readable.
> 
> The patch follows conclusion from discussion on LKML [1][2].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581

I think it would be helpful to mention here how the current code is
actually broken, i.e. that we set the u64 value to (u64)-ENOMEM
on failure but then compare it to (unsigned long)-MAX_ERRNO, which
is much smaller on a 32-bit architecture, and basically relies on
never even needing the range of the u64 variable.

It works because we only do this comparison at allocation time, while
in the non-SMP case it might be larger than (unsigned long)-MAX_ERRNO
later but then we don't do the IS_ERR_VALUE comparison any more.

> -/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
> - * real (percpu) counter.  On !SMP, its just the packet count,
> - * so nothing needs to be done there.
> - *
> - * xt_percpu_counter_alloc returns the address of the percpu
> - * counter, or 0 on !SMP. We force an alignment of 16 bytes
> - * so that bytes/packets share a common cache line.
> - *
> - * Hence caller must use IS_ERR_VALUE to check for error, this
> - * allows us to return 0 for single core systems without forcing
> - * callers to deal with SMP vs. NONSMP issues.
> +/*
> + * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
> + * counter.  On !SMP, it is just the packet count. union ext_counters is used
> + * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
> + * structures as these are exposed to userspace.
>   */
> -static inline u64 xt_percpu_counter_alloc(void)
> +union xt_smp_counters {
> +	struct xt_counters counters;
> +	struct xt_counters __percpu *smp_counters;
> +};
> +
> +static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
> +{
> +	return container_of(cnt, union xt_smp_counters, counters);
> +}

The union is a bit ugly, but I can't think of a much better
way to do this.

However, could you put the union into the three users (struct arpt_entry
etc) to avoid having to cast the inner structure into the union using
container_of()? It doesn't feel right to use container_of() in this
way here.

	Arnd

Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

[Andrzej Hajda]

On 02/17/2016 02:42 PM, Arnd Bergmann wrote:
> On Wednesday 17 February 2016 13:41:29 Andrzej Hajda wrote:
>> IS_ERR_VALUE should be used only with unsigned long type. Otherwise
>> it can work incorrectly. To achieve this function xt_percpu_counter_alloc
>> is modified to return only error code, pointer to counters is passed as an
>> argument. Helper union have been created to avoid ugly typecasting and
>> make code more readable.
>>
>> The patch follows conclusion from discussion on LKML [1][2].
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
>> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581
> I think it would be helpful to mention here how the current code is
> actually broken, i.e. that we set the u64 value to (u64)-ENOMEM
> on failure but then compare it to (unsigned long)-MAX_ERRNO, which
> is much smaller on a 32-bit architecture, and basically relies on
> never even needing the range of the u64 variable.
>
> It works because we only do this comparison at allocation time, while
> in the non-SMP case it might be larger than (unsigned long)-MAX_ERRNO
> later but then we don't do the IS_ERR_VALUE comparison any more.
>
>> -/* On SMP, ip(6)t_entry->counters.pcnt holds address of the
>> - * real (percpu) counter.  On !SMP, its just the packet count,
>> - * so nothing needs to be done there.
>> - *
>> - * xt_percpu_counter_alloc returns the address of the percpu
>> - * counter, or 0 on !SMP. We force an alignment of 16 bytes
>> - * so that bytes/packets share a common cache line.
>> - *
>> - * Hence caller must use IS_ERR_VALUE to check for error, this
>> - * allows us to return 0 for single core systems without forcing
>> - * callers to deal with SMP vs. NONSMP issues.
>> +/*
>> + * On SMP, (ip|ip6|arp)t_entry->counters holds address of the real (percpu)
>> + * counter.  On !SMP, it is just the packet count. union ext_counters is used
>> + * to model this ambiguity in kernel without changing (ip|ip6|arp)t_entry
>> + * structures as these are exposed to userspace.
>>   */
>> -static inline u64 xt_percpu_counter_alloc(void)
>> +union xt_smp_counters {
>> +	struct xt_counters counters;
>> +	struct xt_counters __percpu *smp_counters;
>> +};
>> +
>> +static inline union xt_smp_counters *to_xt_smp_counters(struct xt_counters *cnt)
>> +{
>> +	return container_of(cnt, union xt_smp_counters, counters);
>> +}
> The union is a bit ugly, but I can't think of a much better
> way to do this.
>
> However, could you put the union into the three users (struct arpt_entry
> etc) to avoid having to cast the inner structure into the union using
> container_of()? It doesn't feel right to use container_of() in this
> way here.
>
> 	Arnd
>
>
I am not sure if you are aware of the fact these structures are exposed
to user
space. Is it OK to add such unions to them?


Regards
Andrzej

Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

[Arnd Bergmann]

On Wednesday 17 February 2016 15:54:11 Andrzej Hajda wrote:
> > However, could you put the union into the three users (struct arpt_entry
> > etc) to avoid having to cast the inner structure into the union using
> > container_of()? It doesn't feel right to use container_of() in this
> > way here.
> >
> >
> I am not sure if you are aware of the fact these structures are exposed
> to user
> space. Is it OK to add such unions to them?
> 

You are right, that would be odd. My first idea was actually to put
a union into struct xt_counters, and I did notice that this was
exposed to user space so I did not mention it.

Putting a union into arpt_entry etc would be worse then. The only
alternative I see would be to define xt_counters as

struct xt_counters {
#ifndef __KERNEL__
        __u64 pcnt, bcnt;                       /* Packet and byte counters */
#else
	union {
		__u64 pcnt;
		struct xt_counters __percpu *xt_smp_counters;
	};
	__u64 bcnt;
#endif
};

but that is still really ugly, and no real improvement over your
approach.

One really simple fix would be to basically open-code a correct
version of IS_ERR_VALUE specifically for xt_counters and leave
everything using the __u64 hack:

-static inline u64 xt_percpu_counter_alloc(void)
+static inline int xt_percpu_counter_alloc(struct xt_counters *cnt)
 {
        if (nr_cpu_ids > 1) {
                void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
                                                    sizeof(struct xt_counters));
 
                if (res == NULL)
-                       return (u64) -ENOMEM;
+                       return -ENOMEM;
 
-               return (u64) (__force unsigned long) res;
+               cnt->pcnt = (u64)(uintptr_t)res;
        }
 
        return 0;
 }

that avoids the union but keeps the implicit overloading of the
pcnt field, just local to the alloc/free functions.

	Arnd

Re: [PATCH 4/7] atmel-isi: fix IS_ERR_VALUE usage

[Guennadi Liakhovetski]

Hi Andrzej,

On Mon, 15 Feb 2016, Andrzej Hajda wrote:

> IS_ERR_VALUE macro should be used only with unsigned long type.
> For signed types comparison 'ret < 0' should be used.
> 
> The patch follows conclusion from discussion on LKML [1][2].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2120927
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2150581
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Thanks for the patch, but this one

https://lkml.org/lkml/2016/2/9/392

came a couple of days earlier. Unless there is an important reason to use 
yours, I'll use that one.

Thanks
Guennadi

> ---
>  drivers/media/platform/soc_camera/atmel-isi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 1af779e..ab2d9b9 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -1026,7 +1026,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
>  
>  static int atmel_isi_probe(struct platform_device *pdev)
>  {
> -	unsigned int irq;
> +	int irq;
>  	struct atmel_isi *isi;
>  	struct resource *regs;
>  	int ret, i;
> @@ -1086,7 +1086,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>  		isi->width_flags |= 1 << 9;
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (IS_ERR_VALUE(irq)) {
> +	if (irq < 0) {
>  		ret = irq;
>  		goto err_req_irq;
>  	}
> -- 
> 1.9.1
>