[PATCH] err.h: allow IS_ERR_VALUE to handle properly more types

[PATCH] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

Current implementation of IS_ERR_VALUE works correctly only with
following types:
- unsigned long, unsigned long long,
- short, int, long.
Other types are handled incorrectly either on 32-bit either on 64-bit
either on both architectures.
The patch fixes it by comparing argument with MAX_ERRNO casted
to argument's type for unsigned types and comparing with zero for signed
types. As a result all integer types bigger than char are handled properly.

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

I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 cases
it is used incorrectly, ie it can hide errors depending of 32/64 bit architecture.
Instead of fixing usage I propose to enhance the macro to cover more types.
And just for record: the macro is used 101 times with signed variables, I am not
sure if it should be preferred over simple comparison "ret < 0", but the new version
can do it as well.

In my previous attempt I have added type checking to the macro, I am not sure which
approach is better[1]. Anyway the previous patch did not catch developers attention.

[1]: http://permalink.gmane.org/gmane.linux.kernel/2111532

And below list of detected potential errors:
drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
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/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

Regards
Andrzej

 include/linux/err.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab..40eca39 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,9 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) ((typeof(x))(-1) < 0 \
+				? unlikely((x) < 0) \
+				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

Re: [PATCH] err.h: allow IS_ERR_VALUE to handle properly more types

[kbuild test robot]

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

Hi Andrzej,

[auto build test WARNING on v4.4-rc8]
[also build test WARNING on next-20160107]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Andrzej-Hajda/err-h-allow-IS_ERR_VALUE-to-handle-properly-more-types/20160107-230216
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/processor.h:31:0,
                    from arch/x86/include/asm/thread_info.h:52,
                    from include/linux/thread_info.h:54,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:59,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8723au/include/osdep_service.h:22,
                    from drivers/staging/rtl8723au/include/rtl8723a_sreset.h:18,
                    from drivers/staging/rtl8723au/hal/rtl8723a_sreset.c:17:
   include/linux/err.h: In function 'IS_ERR':
>> include/linux/err.h:21:42: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
    #define IS_ERR_VALUE(x) ((typeof(x))(-1) < 0 \
                                             ^
>> include/linux/err.h:37:9: note: in expansion of macro 'IS_ERR_VALUE'
     return IS_ERR_VALUE((unsigned long)ptr);
            ^
   In file included from include/linux/linkage.h:4:0,
                    from include/linux/preempt.h:9,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8723au/include/osdep_service.h:22,
                    from drivers/staging/rtl8723au/include/rtl8723a_sreset.h:18,
                    from drivers/staging/rtl8723au/hal/rtl8723a_sreset.c:17:
   include/linux/err.h:22:20: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
        ? unlikely((x) < 0) \
                       ^
   include/linux/compiler.h:166:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
>> include/linux/err.h:37:9: note: in expansion of macro 'IS_ERR_VALUE'
     return IS_ERR_VALUE((unsigned long)ptr);
            ^
   In file included from arch/x86/include/asm/processor.h:31:0,
                    from arch/x86/include/asm/thread_info.h:52,
                    from include/linux/thread_info.h:54,
                    from arch/x86/include/asm/preempt.h:6,
                    from include/linux/preempt.h:59,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8723au/include/osdep_service.h:22,
                    from drivers/staging/rtl8723au/include/rtl8723a_sreset.h:18,
                    from drivers/staging/rtl8723au/hal/rtl8723a_sreset.c:17:
   include/linux/err.h: In function 'IS_ERR_OR_NULL':
>> include/linux/err.h:21:42: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
    #define IS_ERR_VALUE(x) ((typeof(x))(-1) < 0 \
                                             ^
   include/linux/err.h:42:17: note: in expansion of macro 'IS_ERR_VALUE'
     return !ptr || IS_ERR_VALUE((unsigned long)ptr);
                    ^
   In file included from include/linux/linkage.h:4:0,
                    from include/linux/preempt.h:9,
                    from include/linux/spinlock.h:50,
                    from drivers/staging/rtl8723au/include/osdep_service.h:22,
                    from drivers/staging/rtl8723au/include/rtl8723a_sreset.h:18,
                    from drivers/staging/rtl8723au/hal/rtl8723a_sreset.c:17:
   include/linux/err.h:22:20: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
        ? unlikely((x) < 0) \
                       ^
   include/linux/compiler.h:166:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
   include/linux/err.h:42:17: note: in expansion of macro 'IS_ERR_VALUE'
     return !ptr || IS_ERR_VALUE((unsigned long)ptr);
                    ^

vim +21 include/linux/err.h

    15	 * error and pointer decisions.
    16	 */
    17	#define MAX_ERRNO	4095
    18	
    19	#ifndef __ASSEMBLY__
    20	
  > 21	#define IS_ERR_VALUE(x) ((typeof(x))(-1) < 0 \
    22					? unlikely((x) < 0) \
    23					: unlikely((x) >= (typeof(x))-MAX_ERRNO))
    24	
    25	static inline void * __must_check ERR_PTR(long error)
    26	{
    27		return (void *) error;
    28	}
    29	
    30	static inline long __must_check PTR_ERR(__force const void *ptr)
    31	{
    32		return (long) ptr;
    33	}
    34	
    35	static inline bool __must_check IS_ERR(__force const void *ptr)
    36	{
  > 37		return IS_ERR_VALUE((unsigned long)ptr);
    38	}
    39	
    40	static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 52542 bytes --]

[PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

Current implementation of IS_ERR_VALUE works correctly only with
following types:
- unsigned long,
- short, int, long.
Other types are handled incorrectly either on 32-bit either on 64-bit
either on both architectures.
The patch fixes it by comparing argument with MAX_ERRNO casted
to argument's type for unsigned types and comparing with zero for signed
types. As a result all integer types bigger than char are handled properly.

I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
architecture. Instead of fixing usage I propose to enhance the macro
to cover more types.
And just for the record: the macro is used 101 times with signed variables,
I am not sure if it should be preferred over simple comparison "ret < 0",
but the new version can do it as well.

And below list of detected potential errors:
drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
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/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v2:
 - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
 - expand commit message.
---
 include/linux/err.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab..43a6adb 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,9 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
+				? unlikely((x) < 0) \
+				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrew Morton]

On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:

> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
> 
> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
> architecture. Instead of fixing usage I propose to enhance the macro
> to cover more types.
> And just for the record: the macro is used 101 times with signed variables,
> I am not sure if it should be preferred over simple comparison "ret < 0",
> but the new version can do it as well.
> 
> And below list of detected potential errors:
>
> ...
>
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) < 0) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  

hm, seems complicated.  Can we simply cast the value to long?

#define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO)

and simplify that to

#define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO)

or something like that.

Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/02/2016 07:23 AM, Andrew Morton wrote:
> On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> Current implementation of IS_ERR_VALUE works correctly only with
>> following types:
>> - unsigned long,
>> - short, int, long.
>> Other types are handled incorrectly either on 32-bit either on 64-bit
>> either on both architectures.
>> The patch fixes it by comparing argument with MAX_ERRNO casted
>> to argument's type for unsigned types and comparing with zero for signed
>> types. As a result all integer types bigger than char are handled properly.
>>
>> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
>> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
>> architecture. Instead of fixing usage I propose to enhance the macro
>> to cover more types.
>> And just for the record: the macro is used 101 times with signed variables,
>> I am not sure if it should be preferred over simple comparison "ret < 0",
>> but the new version can do it as well.
>>
>> And below list of detected potential errors:
>>
>> ...
>>
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +				? unlikely((x) < 0) \
>> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>  
> hm, seems complicated.  Can we simply cast the value to long?
>
> #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO)
>
> and simplify that to
>
> #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO)
>
> or something like that.
It will not work with u32 on 64bit systems.

Short rationales behind my implementation:

1. Typical usage pattern of the macro looks like:
T x;
...
x = -ESOME_ERROR;
...
if (IS_ERR_VALUE(x))
    ...

In error assignment we have casting of -ESOME_ERROR to type T.
Casting of -MAX_ERRNO to the same type in the macro assures that
comparison will be sane, at least for types big enough. In short we ends
at following expression (for unsigned types):
    (T)-ESOME_ERROR >= (T)-MAX_ERRNO
In old implementation we ended at:
    (unsigned)(T)-ESOME_ERROR >= (unsigned)-MAX_ERRNO
Different castings for -ESOME_ERROR and for -MAX_ERRNO makes this
comparison incorrect for some types T.

2. Error checking is completely different for signed and unsigned vars:
a. signed are compared to 0: ret < 0.
b. unsigned are compared with some high value: ret >= (-MAX_ERRNO).
This dualism is clearly visible and emphasized in this implementation.
In old implementation IS_ERR_VALUE works correctly for some signed types
due to obscure C casting rules.

Summarizing: current implementation is short but tricky, answering why
it works/fails for certain types is quite challenging. On the other side
proposed implementation is longer but more straightforward, and of course
is correct for more types :)

Maybe, to make it more clear, it could be good to use separate macro for
signedness:

#define IS_SIGNED_TYPE(t) ((t)(-1) <= 0)

#define IS_ERR_VALUE(x) (IS_SIGNED_TYPE(typeof(x)) \
                ? unlikely((x) < 0) \
                : unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
Regards
Andrzej

Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrew Morton]

On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:

>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>  - expand commit message.
> ---
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..43a6adb 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) < 0) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))

I'm still getting a bunch of 

include/linux/err.h: In function 'IS_ERR':
include/linux/err.h:37: warning: comparison of unsigned expression < 0 is always false
include/linux/err.h: In function 'IS_ERR_OR_NULL':
include/linux/err.h:42: warning: comparison of unsigned expression < 0 is always false

with gcc-4.4.4.

Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/03/2016 01:33 AM, Andrew Morton wrote:
> On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>>  - expand commit message.
>> ---
>>  include/linux/err.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/err.h b/include/linux/err.h
>> index 56762ab..43a6adb 100644
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +				? unlikely((x) < 0) \
>> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
> I'm still getting a bunch of 
>
> include/linux/err.h: In function 'IS_ERR':
> include/linux/err.h:37: warning: comparison of unsigned expression < 0 is always false
> include/linux/err.h: In function 'IS_ERR_OR_NULL':
> include/linux/err.h:42: warning: comparison of unsigned expression < 0 is always false
>
> with gcc-4.4.4.
>
>

These warnings are false positives and gcc up to 4.7 emits them,
gcc 4.8(which I use) behaves correctly (at least on x86 and arm64). I
have tried
to use __builtin_choose_expr instead of ?: operator but it did not help,
although documentation says "the built-in function does not evaluate
the expression that is not chosen"[1].

The sanest gcc silencer I see for now is to replace:
        ? unlikely((x) < 0) \
with
        ? unlikely((x) <= -1) \

On the other side these warnings are caused by -Wtype-limits switch which
is disabled by default in kernel build and treated as broken by Linus [2].
Maybe it is good enough reason to disregard them? :)

Anyway, I will post another iteration.

[1]:
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005fchoose_005fexpr-4184
[2]: http://permalink.gmane.org/gmane.linux.kernel/2053963

Regards
Andrzej

[PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

Current implementation of IS_ERR_VALUE works correctly only with
following types:
- unsigned long,
- short, int, long.
Other types are handled incorrectly either on 32-bit either on 64-bit
either on both architectures.
The patch fixes it by comparing argument with MAX_ERRNO casted
to argument's type for unsigned types and comparing with zero for signed
types. As a result all integer types bigger than char are handled properly.

I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35
cases it is used incorrectly, ie it can hide errors depending of 32/64 bit
architecture. Instead of fixing usage I propose to enhance the macro
to cover more types.
And just for the record: the macro is used 101 times with signed variables,
I am not sure if it should be preferred over simple comparison "ret < 0",
but the new version can do it as well.

And below list of detected potential errors:
drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
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/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v3:
 - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
   older than 4.8,
v2:
 - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
 - expand commit message.
---
 include/linux/err.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab..b7d4a9f 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,9 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
+				? unlikely((x) <= -1) \
+				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
-- 
1.9.1

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..b7d4a9f 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +                               ? unlikely((x) <= -1) \
> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  
>  static inline void * __must_check ERR_PTR(long error)
>  {
> 

This has caused a warning to reappear that I had fixed before:

fs/gfs2/dir.c: In function 'get_first_leaf':
fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
   error = get_leaf(dip, leaf_no, bh_out);
         ^
fs/gfs2/dir.c: In function 'dir_split_leaf':
fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
  error = get_leaf(dip, leaf_no, &obh);

See my original patch that was applied at
http://www.gossamer-threads.com/lists/linux/kernel/2353964

Apparently the new version is complex enough to prevent gcc from doing
some optimizations it should do.

I have tried to come up with a new variant that does not bring
the warning back and that should work in all cases:

diff --git a/include/linux/err.h b/include/linux/err.h
index b7d4a9ff6342..bd4936a2c352 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,9 +18,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
-				? unlikely((x) <= -1) \
-				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
+#define IS_ERR_VALUE(x)	(unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {


I'm not sure if the cast to 'unsigned long long' might cause less
efficient code to be generated by gcc. I would hope that it is smart
enough to not actually extend shorter variables to 64 bit before
doing the comparison but I have not checked yet.

	Arnd

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/04/2016 01:40 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
>> diff --git a/include/linux/err.h b/include/linux/err.h
>> index 56762ab..b7d4a9f 100644
>> --- a/include/linux/err.h
>> +++ b/include/linux/err.h
>> @@ -18,7 +18,9 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>> +                               ? unlikely((x) <= -1) \
>> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>  
>>  static inline void * __must_check ERR_PTR(long error)
>>  {
>>
> This has caused a warning to reappear that I had fixed before:
>
> fs/gfs2/dir.c: In function 'get_first_leaf':
> fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
>    error = get_leaf(dip, leaf_no, bh_out);
>          ^
> fs/gfs2/dir.c: In function 'dir_split_leaf':
> fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
>   error = get_leaf(dip, leaf_no, &obh);

What gcc/arch/build options do you use? I cannot reproduce it in my
environment.

Regards
Andrzej

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Thursday 04 February 2016 15:44:51 Andrzej Hajda wrote:
> On 02/04/2016 01:40 PM, Arnd Bergmann wrote:
> > On Wednesday 03 February 2016 14:15:28 Andrzej Hajda wrote:
> >> diff --git a/include/linux/err.h b/include/linux/err.h
> >> index 56762ab..b7d4a9f 100644
> >> --- a/include/linux/err.h
> >> +++ b/include/linux/err.h
> >> @@ -18,7 +18,9 @@
> >>  
> >>  #ifndef __ASSEMBLY__
> >>  
> >> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> >> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> >> +                               ? unlikely((x) <= -1) \
> >> +                               : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> >>  
> >>  static inline void * __must_check ERR_PTR(long error)
> >>  {
> >>
> > This has caused a warning to reappear that I had fixed before:
> >
> > fs/gfs2/dir.c: In function 'get_first_leaf':
> > fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >    error = get_leaf(dip, leaf_no, bh_out);
> >          ^
> > fs/gfs2/dir.c: In function 'dir_split_leaf':
> > fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >   error = get_leaf(dip, leaf_no, &obh);
> 
> What gcc/arch/build options do you use? I cannot reproduce it in my
> environment.
> 

I use an ARM gcc-5.3 with an allmodconfig kernel and CONFIG_CC_OPTIMIZE_FOR_SIZE
disabled. I see the same warning with any gcc version since 4.9, but not earlier.

With the IS_ERR_VALUE() macro I sent, I don't see the warning on any gcc
version. I have now also checked that the behavior on x86 gcc-.4.9 is the
same that I see on ARM (I don't have a large collection of x86 gcc versions
though): same warning with linux-next, no warning with my version or after
reverting your patch.

	Arnd

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Thursday 04 February 2016 16:00:09 Arnd Bergmann wrote:
> With the IS_ERR_VALUE() macro I sent, I don't see the warning on any gcc
> version. I have now also checked that the behavior on x86 gcc-.4.9 is the
> same that I see on ARM (I don't have a large collection of x86 gcc versions
> though): same warning with linux-next, no warning with my version or after
> reverting your patch.
> 

Sorry, scratch that: I don't get this warning at all on x86 with gcc-4.9,
with any version of err.h.

	Arnd

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrew Morton]

On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> diff --git a/include/linux/err.h b/include/linux/err.h
> index b7d4a9ff6342..bd4936a2c352 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,9 +18,7 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> -				? unlikely((x) <= -1) \
> -				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
> +#define IS_ERR_VALUE(x)	(unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>  
>  static inline void * __must_check ERR_PTR(long error)
>  {
> 
> 
> I'm not sure if the cast to 'unsigned long long' might cause less
> efficient code to be generated by gcc. I would hope that it is smart
> enough to not actually extend shorter variables to 64 bit before
> doing the comparison but I have not checked yet.

I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
the text size from 9098 bytes to 9133.  That file has 11 instances of
IS_ERR_VALUE().  

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > index b7d4a9ff6342..bd4936a2c352 100644
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -18,9 +18,7 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> > -                             ? unlikely((x) <= -1) \
> > -                             : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> > +#define IS_ERR_VALUE(x)      (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
> >  
> >  static inline void * __must_check ERR_PTR(long error)
> >  {
> > 
> > 
> > I'm not sure if the cast to 'unsigned long long' might cause less
> > efficient code to be generated by gcc. I would hope that it is smart
> > enough to not actually extend shorter variables to 64 bit before
> > doing the comparison but I have not checked yet.
> 
> I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
> the text size from 9098 bytes to 9133.  That file has 11 instances of
> IS_ERR_VALUE().  

This seems to be because it brings back the logic to what it was before
in case of 'int' arguments. I checked the assembly output and found mine
to be identical to v4.4 in this case:

	 text    data     bss     dec     hex filename
v4.4	 9942    1872    2856   14670    394e drivers/nvmem/core.o
a.hajda  9922    1872    2856   14650    393a drivers/nvmem/core.o
arnd	 9942    1872    2856   14670    394e drivers/nvmem/core.o

Andrzej's version is a little shorter on ARM because in case of signed numbers
it only checks for negative values, rather than checking for values in the
[-MAX_ERRNO..-1] range. I think the original behavior is more logical
in this case, and my version restores it.

Looking at drivers/char/mem.o, which had an actual bug that was fixed by
Andrzej's patch, the output with my version and his is identical (failing
an lseek on /dev/mem to offset 0xfffffffffffffe00 or higher, instead of
failing for offset 0x00000000fffffe00-0x00000000ffffffff).

	Arnd

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> On Thu, 04 Feb 2016 13:40:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
>>
>>> diff --git a/include/linux/err.h b/include/linux/err.h
>>> index b7d4a9ff6342..bd4936a2c352 100644
>>> --- a/include/linux/err.h
>>> +++ b/include/linux/err.h
>>> @@ -18,9 +18,7 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> -#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
>>> -                             ? unlikely((x) <= -1) \
>>> -                             : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>> +#define IS_ERR_VALUE(x)      (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>  
>>>  static inline void * __must_check ERR_PTR(long error)
>>>  {
>>>
>>>
>>> I'm not sure if the cast to 'unsigned long long' might cause less
>>> efficient code to be generated by gcc. I would hope that it is smart
>>> enough to not actually extend shorter variables to 64 bit before
>>> doing the comparison but I have not checked yet.
>> I did a quick test with i386 on drivers/nvmem/core.o.  The patch takes
>> the text size from 9098 bytes to 9133.  That file has 11 instances of
>> IS_ERR_VALUE().  
> This seems to be because it brings back the logic to what it was before
> in case of 'int' arguments. I checked the assembly output and found mine
> to be identical to v4.4 in this case:
>
> 	 text    data     bss     dec     hex filename
> v4.4	 9942    1872    2856   14670    394e drivers/nvmem/core.o
> a.hajda  9922    1872    2856   14650    393a drivers/nvmem/core.o
> arnd	 9942    1872    2856   14670    394e drivers/nvmem/core.o

I have compared all proposed version with all compilers I have:

            text       data        bss        dec        hex    filename
gcc-4.4
old         8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
andrzej     8155       1016       2968      12139       2f6b   
.x86/drivers/nvmem/core.o
arnd        8188       1016       2968      12172       2f8c   
.x86/drivers/nvmem/core.o
rasmus      8266       1016       2968      12250       2fda   
.x86/drivers/nvmem/core.o
 ---
gcc-4.7
old         7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
andrzej     7606       3816       3248      14670       394e   
.x86/drivers/nvmem/core.o
arnd        7642       3816       3248      14706       3972   
.x86/drivers/nvmem/core.o
rasmus      7719       3816       3248      14783       39bf   
.x86/drivers/nvmem/core.o
 ---
gcc-4.8
old         7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
andrzej     7698       3888       3272      14858       3a0a   
.x86/drivers/nvmem/core.o
arnd        7735       3888       3272      14895       3a2f   
.x86/drivers/nvmem/core.o
rasmus      7812       3888       3272      14972       3a7c   
.x86/drivers/nvmem/core.o
 ---
arm-linux-gnueabi-gcc-4.7
old        12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
andrzej    12772       1680       3432      17884       45dc   
.arm/drivers/nvmem/core.o
arnd       12776       1680       3432      17888       45e0   
.arm/drivers/nvmem/core.o
rasmus     12948       1680       3432      18060       468c   
.arm/drivers/nvmem/core.o
 ---
aarch64-linux-gnu-gcc-4.8
old         5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
andrzej     5947        440         48       6435       1923   
.arm64/drivers/nvmem/core.o
arnd        5967        440         48       6455       1937   
.arm64/drivers/nvmem/core.o
rasmus      5991        440         48       6479       194f   
.arm64/drivers/nvmem/core.o
 ---

My version produces shortest code, Arnd's is the same as the old one.
On the other side Rasmus proposition seems to be the most straightforward
to me. Anyway I am not sure if the code length is the most important here.

By the way .data segment size grows almost 4 times between gcc 4.4 and
4.8 :)
Also numbers for arm64 looks interesting.

Just for the record below all proposed implementations:
#define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
#define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
                                ? unlikely((x) <= -1) \
                                : unlikely((x) >= (typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
(unsigned long long)(typeof(x))-MAX_ERRNO))
#define IS_ERR_VALUE_rasmus(x) ({\
        typeof(x) _x = (x);\
        unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
})

>
> Andrzej's version is a little shorter on ARM because in case of signed numbers
> it only checks for negative values, rather than checking for values in the
> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> in this case, and my version restores it.

As I looked at the usage of the macro in the kernel I have not found any
code
which could benefit from the original behavior, except some buggy code in
staging which have already pending fix[1].
But maybe it would be better to use IS_ERR_VALUE to always check if err
is in
range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
signed
types.

[1]: http://permalink.gmane.org/gmane.comp.file-systems.lustre.devel/4164

Regards
Andrzej

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> > On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:

> My version produces shortest code, Arnd's is the same as the old one.
> On the other side Rasmus proposition seems to be the most straightforward
> to me. Anyway I am not sure if the code length is the most important here.
> 
> By the way .data segment size grows almost 4 times between gcc 4.4 and
> 4.8 :)
> Also numbers for arm64 looks interesting.
> 
> Just for the record below all proposed implementations:
> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>                                 ? unlikely((x) <= -1) \
>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
> (unsigned long long)(typeof(x))-MAX_ERRNO))
> #define IS_ERR_VALUE_rasmus(x) ({\
>         typeof(x) _x = (x);\
>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
> })
> 
> >
> > Andrzej's version is a little shorter on ARM because in case of signed numbers
> > it only checks for negative values, rather than checking for values in the
> > [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> > in this case, and my version restores it.
> 
> As I looked at the usage of the macro in the kernel I have not found any
> code
> which could benefit from the original behavior, except some buggy code in
> staging which have already pending fix[1].
> But maybe it would be better to use IS_ERR_VALUE to always check if err
> is in
> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
> signed types.

If we do that, should we also make it illegal to use an invalid type
for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
char' that are still broken.

diff --git a/include/linux/err.h b/include/linux/err.h
index 039b80bb56ae..6700ad40f73f 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,7 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
+#define IS_ERR_VALUE(x) ({typeof(x) max_errno = -MAX_ERRNO; compiletime_assert(max_errno > 0, "IS_ERR_VALUE takes an unsigned argument"); (x >= max_errno); })
 
 static inline void * __must_check ERR_PTR(long error)
 {


Unfortunately, we get a significant number of errors if we do that, I guess the
list is what you had before

	Arnd

/git/arm-soc/drivers/irqchip/irq-hip04.c: In function 'hip04_of_init':                                   
/git/arm-soc/drivers/irqchip/irq-hip04.c:405:203: error: call to '__compiletime_assert_405' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/irqchip/irq-hip04.o] Error 1
/git/arm-soc/drivers/irqchip/irq-gic.c: In function '__gic_init_bases':
/git/arm-soc/drivers/irqchip/irq-gic.c:1106:205: error: call to '__compiletime_assert_1106' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/irqchip/irq-gic.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/irqchip] Error 2
/git/arm-soc/kernel/pid.c: In function 'alloc_pid':
/git/arm-soc/kernel/pid.c:314:198: error: call to '__compiletime_assert_314' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/kernel/pid.c:314:198: error: call to '__compiletime_assert_314' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [kernel/pid.o] Error 1
In function 'arm_smmu_init_domain_context',
    inlined from 'arm_smmu_attach_dev' at /git/arm-soc/drivers/iommu/arm-smmu.c:1138:6:
/git/arm-soc/drivers/iommu/arm-smmu.c:877:198: error: call to '__compiletime_assert_877' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/iommu/arm-smmu.c:916:198: error: call to '__compiletime_assert_916' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/iommu/arm-smmu.c: In function 'arm_smmu_attach_dev':
/git/arm-soc/drivers/iommu/arm-smmu.c:1139:199: error: call to '__compiletime_assert_1139' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/iommu/arm-smmu.o] Error 1
/git/arm-soc/drivers/dma/sun4i-dma.c: In function 'generate_ndma_promise':
/git/arm-soc/drivers/dma/sun4i-dma.c:464:198: error: call to '__compiletime_assert_464' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/dma/sun4i-dma.c: In function 'generate_ddma_promise':
/git/arm-soc/drivers/dma/sun4i-dma.c:521:198: error: call to '__compiletime_assert_521' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/dma/sun4i-dma.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/iommu] Error 2
In function 'pl011_probe_dt_alias',
    inlined from 'pl011_setup_port' at /git/arm-soc/drivers/tty/serial/amba-pl011.c:2414:8:
/git/arm-soc/drivers/tty/serial/amba-pl011.c:2360:199: error: call to '__compiletime_assert_2360' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/tty/serial/amba-pl011.o] Error 1
/git/arm-soc/drivers/tty/serial/clps711x.c: In function 'uart_clps711x_probe':
/git/arm-soc/drivers/tty/serial/clps711x.c:475:204: error: call to '__compiletime_assert_475' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/tty/serial/clps711x.o] Error 1
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:252: error: data definition has no type or storage class [-Werror]
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:252: error: type defaults to 'int' in declaration of 'device_initcall' [-Werror=implicit-int]
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:1: error: parameter names (without types) in function declaration [-Werror]
 builtin_platform_driver(ingenic_uart_platform_driver);
 ^
/git/arm-soc/drivers/tty/serial/8250/8250_ingenic.c:337:122: error: 'ingenic_uart_platform_driver_init' defined but not used [-Werror=unused-function]
cc1: all warnings being treated as errors
make[5]: *** [drivers/tty/serial/8250/8250_ingenic.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/tty/serial/8250] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/dma] Error 2
/git/arm-soc/drivers/mfd/twl4030-irq.c: In function 'twl4030_init_irq':
/git/arm-soc/drivers/mfd/twl4030-irq.c:699:203: error: call to '__compiletime_assert_699' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/mfd/twl4030-irq.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/tty/serial] Error 2
/git/arm-soc/fs/afs/write.c: In function 'afs_file_write':
/git/arm-soc/fs/afs/write.c:646:201: error: call to '__compiletime_assert_646' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [fs/afs/write.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [fs/afs] Error 2
In function 'da8xx_fb_config_clk_divider',
    inlined from 'da8xx_fb_calc_config_clk_divider' at /git/arm-soc/drivers/video/fbdev/da8xx-fb.c:769:9:
/git/arm-soc/drivers/video/fbdev/da8xx-fb.c:717:199: error: call to '__compiletime_assert_717' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
In function 'lcd_init',
    inlined from 'da8xxfb_set_par' at /git/arm-soc/drivers/video/fbdev/da8xx-fb.c:1284:6:
/git/arm-soc/drivers/video/fbdev/da8xx-fb.c:788:198: error: call to '__compiletime_assert_788' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/video/fbdev/da8xx-fb.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/tty] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/video/fbdev] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/video] Error 2
/git/arm-soc/drivers/cpufreq/omap-cpufreq.c: In function 'omap_target':
/git/arm-soc/drivers/cpufreq/omap-cpufreq.c:54:197: error: call to '__compiletime_assert_54' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/cpufreq/omap-cpufreq.o] Error 1
In function 'mmc_select_hs200',
    inlined from 'mmc_select_timing' at /git/arm-soc/drivers/mmc/core/mmc.c:1310:7,
    inlined from 'mmc_init_card' at /git/arm-soc/drivers/mmc/core/mmc.c:1565:6:
/git/arm-soc/drivers/mmc/core/mmc.c:1271:200: error: call to '__compiletime_assert_1271' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/core/mmc.c: In function 'mmc_init_card':
/git/arm-soc/drivers/mmc/core/mmc.c:1580:201: error: call to '__compiletime_assert_1580' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/core/mmc.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/cpufreq] Error 2
/git/arm-soc/drivers/crypto/caam/ctrl.c: In function 'caam_get_era':
/git/arm-soc/drivers/crypto/caam/ctrl.c:405:201: error: call to '__compiletime_assert_405' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/crypto/caam/ctrl.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/mmc/core] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/crypto/caam] Error 2
In file included from /git/arm-soc/include/linux/clk.h:15:0,
                 from /git/arm-soc/drivers/crypto/ux500/cryp/cryp_core.c:12:
/git/arm-soc/include/linux/err.h: In function 'IS_ERR':
/git/arm-soc/include/linux/err.h:35:215: error: call to '__compiletime_assert_35' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[5]: *** [drivers/crypto/ux500/cryp/cryp_core.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/crypto/ux500/cryp] Error 2
/git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_do_get_cd':
/git/arm-soc/drivers/mmc/host/sdhci.c:1628:204: error: call to '__compiletime_assert_1628' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/sdhci.c: In function 'sdhci_add_host':
/git/arm-soc/drivers/mmc/host/sdhci.c:3117:222: error: call to '__compiletime_assert_3117' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/sdhci.o] Error 1
In file included from /git/arm-soc/include/linux/clk.h:15:0,
                 from /git/arm-soc/drivers/crypto/ux500/hash/hash_core.c:16:
/git/arm-soc/include/linux/err.h: In function 'IS_ERR':
/git/arm-soc/include/linux/err.h:35:215: error: call to '__compiletime_assert_35' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[5]: *** [drivers/crypto/ux500/hash/hash_core.o] Error 1
make[5]: Target `__build' not remade because of errors.
make[4]: *** [drivers/crypto/ux500/hash] Error 2
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/crypto/ux500] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/crypto] Error 2
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x01.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x01.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x02.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x02.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x04.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x04.o] Error 1
In file included from /git/arm-soc/drivers/gpu/host1x/hw/host1x05.c:27:0:
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c: In function '_host1x_intr_init_host_sync':
/git/arm-soc/drivers/gpu/host1x/hw/intr_hw.c:88:197: error: call to '__compiletime_assert_88' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/gpu/host1x/hw/host1x05.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/gpu/host1x] Error 2
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_get_ro':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:1434:204: error: call to '__compiletime_assert_1434' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_get_cd':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:1457:209: error: call to '__compiletime_assert_1457' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/mmc/host/dw_mmc.c: In function 'dw_mci_enable_cd':
/git/arm-soc/drivers/mmc/host/dw_mmc.c:2930:223: error: call to '__compiletime_assert_2930' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/dw_mmc.o] Error 1
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/mfd] Error 2
/git/arm-soc/fs/gfs2/dir.c: In function 'get_first_leaf':
/git/arm-soc/fs/gfs2/dir.c:801:201: error: call to '__compiletime_assert_801' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/fs/gfs2/dir.c: In function 'dir_split_leaf':
/git/arm-soc/fs/gfs2/dir.c:1017:201: error: call to '__compiletime_assert_1017' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [fs/gfs2/dir.o] Error 1
In function 'sdhci_esdhc_imx_probe_dt',
    inlined from 'sdhci_esdhc_imx_probe' at /git/arm-soc/drivers/mmc/host/sdhci-esdhc-imx.c:1213:7:
/git/arm-soc/drivers/mmc/host/sdhci-esdhc-imx.c:1014:223: error: call to '__compiletime_assert_1014' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[4]: *** [drivers/mmc/host/sdhci-esdhc-imx.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [drivers/mmc/host] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/mmc] Error 2
In function 'highbank_set_em_messages',
    inlined from 'ahci_highbank_probe' at /git/arm-soc/drivers/ata/sata_highbank.c:537:2:
/git/arm-soc/drivers/ata/sata_highbank.c:200:199: error: call to '__compiletime_assert_200' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/ata/sata_highbank.o] Error 1
make[4]: Target `__build' not remade because of errors.
make[3]: *** [fs/gfs2] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [kernel] Error 2
make[3]: Target `__build' not remade because of errors.
make[2]: *** [drivers/ata] Error 2
/git/arm-soc/drivers/nvmem/core.c: In function 'bin_attr_nvmem_write':
/git/arm-soc/drivers/nvmem/core.c:105:197: error: call to '__compiletime_assert_105' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'bin_attr_nvmem_read':
/git/arm-soc/drivers/nvmem/core.c:80:196: error: call to '__compiletime_assert_80' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function '__nvmem_cell_read':
/git/arm-soc/drivers/nvmem/core.c:824:197: error: call to '__compiletime_assert_824' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_cell_read':
/git/arm-soc/drivers/nvmem/core.c:972:197: error: call to '__compiletime_assert_972' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c:976:197: error: call to '__compiletime_assert_976' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_read':
/git/arm-soc/drivers/nvmem/core.c:1031:198: error: call to '__compiletime_assert_1031' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_write':
/git/arm-soc/drivers/nvmem/core.c:1059:198: error: call to '__compiletime_assert_1059' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_cell_write':
/git/arm-soc/drivers/nvmem/core.c:944:197: error: call to '__compiletime_assert_944' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_device_cell_write':
/git/arm-soc/drivers/nvmem/core.c:1002:198: error: call to '__compiletime_assert_1002' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
In function 'nvmem_add_cells',
    inlined from 'nvmem_register' at /git/arm-soc/drivers/nvmem/core.c:365:3:
/git/arm-soc/drivers/nvmem/core.c:277:200: error: call to '__compiletime_assert_277' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
/git/arm-soc/drivers/nvmem/core.c: In function 'nvmem_cell_read':
/git/arm-soc/drivers/nvmem/core.c:859:197: error: call to '__compiletime_assert_859' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
make[3]: *** [drivers/nvmem/core.o] Error 1
make[3]: Target `__build' not remade because of errors.

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Al Viro]

On Mon, Feb 08, 2016 at 01:01:32PM +0100, Arnd Bergmann wrote:

> /git/arm-soc/fs/afs/write.c: In function 'afs_file_write':
> /git/arm-soc/fs/afs/write.c:646:201: error: call to '__compiletime_assert_646' declared with attribute error: IS_ERR_VALUE takes an unsigned argument
> make[4]: *** [fs/afs/write.o] Error 1
> make[4]: Target `__build' not remade because of errors.
> make[3]: *** [fs/afs] Error 2

... and taking a look at that code, we see this:
        if (IS_ERR_VALUE(result)) {
                _leave(" = %zd", result);
                return result;
        }

        _leave(" = %zd", result);
        return result;

Further grep for that shows e.g.
        /* determine the length of the filename */
        nlen = romfs_dev_strnlen(sb, pos + ROMFH_SIZE, ROMFS_MAXFN);
        if (IS_ERR_VALUE(nlen))
                goto eio;
which is also fucked in head, seeing that nlen should simply have been
int there, with check being if (nlen < 0).  But that would've been
insufficiently future-proof, I suppose - what if somebody wishes to modify
romfs to allow 2Gb-long file names?

For sarcasm-impaired: use of IS_ERR_VALUE turns out to be a nice red flag.
Often of the "I'm too lazy to check the calling conventions, let's gamble on
IS_ERR_VALUE doing no harm and to hell with whoever will be scratching head
reading the results" variety...

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

+cc Rasmus Villemoes, I forgot to add him earlier.

On 02/08/2016 01:01 PM, Arnd Bergmann wrote:
> On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
>> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
>>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>> My version produces shortest code, Arnd's is the same as the old one.
>> On the other side Rasmus proposition seems to be the most straightforward
>> to me. Anyway I am not sure if the code length is the most important here.
>>
>> By the way .data segment size grows almost 4 times between gcc 4.4 and
>> 4.8 :)
>> Also numbers for arm64 looks interesting.
>>
>> Just for the record below all proposed implementations:
>> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>>                                 ? unlikely((x) <= -1) \
>>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
>> (unsigned long long)(typeof(x))-MAX_ERRNO))
>> #define IS_ERR_VALUE_rasmus(x) ({\
>>         typeof(x) _x = (x);\
>>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
>> })
>>
>>> Andrzej's version is a little shorter on ARM because in case of signed numbers
>>> it only checks for negative values, rather than checking for values in the
>>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
>>> in this case, and my version restores it.
>> As I looked at the usage of the macro in the kernel I have not found any
>> code
>> which could benefit from the original behavior, except some buggy code in
>> staging which have already pending fix[1].
>> But maybe it would be better to use IS_ERR_VALUE to always check if err
>> is in
>> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
>> signed types.
> If we do that, should we also make it illegal to use an invalid type
> for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
> char' that are still broken.
I meant rather to make such 'policy' for future code by adding some
comment to the macro. Optionally adding compile time warning
to encourage developers to change current usage, however I am
not sure if it is not too harsh.
This way it could be also good to use your version of the macro.
It could be also good to add compiletime_assert to prevent char types
as suggested by Rasmus.

Finally it could look like:
/*
 * Use IS_ERR_VALUE only on unsigned types of at least two bytes size.
 * For signed types use '< 0' comparison.
 */
#define IS_ERR_VALUE(x)\
({\
        compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle
byte-size types");\
        compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE
should be called on unsigned types only, use '< 0' instead");\
        (unlikely((unsigned long long)(x) >= (unsigned long
long)(typeof(x))-MAX_ERRNO));\
})

Minor issue: there are no compile-time warning macros in kernel.
Helper provided by gcc (warning attribute) is not so nice:
optimizations removes the warning itself, preventing optimization
influences final code.
I have put my proposition of workaround below:

#define compiletime_assert_warning(cond, msg) \
({ \
        __maybe_unused void const *p = (cond) ? 0 : 1; \
})

On older compilers it issues just warning:

drivers/nvmem/core.c:1059: warning: initialization makes pointer from
integer without a cast

Since gcc 4.8 it is more verbose:

drivers/nvmem/core.c: In function ‘nvmem_device_write’:
include/linux/err.h:33:33: warning: initialization makes pointer from
integer without a cast [enabled by default]
  __maybe_unused void const *p = (cond) ? 0 : 1; \
                                 ^
include/linux/err.h:41:2: note: in expansion of macro
‘compiletime_assert_warning’
  compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE should
be called on unsigned types only, use '< 0' instead");\
  ^
drivers/nvmem/core.c:1059:6: note: in expansion of macro ‘IS_ERR_VALUE’
  if (IS_ERR_VALUE(rc))


Regards
Andrzej

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Tuesday 09 February 2016 09:42:26 Andrzej Hajda wrote:
> +cc Rasmus Villemoes, I forgot to add him earlier.
> 
> On 02/08/2016 01:01 PM, Arnd Bergmann wrote:
> > On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
> >> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
> >>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
> >> My version produces shortest code, Arnd's is the same as the old one.
> >> On the other side Rasmus proposition seems to be the most straightforward
> >> to me. Anyway I am not sure if the code length is the most important here.
> >>
> >> By the way .data segment size grows almost 4 times between gcc 4.4 and
> >> 4.8 :)
> >> Also numbers for arm64 looks interesting.
> >>
> >> Just for the record below all proposed implementations:
> >> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> >> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
> >>                                 ? unlikely((x) <= -1) \
> >>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
> >> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
> >> (unsigned long long)(typeof(x))-MAX_ERRNO))
> >> #define IS_ERR_VALUE_rasmus(x) ({\
> >>         typeof(x) _x = (x);\
> >>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
> >> })
> >>
> >>> Andrzej's version is a little shorter on ARM because in case of signed numbers
> >>> it only checks for negative values, rather than checking for values in the
> >>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
> >>> in this case, and my version restores it.
> >> As I looked at the usage of the macro in the kernel I have not found any
> >> code
> >> which could benefit from the original behavior, except some buggy code in
> >> staging which have already pending fix[1].
> >> But maybe it would be better to use IS_ERR_VALUE to always check if err
> >> is in
> >> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
> >> signed types.
> > If we do that, should we also make it illegal to use an invalid type
> > for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
> > char' that are still broken.
> I meant rather to make such 'policy' for future code by adding some
> comment to the macro. Optionally adding compile time warning
> to encourage developers to change current usage, however I am
> not sure if it is not too harsh.
> This way it could be also good to use your version of the macro.
> It could be also good to add compiletime_assert to prevent char types
> as suggested by Rasmus.
> 
> Finally it could look like:
> /*
>  * Use IS_ERR_VALUE only on unsigned types of at least two bytes size.
>  * For signed types use '< 0' comparison.
>  */
> #define IS_ERR_VALUE(x)\
> ({\
>         compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle
> byte-size types");\
>         compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE
> should be called on unsigned types only, use '< 0' instead");\
>         (unlikely((unsigned long long)(x) >= (unsigned long
> long)(typeof(x))-MAX_ERRNO));\
> })
> 

I think the easiest way to express this would be to ensure that the argument
is 'unsigned long', like:

#define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
       unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))


I tried to figure out where that would take as, as we then have to change
all users of IS_ERR_VALUE() that don't use an 'unsigned long' argument, not
just the ones that use one of the forbidden types (long long, unsigned long long,
unsigned int, unsigned short, char, unsigned char, signed char).

The patch below modifies all drivers that see newly introduced warnings with
the version above. Most of them pass an 'int' into IS_ERR_VALUE(), which currently
works as expected.

There are probably a couple of other architecture-specific files using IS_ERR_VALUE()
that I did not catch with ARM allmodocnfig and randconfig builds.

	Arnd

 drivers/ata/sata_highbank.c                      |  2 +-
 drivers/char/mem.c                               |  2 +-
 drivers/cpufreq/omap-cpufreq.c                   |  2 +-
 drivers/crypto/caam/ctrl.c                       |  2 +-
 drivers/dma/sun4i-dma.c                          | 16 ++++++++--------
 drivers/gpu/drm/sti/sti_vtg.c                    |  4 ++--
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c           |  2 +-
 drivers/gpu/host1x/hw/intr_hw.c                  |  2 +-
 drivers/iommu/arm-smmu.c                         |  8 ++++----
 drivers/irqchip/irq-clps711x.c                   |  2 +-
 drivers/irqchip/irq-gic.c                        |  2 +-
 drivers/irqchip/irq-hip04.c                      |  2 +-
 drivers/media/i2c/adp1653.c                      | 10 +++++-----
 drivers/media/platform/s5p-tv/mixer_drv.c        |  2 +-
 drivers/media/platform/soc_camera/atmel-isi.c    |  2 +-
 drivers/mfd/twl4030-irq.c                        |  2 +-
 drivers/mmc/core/mmc.c                           |  4 ++--
 drivers/mmc/host/dw_mmc.c                        |  6 +++---
 drivers/mmc/host/sdhci-esdhc-imx.c               |  2 +-
 drivers/mmc/host/sdhci.c                         |  4 ++--
 drivers/net/ethernet/freescale/fman/fman.c       |  2 +-
 drivers/net/ethernet/freescale/fman/fman_muram.c |  4 ++--
 drivers/net/ethernet/freescale/fman/fman_muram.h |  4 ++--
 drivers/net/wireless/ti/wlcore/spi.c             |  4 ++--
 drivers/nvmem/core.c                             | 22 +++++++++++-----------
 drivers/staging/lustre/lnet/lnet/acceptor.c      |  4 ++--
 drivers/staging/lustre/lnet/lnet/router.c        |  4 ++--
 drivers/staging/lustre/lustre/llite/statahead.c  |  4 ++--
 drivers/staging/lustre/lustre/mdc/mdc_request.c  |  4 ++--
 drivers/staging/lustre/lustre/mgc/mgc_request.c  |  4 ++--
 drivers/staging/lustre/lustre/obdclass/llog.c    |  4 ++--
 drivers/staging/lustre/lustre/ptlrpc/pinger.c    |  4 ++--
 drivers/staging/lustre/lustre/ptlrpc/service.c   | 10 +++++-----
 drivers/tty/serial/amba-pl011.c                  |  2 +-
 drivers/tty/serial/clps711x.c                    |  4 ++--
 drivers/tty/serial/digicolor-usart.c             |  2 +-
 drivers/video/fbdev/da8xx-fb.c                   |  4 ++--
 drivers/video/fbdev/exynos/exynos_mipi_dsi.c     |  2 +-
 fs/afs/write.c                                   |  4 ----
 fs/gfs2/dir.c                                    |  4 ++--
 include/linux/err.h                              |  3 ++-
 kernel/pid.c                                     |  2 +-
 net/9p/client.c                                  |  4 ++--
 net/ipv4/netfilter/arp_tables.c                  |  2 +-
 net/ipv4/netfilter/ip_tables.c                   |  2 +-
 net/ipv6/netfilter/ip6_tables.c                  |  2 +-
 sound/soc/qcom/lpass-platform.c                  |  2 +-
 47 files changed, 94 insertions(+), 97 deletions(-)

diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 8638d575b2b9..aafb8cc03523 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -197,7 +197,7 @@ static void highbank_set_em_messages(struct device *dev,
 
 	for (i = 0; i < SGPIO_PINS; i++) {
 		err = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
-		if (IS_ERR_VALUE(err))
+		if (err < 0)
 			return;
 
 		pdata->sgpio_gpio[i] = err;
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 4f6f94c43412..3ffd3e32051b 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 (offset > (unsigned long long)-MAX_ERRNO) {
 			ret = -EOVERFLOW;
 			break;
 		}
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index e3866e0d5bf8..c182c8c84b8d 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -51,7 +51,7 @@ static int omap_target(struct cpufreq_policy *policy, unsigned int index)
 
 	freq = new_freq * 1000;
 	ret = clk_round_rate(policy->clk, freq);
-	if (IS_ERR_VALUE(ret)) {
+	if (ret < 0) {
 		dev_warn(mpu_dev,
 			 "CPUfreq: Cannot find matching frequency for %lu\n",
 			 freq);
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 44d30b45f3cc..5ad5f3009ae0 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -402,7 +402,7 @@ int caam_get_era(void)
 	ret = of_property_read_u32(caam_node, "fsl,sec-era", &prop);
 	of_node_put(caam_node);
 
-	return IS_ERR_VALUE(ret) ? -ENOTSUPP : prop;
+	return ret ? -ENOTSUPP : prop;
 }
 EXPORT_SYMBOL(caam_get_era);
 
diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
index 1661d518224a..4dc7a686af1c 100644
--- a/drivers/dma/sun4i-dma.c
+++ b/drivers/dma/sun4i-dma.c
@@ -461,25 +461,25 @@ generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
 
 	/* Source burst */
 	ret = convert_burst(sconfig->src_maxburst);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_SRC_BURST_LENGTH(ret);
 
 	/* Destination burst */
 	ret = convert_burst(sconfig->dst_maxburst);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_DST_BURST_LENGTH(ret);
 
 	/* Source bus width */
 	ret = convert_buswidth(sconfig->src_addr_width);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_SRC_DATA_WIDTH(ret);
 
 	/* Destination bus width */
 	ret = convert_buswidth(sconfig->dst_addr_width);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_DST_DATA_WIDTH(ret);
 
@@ -518,25 +518,25 @@ generate_ddma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
 
 	/* Source burst */
 	ret = convert_burst(sconfig->src_maxburst);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_SRC_BURST_LENGTH(ret);
 
 	/* Destination burst */
 	ret = convert_burst(sconfig->dst_maxburst);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_DST_BURST_LENGTH(ret);
 
 	/* Source bus width */
 	ret = convert_buswidth(sconfig->src_addr_width);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_SRC_DATA_WIDTH(ret);
 
 	/* Destination bus width */
 	ret = convert_buswidth(sconfig->dst_addr_width);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto fail;
 	promise->cfg |= SUN4I_DMA_CFG_DST_DATA_WIDTH(ret);
 
diff --git a/drivers/gpu/drm/sti/sti_vtg.c b/drivers/gpu/drm/sti/sti_vtg.c
index d56630c60039..71c03ea20e32 100644
--- a/drivers/gpu/drm/sti/sti_vtg.c
+++ b/drivers/gpu/drm/sti/sti_vtg.c
@@ -355,7 +355,7 @@ static int vtg_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	} else {
 		vtg->irq = platform_get_irq(pdev, 0);
-		if (IS_ERR_VALUE(vtg->irq)) {
+		if (vtg->irq < 0) {
 			DRM_ERROR("Failed to get VTG interrupt\n");
 			return vtg->irq;
 		}
@@ -365,7 +365,7 @@ static int vtg_probe(struct platform_device *pdev)
 		ret = devm_request_threaded_irq(dev, vtg->irq, vtg_irq,
 				vtg_irq_thread, IRQF_ONESHOT,
 				dev_name(dev), vtg);
-		if (IS_ERR_VALUE(ret)) {
+		if (ret < 0) {
 			DRM_ERROR("Failed to register VTG interrupt\n");
 			return ret;
 		}
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 5052a8af7ecb..7e4f56254af7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -359,7 +359,7 @@ static int tfp410_probe(struct platform_device *pdev)
 
 	tfp410_mod->gpio = of_get_named_gpio_flags(node, "powerdn-gpio",
 			0, NULL);
-	if (IS_ERR_VALUE(tfp410_mod->gpio)) {
+	if (tfp410_mod->gpio < 0) {
 		dev_warn(&pdev->dev, "No power down GPIO\n");
 	} else {
 		ret = gpio_request(tfp410_mod->gpio, "DVI_PDn");
diff --git a/drivers/gpu/host1x/hw/intr_hw.c b/drivers/gpu/host1x/hw/intr_hw.c
index 498b37e39058..e1e31e9e67cd 100644
--- a/drivers/gpu/host1x/hw/intr_hw.c
+++ b/drivers/gpu/host1x/hw/intr_hw.c
@@ -85,7 +85,7 @@ static int _host1x_intr_init_host_sync(struct host1x *host, u32 cpm,
 	err = devm_request_irq(host->dev, host->intr_syncpt_irq,
 			       syncpt_thresh_isr, IRQF_SHARED,
 			       "host1x_syncpt", host);
-	if (IS_ERR_VALUE(err)) {
+	if (err < 0) {
 		WARN_ON(1);
 		return err;
 	}
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8a3236..f6750772f6d8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -874,7 +874,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 
 	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
 				      smmu->num_context_banks);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		goto out_unlock;
 
 	cfg->cbndx = ret;
@@ -913,7 +913,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
 	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
 			  "arm-smmu-context-fault", domain);
-	if (IS_ERR_VALUE(ret)) {
+	if (ret < 0) {
 		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
 			cfg->irptndx, irq);
 		cfg->irptndx = INVALID_IRPTNDX;
@@ -1016,7 +1016,7 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
 						  smmu->num_mapping_groups);
-		if (IS_ERR_VALUE(idx)) {
+		if (idx < 0) {
 			dev_err(smmu->dev, "failed to allocate free SMR\n");
 			goto err_free_smrs;
 		}
@@ -1136,7 +1136,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	/* Ensure that the domain is finalised */
 	ret = arm_smmu_init_domain_context(domain, smmu);
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		return ret;
 
 	/*
diff --git a/drivers/irqchip/irq-clps711x.c b/drivers/irqchip/irq-clps711x.c
index eb5eb0cd414d..2223b3f15d68 100644
--- a/drivers/irqchip/irq-clps711x.c
+++ b/drivers/irqchip/irq-clps711x.c
@@ -182,7 +182,7 @@ static int __init _clps711x_intc_init(struct device_node *np,
 	writel_relaxed(0, clps711x_intc->intmr[2]);
 
 	err = irq_alloc_descs(-1, 0, ARRAY_SIZE(clps711x_irqs), numa_node_id());
-	if (IS_ERR_VALUE(err))
+	if (err < 0)
 		goto out_iounmap;
 
 	clps711x_intc->ops.map = clps711x_intc_irq_map;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 911758c056c1..355d0779bec0 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1103,7 +1103,7 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 
 		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
 					   numa_node_id());
-		if (IS_ERR_VALUE(irq_base)) {
+		if (irq_base < 0) {
 			WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
 			     irq_start);
 			irq_base = irq_start;
diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
index 9688d2e2a636..9e25d8ce08e5 100644
--- a/drivers/irqchip/irq-hip04.c
+++ b/drivers/irqchip/irq-hip04.c
@@ -402,7 +402,7 @@ hip04_of_init(struct device_node *node, struct device_node *parent)
 	nr_irqs -= hwirq_base; /* calculate # of irqs to allocate */
 
 	irq_base = irq_alloc_descs(-1, hwirq_base, nr_irqs, numa_node_id());
-	if (IS_ERR_VALUE(irq_base)) {
+	if (irq_base < 0) {
 		pr_err("failed to allocate IRQ numbers\n");
 		return -EINVAL;
 	}
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 7e9cbf757e95..efa1c11f8f0b 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -95,7 +95,7 @@ static int adp1653_get_fault(struct adp1653_flash *flash)
 	int rval;
 
 	fault = i2c_smbus_read_byte_data(client, ADP1653_REG_FAULT);
-	if (IS_ERR_VALUE(fault))
+	if (fault < 0)
 		return fault;
 
 	flash->fault |= fault;
@@ -105,13 +105,13 @@ static int adp1653_get_fault(struct adp1653_flash *flash)
 
 	/* Clear faults. */
 	rval = i2c_smbus_write_byte_data(client, ADP1653_REG_OUT_SEL, 0);
-	if (IS_ERR_VALUE(rval))
+	if (rval < 0)
 		return rval;
 
 	flash->led_mode->val = V4L2_FLASH_LED_MODE_NONE;
 
 	rval = adp1653_update_hw(flash);
-	if (IS_ERR_VALUE(rval))
+	if (rval)
 		return rval;
 
 	return flash->fault;
@@ -158,7 +158,7 @@ static int adp1653_get_ctrl(struct v4l2_ctrl *ctrl)
 	int rval;
 
 	rval = adp1653_get_fault(flash);
-	if (IS_ERR_VALUE(rval))
+	if (rval)
 		return rval;
 
 	ctrl->cur.val = 0;
@@ -184,7 +184,7 @@ static int adp1653_set_ctrl(struct v4l2_ctrl *ctrl)
 	int rval;
 
 	rval = adp1653_get_fault(flash);
-	if (IS_ERR_VALUE(rval))
+	if (rval)
 		return rval;
 	if ((rval & (ADP1653_REG_FAULT_FLT_SCP |
 		     ADP1653_REG_FAULT_FLT_OT |
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c b/drivers/media/platform/s5p-tv/mixer_drv.c
index 5ef67774971d..8a5d19469ddc 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -146,7 +146,7 @@ int mxr_power_get(struct mxr_device *mdev)
 
 	/* returning 1 means that power is already enabled,
 	 * so zero success be returned */
-	if (IS_ERR_VALUE(ret))
+	if (ret < 0)
 		return ret;
 	return 0;
 }
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 1af779ee3c74..fbc8f633521e 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -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;
 	}
diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index 40e51b0baa46..b46c0cfc27d9 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -696,7 +696,7 @@ int twl4030_init_irq(struct device *dev, int irq_num)
 	nr_irqs = TWL4030_PWR_NR_IRQS + TWL4030_CORE_NR_IRQS;
 
 	irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
-	if (IS_ERR_VALUE(irq_base)) {
+	if (irq_base < 0) {
 		dev_err(dev, "Fail to allocate IRQ descs\n");
 		return irq_base;
 	}
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4dbe3df8024b..6304883f35a2 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1268,7 +1268,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 	 * switch to HS200 mode if bus width is set successfully.
 	 */
 	err = mmc_select_bus_width(card);
-	if (!IS_ERR_VALUE(err)) {
+	if (!err) {
 		val = EXT_CSD_TIMING_HS200 |
 		      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -1577,7 +1577,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	} else if (mmc_card_hs(card)) {
 		/* Select the desired bus width optionally */
 		err = mmc_select_bus_width(card);
-		if (!IS_ERR_VALUE(err)) {
+		if (!err) {
 			err = mmc_select_hs_ddr(card);
 			if (err)
 				goto free_card;
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 242f9a0769bd..a4cbbfe1eabe 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1431,7 +1431,7 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
 	int gpio_ro = mmc_gpio_get_ro(mmc);
 
 	/* Use platform get_ro function, else try on board write protect */
-	if (!IS_ERR_VALUE(gpio_ro))
+	if (gpio_ro >= 0)
 		read_only = gpio_ro;
 	else
 		read_only =
@@ -1454,7 +1454,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	if ((mmc->caps & MMC_CAP_NEEDS_POLL) ||
 	    (mmc->caps & MMC_CAP_NONREMOVABLE))
 		present = 1;
-	else if (!IS_ERR_VALUE(gpio_cd))
+	else if (gpio_cd >= 0)
 		present = gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
@@ -2927,7 +2927,7 @@ static void dw_mci_enable_cd(struct dw_mci *host)
 		if (slot->mmc->caps & MMC_CAP_NEEDS_POLL)
 			return;
 
-		if (IS_ERR_VALUE(mmc_gpio_get_cd(slot->mmc)))
+		if (mmc_gpio_get_cd(slot->mmc) < 0)
 			break;
 	}
 	if (i == host->num_slots)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index f25f29253595..22e47ab06e9f 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1011,7 +1011,7 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	if (!IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
+	if (mmc_gpio_get_cd(host->mmc) >= 0)
 		host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
 
 	return 0;
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d622435d1bcc..8f1dca39a940 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1625,7 +1625,7 @@ static int sdhci_do_get_cd(struct sdhci_host *host)
 	 * Try slot gpio detect, if defined it take precedence
 	 * over build in controller functionality
 	 */
-	if (!IS_ERR_VALUE(gpio_cd))
+	if (gpio_cd >= 0)
 		return !!gpio_cd;
 
 	/* If polling, assume that the card is always present. */
@@ -3114,7 +3114,7 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
 	    !(mmc->caps & MMC_CAP_NONREMOVABLE) &&
-	    IS_ERR_VALUE(mmc_gpio_get_cd(host->mmc)))
+	    mmc_gpio_get_cd(host->mmc) < 0)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	/* If there are external regulators, get them */
diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 623aa1c8ebc6..f1a5f2a805ec 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -614,7 +614,7 @@ struct fman {
 	struct fman_cfg *cfg;
 	struct muram_info *muram;
 	/* cam section in muram */
-	int cam_offset;
+	unsigned long cam_offset;
 	size_t cam_size;
 	/* Fifo in MURAM */
 	int fifo_offset;
diff --git a/drivers/net/ethernet/freescale/fman/fman_muram.c b/drivers/net/ethernet/freescale/fman/fman_muram.c
index 4eb0e9ac7182..47394c45b6e8 100644
--- a/drivers/net/ethernet/freescale/fman/fman_muram.c
+++ b/drivers/net/ethernet/freescale/fman/fman_muram.c
@@ -129,7 +129,7 @@ unsigned long fman_muram_offset_to_vbase(struct muram_info *muram,
  *
  * Return: address of the allocated memory; NULL otherwise.
  */
-int fman_muram_alloc(struct muram_info *muram, size_t size)
+unsigned long fman_muram_alloc(struct muram_info *muram, size_t size)
 {
 	unsigned long vaddr;
 
@@ -150,7 +150,7 @@ int fman_muram_alloc(struct muram_info *muram, size_t size)
  *
  * Free an allocated memory from FM-MURAM partition.
  */
-void fman_muram_free_mem(struct muram_info *muram, u32 offset, size_t size)
+void fman_muram_free_mem(struct muram_info *muram, unsigned long offset, size_t size)
 {
 	unsigned long addr = fman_muram_offset_to_vbase(muram, offset);
 
diff --git a/drivers/net/ethernet/freescale/fman/fman_muram.h b/drivers/net/ethernet/freescale/fman/fman_muram.h
index dbf0af9e5bb5..889649ad8931 100644
--- a/drivers/net/ethernet/freescale/fman/fman_muram.h
+++ b/drivers/net/ethernet/freescale/fman/fman_muram.h
@@ -44,8 +44,8 @@ struct muram_info *fman_muram_init(phys_addr_t base, size_t size);
 unsigned long fman_muram_offset_to_vbase(struct muram_info *muram,
 					 unsigned long offset);
 
-int fman_muram_alloc(struct muram_info *muram, size_t size);
+unsigned long fman_muram_alloc(struct muram_info *muram, size_t size);
 
-void fman_muram_free_mem(struct muram_info *muram, u32 offset, size_t size);
+void fman_muram_free_mem(struct muram_info *muram, unsigned long offset, size_t size);
 
 #endif /* __FM_MURAM_EXT */
diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
index 020ac1a4b408..cea9443c22a6 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -382,7 +382,7 @@ static int wlcore_probe_of(struct spi_device *spi, struct wl12xx_spi_glue *glue,
 
 	ret = of_property_read_u32(dt_node, "ref-clock-frequency",
 				   &pdev_data->ref_clock_freq);
-	if (IS_ERR_VALUE(ret)) {
+	if (ret) {
 		dev_err(glue->dev,
 			"can't get reference clock frequency (%d)\n", ret);
 		return ret;
@@ -425,7 +425,7 @@ static int wl1271_probe(struct spi_device *spi)
 	}
 
 	ret = wlcore_probe_of(spi, glue, &pdev_data);
-	if (IS_ERR_VALUE(ret)) {
+	if (ret) {
 		dev_err(glue->dev,
 			"can't get device tree parameters (%d)\n", ret);
 		return ret;
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9d11d9837312..69f2c6391ec5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -80,7 +80,7 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 
 	rc = regmap_raw_read(nvmem->regmap, pos, buf, count);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return count;
@@ -108,7 +108,7 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 	rc = regmap_raw_write(nvmem->regmap, pos, buf, count);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return count;
@@ -280,7 +280,7 @@ static int nvmem_add_cells(struct nvmem_device *nvmem,
 		}
 
 		rval = nvmem_cell_info_to_nvmem_cell(nvmem, &info[i], cells[i]);
-		if (IS_ERR_VALUE(rval)) {
+		if (rval) {
 			kfree(cells[i]);
 			goto err;
 		}
@@ -827,7 +827,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 
 	rc = regmap_raw_read(nvmem->regmap, cell->offset, buf, cell->bytes);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	/* shift bits in-place */
@@ -862,7 +862,7 @@ void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len)
 		return ERR_PTR(-ENOMEM);
 
 	rc = __nvmem_cell_read(nvmem, cell, buf, len);
-	if (IS_ERR_VALUE(rc)) {
+	if (rc) {
 		kfree(buf);
 		return ERR_PTR(rc);
 	}
@@ -947,7 +947,7 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
 	if (cell->bit_offset || cell->nbits)
 		kfree(buf);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return len;
@@ -975,11 +975,11 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
 		return -EINVAL;
 
 	rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	rc = __nvmem_cell_read(nvmem, &cell, buf, &len);
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return len;
@@ -1005,7 +1005,7 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem,
 		return -EINVAL;
 
 	rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return nvmem_cell_write(&cell, buf, cell.bytes);
@@ -1034,7 +1034,7 @@ int nvmem_device_read(struct nvmem_device *nvmem,
 
 	rc = regmap_raw_read(nvmem->regmap, offset, buf, bytes);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 	return bytes;
@@ -1062,7 +1062,7 @@ int nvmem_device_write(struct nvmem_device *nvmem,
 
 	rc = regmap_raw_write(nvmem->regmap, offset, buf, bytes);
 
-	if (IS_ERR_VALUE(rc))
+	if (rc)
 		return rc;
 
 
diff --git a/drivers/staging/lustre/lnet/lnet/acceptor.c b/drivers/staging/lustre/lnet/lnet/acceptor.c
index fed57d90028d..78c3a5323295 100644
--- a/drivers/staging/lustre/lnet/lnet/acceptor.c
+++ b/drivers/staging/lustre/lnet/lnet/acceptor.c
@@ -454,10 +454,10 @@ lnet_acceptor_start(void)
 	if (lnet_count_acceptor_nis() == 0)  /* not required */
 		return 0;
 
-	rc2 = PTR_ERR(kthread_run(lnet_acceptor,
+	rc2 = PTR_ERR_OR_ZERO(kthread_run(lnet_acceptor,
 				  (void *)(ulong_ptr_t)secure,
 				  "acceptor_%03ld", secure));
-	if (IS_ERR_VALUE(rc2)) {
+	if (rc2) {
 		CERROR("Can't start acceptor thread: %ld\n", rc2);
 
 		return -ESRCH;
diff --git a/drivers/staging/lustre/lnet/lnet/router.c b/drivers/staging/lustre/lnet/lnet/router.c
index f5faa414d250..2a8a684c5fb8 100644
--- a/drivers/staging/lustre/lnet/lnet/router.c
+++ b/drivers/staging/lustre/lnet/lnet/router.c
@@ -1029,9 +1029,9 @@ lnet_router_checker_start(void)
 	}
 
 	the_lnet.ln_rc_state = LNET_RC_STATE_RUNNING;
-	rc = PTR_ERR(kthread_run(lnet_router_checker,
+	rc = PTR_ERR_OR_ZERO(kthread_run(lnet_router_checker,
 				 NULL, "router_checker"));
-	if (IS_ERR_VALUE(rc)) {
+	if (rc) {
 		CERROR("Can't start router checker thread: %d\n", rc);
 		/* block until event callback signals exit */
 		down(&the_lnet.ln_rc_signal);
diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c
index 88ffd8e3abdb..e69b45f13799 100644
--- a/drivers/staging/lustre/lustre/llite/statahead.c
+++ b/drivers/staging/lustre/lustre/llite/statahead.c
@@ -1656,10 +1656,10 @@ int do_statahead_enter(struct inode *dir, struct dentry **dentryp,
 	lli->lli_sai = sai;
 
 	plli = ll_i2info(d_inode(parent));
-	rc = PTR_ERR(kthread_run(ll_statahead_thread, parent,
+	rc = PTR_ERR_OR_ZERO(kthread_run(ll_statahead_thread, parent,
 				 "ll_sa_%u", plli->lli_opendir_pid));
 	thread = &sai->sai_thread;
-	if (IS_ERR_VALUE(rc)) {
+	if (rc) {
 		CERROR("can't start ll_sa thread, rc: %d\n", rc);
 		dput(parent);
 		lli->lli_opendir_key = NULL;
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c b/drivers/staging/lustre/lustre/mdc/mdc_request.c
index 43481278096f..7b1445c6ea5f 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
@@ -1571,9 +1571,9 @@ static int mdc_ioc_changelog_send(struct obd_device *obd,
 	 * New thread because we should return to user app before
 	 * writing into our pipe
 	 */
-	rc = PTR_ERR(kthread_run(mdc_changelog_send_thread, cs,
+	rc = PTR_ERR_OR_ZERO(kthread_run(mdc_changelog_send_thread, cs,
 				 "mdc_clg_send_thread"));
-	if (!IS_ERR_VALUE(rc)) {
+	if (!rc) {
 		CDEBUG(D_CHANGELOG, "start changelog thread\n");
 		return 0;
 	}
diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c
index ab4800c20a95..815b33c8fd97 100644
--- a/drivers/staging/lustre/lustre/mgc/mgc_request.c
+++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c
@@ -734,9 +734,9 @@ static int mgc_setup(struct obd_device *obd, struct lustre_cfg *lcfg)
 		init_waitqueue_head(&rq_waitq);
 
 		/* start requeue thread */
-		rc = PTR_ERR(kthread_run(mgc_requeue_thread, NULL,
+		rc = PTR_ERR_OR_ZERO(kthread_run(mgc_requeue_thread, NULL,
 					     "ll_cfg_requeue"));
-		if (IS_ERR_VALUE(rc)) {
+		if (rc) {
 			CERROR("%s: Cannot start requeue thread (%d),no more log updates!\n",
 			       obd->obd_name, rc);
 			goto err_cleanup;
diff --git a/drivers/staging/lustre/lustre/obdclass/llog.c b/drivers/staging/lustre/lustre/obdclass/llog.c
index f956d7ed6785..91230c84ad6d 100644
--- a/drivers/staging/lustre/lustre/obdclass/llog.c
+++ b/drivers/staging/lustre/lustre/obdclass/llog.c
@@ -380,9 +380,9 @@ int llog_process_or_fork(const struct lu_env *env,
 		 * init the new one in llog_process_thread_daemonize. */
 		lpi->lpi_env = NULL;
 		init_completion(&lpi->lpi_completion);
-		rc = PTR_ERR(kthread_run(llog_process_thread_daemonize, lpi,
+		rc = PTR_ERR_OR_ZERO(kthread_run(llog_process_thread_daemonize, lpi,
 					     "llog_process_thread"));
-		if (IS_ERR_VALUE(rc)) {
+		if (rc) {
 			CERROR("%s: cannot start thread: rc = %d\n",
 			       loghandle->lgh_ctxt->loc_obd->obd_name, rc);
 			kfree(lpi);
diff --git a/drivers/staging/lustre/lustre/ptlrpc/pinger.c b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
index fb2d5236a971..5b7743b9f48f 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/pinger.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
@@ -303,9 +303,9 @@ int ptlrpc_start_pinger(void)
 
 	strcpy(pinger_thread.t_name, "ll_ping");
 
-	rc = PTR_ERR(kthread_run(ptlrpc_pinger_main, &pinger_thread,
+	rc = PTR_ERR_OR_ZERO(kthread_run(ptlrpc_pinger_main, &pinger_thread,
 				 "%s", pinger_thread.t_name));
-	if (IS_ERR_VALUE(rc)) {
+	if (rc) {
 		CERROR("cannot start thread: %d\n", rc);
 		return rc;
 	}
diff --git a/drivers/staging/lustre/lustre/ptlrpc/service.c b/drivers/staging/lustre/lustre/ptlrpc/service.c
index 8598300a61d1..8ca33f9dbf42 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/service.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/service.c
@@ -2256,17 +2256,17 @@ static int ptlrpc_start_hr_threads(void)
 		for (j = 0; j < hrp->hrp_nthrs; j++) {
 			struct	ptlrpc_hr_thread *hrt = &hrp->hrp_thrs[j];
 
-			rc = PTR_ERR(kthread_run(ptlrpc_hr_main,
+			rc = PTR_ERR_OR_ZERO(kthread_run(ptlrpc_hr_main,
 						 &hrp->hrp_thrs[j],
 						 "ptlrpc_hr%02d_%03d",
 						 hrp->hrp_cpt,
 						 hrt->hrt_id));
-			if (IS_ERR_VALUE(rc))
+			if (rc)
 				break;
 		}
 		wait_event(ptlrpc_hr.hr_waitq,
 			       atomic_read(&hrp->hrp_nstarted) == j);
-		if (!IS_ERR_VALUE(rc))
+		if (!rc)
 			continue;
 
 		CERROR("Reply handling thread %d:%d Failed on starting: rc = %d\n",
@@ -2442,8 +2442,8 @@ int ptlrpc_start_thread(struct ptlrpc_service_part *svcpt, int wait)
 	}
 
 	CDEBUG(D_RPCTRACE, "starting thread '%s'\n", thread->t_name);
-	rc = PTR_ERR(kthread_run(ptlrpc_main, thread, "%s", thread->t_name));
-	if (IS_ERR_VALUE(rc)) {
+	rc = PTR_ERR_OR_ZERO(kthread_run(ptlrpc_main, thread, "%s", thread->t_name));
+	if (rc) {
 		CERROR("cannot start thread '%s': rc %d\n",
 		       thread->t_name, rc);
 		spin_lock(&svcpt->scp_lock);
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 500232ad38f3..e328df11b253 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2357,7 +2357,7 @@ static int pl011_probe_dt_alias(int index, struct device *dev)
 		return ret;
 
 	ret = of_alias_get_id(np, "serial");
-	if (IS_ERR_VALUE(ret)) {
+	if (ret < 0) {
 		seen_dev_without_alias = true;
 		ret = index;
 	} else {
diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index b3a4e0cdddaa..fbb2b715afed 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -468,11 +468,11 @@ static int uart_clps711x_probe(struct platform_device *pdev)
 		return PTR_ERR(s->port.membase);
 
 	s->port.irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(s->port.irq))
+	if (s->port.irq >= 0)
 		return s->port.irq;
 
 	s->rx_irq = platform_get_irq(pdev, 1);
-	if (IS_ERR_VALUE(s->rx_irq))
+	if (s->rx_irq >= 0)
 		return s->rx_irq;
 
 	if (!np) {
diff --git a/drivers/tty/serial/digicolor-usart.c b/drivers/tty/serial/digicolor-usart.c
index a80cdad114f3..a576555de987 100644
--- a/drivers/tty/serial/digicolor-usart.c
+++ b/drivers/tty/serial/digicolor-usart.c
@@ -482,7 +482,7 @@ static int digicolor_uart_probe(struct platform_device *pdev)
 		return PTR_ERR(dp->port.membase);
 
 	dp->port.irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(dp->port.irq))
+	if (dp->port.irq < 0)
 		return dp->port.irq;
 
 	dp->port.iotype = UPIO_MEM;
diff --git a/drivers/video/fbdev/da8xx-fb.c b/drivers/video/fbdev/da8xx-fb.c
index 6b2a06d09f2b..ec0573ff3b8c 100644
--- a/drivers/video/fbdev/da8xx-fb.c
+++ b/drivers/video/fbdev/da8xx-fb.c
@@ -714,7 +714,7 @@ static int da8xx_fb_config_clk_divider(struct da8xx_fb_par *par,
 
 	if (par->lcdc_clk_rate != lcdc_clk_rate) {
 		ret = clk_set_rate(par->lcdc_clk, lcdc_clk_rate);
-		if (IS_ERR_VALUE(ret)) {
+		if (ret) {
 			dev_err(par->dev,
 				"unable to set clock rate at %u\n",
 				lcdc_clk_rate);
@@ -785,7 +785,7 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
 	int ret = 0;
 
 	ret = da8xx_fb_calc_config_clk_divider(par, panel);
-	if (IS_ERR_VALUE(ret)) {
+	if (ret) {
 		dev_err(par->dev, "unable to configure clock\n");
 		return ret;
 	}
diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
index 18edc724bf57..8a30b653c9db 100644
--- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
@@ -404,7 +404,7 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)
 	}
 
 	dsim->irq = platform_get_irq(pdev, 0);
-	if (IS_ERR_VALUE(dsim->irq)) {
+	if (dsim->irq < 0) {
 		dev_err(&pdev->dev, "failed to request dsim irq resource\n");
 		ret = -EINVAL;
 		goto error;
diff --git a/fs/afs/write.c b/fs/afs/write.c
index dfef94f70667..aa4adff3c0c2 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -643,10 +643,6 @@ ssize_t afs_file_write(struct kiocb *iocb, struct iov_iter *from)
 		return 0;
 
 	result = generic_file_write_iter(iocb, from);
-	if (IS_ERR_VALUE(result)) {
-		_leave(" = %zd", result);
-		return result;
-	}
 
 	_leave(" = %zd", result);
 	return result;
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index d4014af4f064..5c022acf116d 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -798,7 +798,7 @@ static int get_first_leaf(struct gfs2_inode *dip, u32 index,
 	int error;
 
 	error = get_leaf_nr(dip, index, &leaf_no);
-	if (!IS_ERR_VALUE(error))
+	if (error)
 		error = get_leaf(dip, leaf_no, bh_out);
 
 	return error;
@@ -1014,7 +1014,7 @@ static int dir_split_leaf(struct inode *inode, const struct qstr *name)
 
 	index = name->hash >> (32 - dip->i_depth);
 	error = get_leaf_nr(dip, index, &leaf_no);
-	if (IS_ERR_VALUE(error))
+	if (error)
 		return error;
 
 	/*  Get the old leaf block  */
diff --git a/include/linux/err.h b/include/linux/err.h
index 039b80bb56ae..141869e450a4 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,8 @@
 
 #ifndef __ASSEMBLY__
 
-#define IS_ERR_VALUE(x) (unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
+#define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
+	unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
 
 static inline void * __must_check ERR_PTR(long error)
 {
diff --git a/kernel/pid.c b/kernel/pid.c
index 4d73a834c7e6..f66162f2359b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -311,7 +311,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
 	pid->level = ns->level;
 	for (i = ns->level; i >= 0; i--) {
 		nr = alloc_pidmap(tmp);
-		if (IS_ERR_VALUE(nr)) {
+		if (nr < 0) {
 			retval = nr;
 			goto out_free;
 		}
diff --git a/net/9p/client.c b/net/9p/client.c
index ea79ee9a7348..7f6760ef7897 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -521,7 +521,7 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 		if (p9_is_proto_dotu(c))
 			err = -ecode;
 
-		if (!err || !IS_ERR_VALUE(err)) {
+		if (!err || !IS_ERR_VALUE((unsigned long)err)) {
 			err = p9_errstr2errno(ename, strlen(ename));
 
 			p9_debug(P9_DEBUG_9P, "<<< RERROR (%d) %s\n",
@@ -608,7 +608,7 @@ static int p9_check_zc_errors(struct p9_client *c, struct p9_req_t *req,
 		if (p9_is_proto_dotu(c))
 			err = -ecode;
 
-		if (!err || !IS_ERR_VALUE(err)) {
+		if (!err || !IS_ERR_VALUE((unsigned long)err)) {
 			err = p9_errstr2errno(ename, strlen(ename));
 
 			p9_debug(P9_DEBUG_9P, "<<< RERROR (%d) %s\n",
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac9c5ca..e64e850b8a9c 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -527,7 +527,7 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 		return ret;
 
 	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	if (IS_ERR_VALUE((unsigned long)e->counters.pcnt))
 		return -ENOMEM;
 
 	t = arpt_get_target(e);
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affad6ba1..3e3c5fe35c83 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -671,7 +671,7 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 		return ret;
 
 	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	if (IS_ERR_VALUE((unsigned long)e->counters.pcnt))
 		return -ENOMEM;
 
 	j = 0;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf2819b..287ad4126e24 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -684,7 +684,7 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 		return ret;
 
 	e->counters.pcnt = xt_percpu_counter_alloc();
-	if (IS_ERR_VALUE(e->counters.pcnt))
+	if (IS_ERR_VALUE((unsigned long)e->counters.pcnt))
 		return -ENOMEM;
 
 	j = 0;
diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
index 4aeb8e1a7160..6c8f084a7599 100644
--- a/sound/soc/qcom/lpass-platform.c
+++ b/sound/soc/qcom/lpass-platform.c
@@ -491,7 +491,7 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime)
 	if (v->alloc_dma_channel)
 		data->rdma_ch = v->alloc_dma_channel(drvdata);
 
-	if (IS_ERR_VALUE(data->rdma_ch))
+	if (data->rdma_ch < 0)
 		return data->rdma_ch;
 
 	drvdata->substream[data->rdma_ch] = substream;

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/10/2016 10:01 PM, Arnd Bergmann wrote:
> On Tuesday 09 February 2016 09:42:26 Andrzej Hajda wrote:
>> +cc Rasmus Villemoes, I forgot to add him earlier.
>>
>> On 02/08/2016 01:01 PM, Arnd Bergmann wrote:
>>> On Monday 08 February 2016 09:45:55 Andrzej Hajda wrote:
>>>> On 02/05/2016 11:52 AM, Arnd Bergmann wrote:
>>>>> On Thursday 04 February 2016 10:59:31 Andrew Morton wrote:
>>>> My version produces shortest code, Arnd's is the same as the old one.
>>>> On the other side Rasmus proposition seems to be the most straightforward
>>>> to me. Anyway I am not sure if the code length is the most important here.
>>>>
>>>> By the way .data segment size grows almost 4 times between gcc 4.4 and
>>>> 4.8 :)
>>>> Also numbers for arm64 looks interesting.
>>>>
>>>> Just for the record below all proposed implementations:
>>>> #define IS_ERR_VALUE_old(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>>>> #define IS_ERR_VALUE_andrzej(x) ((typeof(x))(-1) <= 0 \
>>>>                                 ? unlikely((x) <= -1) \
>>>>                                 : unlikely((x) >= (typeof(x))-MAX_ERRNO))
>>>> #define IS_ERR_VALUE_arnd(x)      (unlikely((unsigned long long)(x) >=
>>>> (unsigned long long)(typeof(x))-MAX_ERRNO))
>>>> #define IS_ERR_VALUE_rasmus(x) ({\
>>>>         typeof(x) _x = (x);\
>>>>         unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);\
>>>> })
>>>>
>>>>> Andrzej's version is a little shorter on ARM because in case of signed numbers
>>>>> it only checks for negative values, rather than checking for values in the
>>>>> [-MAX_ERRNO..-1] range. I think the original behavior is more logical
>>>>> in this case, and my version restores it.
>>>> As I looked at the usage of the macro in the kernel I have not found any
>>>> code
>>>> which could benefit from the original behavior, except some buggy code in
>>>> staging which have already pending fix[1].
>>>> But maybe it would be better to use IS_ERR_VALUE to always check if err
>>>> is in
>>>> range [-MAX_ERRNO..-1] and just use simple 'err < 0' in typical case of
>>>> signed types.
>>> If we do that, should we also make it illegal to use an invalid type
>>> for IS_ERR()? At least that could also catch any use of 'char' and 'unsigned
>>> char' that are still broken.
>> I meant rather to make such 'policy' for future code by adding some
>> comment to the macro. Optionally adding compile time warning
>> to encourage developers to change current usage, however I am
>> not sure if it is not too harsh.
>> This way it could be also good to use your version of the macro.
>> It could be also good to add compiletime_assert to prevent char types
>> as suggested by Rasmus.
>>
>> Finally it could look like:
>> /*
>>  * Use IS_ERR_VALUE only on unsigned types of at least two bytes size.
>>  * For signed types use '< 0' comparison.
>>  */
>> #define IS_ERR_VALUE(x)\
>> ({\
>>         compiletime_assert(sizeof(x) > 1, "IS_ERR_VALUE does not handle
>> byte-size types");\
>>         compiletime_assert_warning((typeof(x))(-1) > 0, "IS_ERR_VALUE
>> should be called on unsigned types only, use '< 0' instead");\
>>         (unlikely((unsigned long long)(x) >= (unsigned long
>> long)(typeof(x))-MAX_ERRNO));\
>> })
>>
> I think the easiest way to express this would be to ensure that the argument
> is 'unsigned long', like:
>
> #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
>        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))

This way you will limit it only to unsigned long type, which seems too
strict to me.
I think the macro should accept all long enough unsigned types, otherwise we
could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

Regards
Andrzej

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Arnd Bergmann]

On Thursday 11 February 2016 08:00:54 Andrzej Hajda wrote:
> > I think the easiest way to express this would be to ensure that the argument
> > is 'unsigned long', like:
> >
> > #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
> >        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
> 
> This way you will limit it only to unsigned long type, which seems too
> strict to me.
> I think the macro should accept all long enough unsigned types, otherwise we
> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

I think in practice we only care about 'int' and 'unsigned long', which are
the ones that 90% of the existing users pass in today. u32 has never worked
on 64-bit architectures so far, so we don't necessarily have to make it work.
As Al mentioned, most users of IS_ERR_VALUE are wrong anyway and should
just use 'if (err < 0)' or 'if (err)'.

We could also consider making just 'int' and 'unsigned long' allowed types
for the moment, and then change all users passing 'int' before forbidding them.

	Arnd

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

On 02/11/2016 05:39 PM, Arnd Bergmann wrote:
> On Thursday 11 February 2016 08:00:54 Andrzej Hajda wrote:
>>> I think the easiest way to express this would be to ensure that the argument
>>> is 'unsigned long', like:
>>>
>>> #define IS_ERR_VALUE(x) ((unsigned long*)NULL == (typeof (x)*)NULL && \
>>>        unlikely((unsigned long long)(x) >= (unsigned long long)(typeof(x))-MAX_ERRNO))
>> This way you will limit it only to unsigned long type, which seems too
>> strict to me.
>> I think the macro should accept all long enough unsigned types, otherwise we
>> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...
> I think in practice we only care about 'int' and 'unsigned long', which are
> the ones that 90% of the existing users pass in today. u32 has never worked
> on 64-bit architectures so far, so we don't necessarily have to make it work.
> As Al mentioned, most users of IS_ERR_VALUE are wrong anyway and should
> just use 'if (err < 0)' or 'if (err)'.
>
> We could also consider making just 'int' and 'unsigned long' allowed types
> for the moment, and then change all users passing 'int' before forbidding them.
>
> 	Arnd
>
>

OK so in short we need to fix 140 usages of the macro? Who should do them?
I can create cocci patch for more obvious cases. What about these less
obvious?
As I understand we do not touch the macro till fixes are merged?

Regards
Andrzej

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Al Viro]

On Thu, Feb 11, 2016 at 08:00:54AM +0100, Andrzej Hajda wrote:

> This way you will limit it only to unsigned long type, which seems too
> strict to me.
> I think the macro should accept all long enough unsigned types, otherwise we
> could end up with bunch of macros IS_ERR_VALUE_U32, IS_ERR_VALUE_ULL...

... or use it a whole lot less.  Which is a Good Thing(tm), because it
corrects the damage caused by seriously flawed concept of "it's a bad
value, mmkay?" kind of predicate that could be used in all situations,
without having to look at the calling conventions of the functions
used to produce the value.  It doesn't work and it has already spread
around too much.

For starters, it conflates "0 on success, -E... on error" with "non-zero
on success, 0 on failure" with "-E... on error, 0 or positive on success"
with "positive on success, non-positive on error" with "address of some
object or -E..., 0 to be treated as -ENOMEM", etc.

*All* of those are present in the kernel.  Promoting blind use of magic
macro will keep causing bugs, and extra degree of polymorphism will only
encourage such blind use.

Look at the actual users.  IS_ERR() aside (and don't get me started on
the abortion that is IS_ERR_OR_NULL()), there is one more or less common
legitimate use.  Treatement of vm_mmap()/do_mmap_pgoff()/etc. return values.
The rest is very mixed bag.  To pick a random one (in net/9p/client.c):
                ename = NULL;
                err = p9pdu_readf(req->rc, c->proto_version, "s?d",
                                  &ename, &ecode);
                if (err)
                        goto out_err;

                if (p9_is_proto_dotu(c))
                        err = -ecode;

                if (!err || !IS_ERR_VALUE(err)) {
                        err = p9_errstr2errno(ename, strlen(ename));

                        p9_debug(P9_DEBUG_9P, "<<< RERROR (%d) %s\n",
                                 -ecode, ename);
                }
                kfree(ename);

What's going on here?  We have an RERROR or RLERROR reply coming from server.
We want to convert that to kernel-recognizable error value.  Normal 9P
uses strings for errors; kernel uses numbers.  String is represented as
16bit little-endian length + that many characters; that's all that plain
9P puts into RERROR payload.  9P.U (unix extensions) appends suggested 32bit
little-endian numeric value after that.  9P.L simply puts the 32bit l-e
numeric value, with no strings involved.

The quoted code deals with 9P and 9P.U.  Response is parsed (FWIW, '?' in
format is "ignore the rest for plain 9P"), any failure is returned as
an error in its own right, then for 9P.U we look at the numeric value
and if it's from 1 to MAX_ERRNO we just accept that.  Otherwise (plain
9P or a strange numeric value in 9P.U) we look at the string part and
convert it to E...  For 9P.L we simply accept the error value given.

Incidentally, that use of numeric values assumes that all relevant error
values will have the same encoding on all architectures - 9P is a network
protocol, after all.  <checks the list of error values used> Uh-oh...
        {"Resource temporarily unavailable", EAGAIN},
        {"exclusive use file already open", EAGAIN},
        {"file is in use", EAGAIN},
... and while the normal value of EAGAIN is 11, on alpha it's 35.  Oops...
It also contains a lot more values than arch-consistent subset in errno-base.h
- there are only 34 in the latter (and some architectures redefine some of
those, like alpha does to EAGAIN) and there's more than twice as much in the
former...  Some of those are fairly unlikely to be generated by server, but
e.g. ENAMETOOLONG (63 on alpha and sparc, 78 on mips, 248 on parisc, 36 on
everything else) is not impossible for a fileserver at all.  Looks like
trouble...

Further investigation belongs on 9P list, but in this case a blind use of
IS_ERR_VALUE() has turned out to be an indication of a bug.  QED.  Any such
places need careful review, and no amount of magic will avoid the need to
understand the surrounding code.  I'd be seriously tempted to add "uses
IS_ERR_VALUE outside of few known-good places" to checkpatch.pl, if not
for the fact that inevitable flood of mindless "fixes" is precisely what
we do *NOT* need in this case...

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Rasmus Villemoes]

On Wed, Feb 03 2016, Andrzej Hajda <a.hajda@samsung.com> wrote:

> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
>
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> v3:
>  - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
>    older than 4.8,
> v2:
>  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
>  - expand commit message.
> ---
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..b7d4a9f 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> +				? unlikely((x) <= -1) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  

I'm a bit worried that you consider any negative value an error when x
is signed - at least that's a change which deserves some comment why
that's ok. For example, I could imagine someone using e.g. INT_MIN as a
sentinel return value meaning 'not an error, but something special
still'.

I think that, since we're there, one should stick in a BUILD_BUG_ON to
catch people passing in a u8 or s8.

Something that seems that it would work for all (wide enough) types
is

({
typeof(x) _x = (x);
BUILD_BUG_ON(sizeof(_x) < 2);
unlikely(_x >= (typeof(x))-MAX_ERRNO &&  _x <= (typeof(x))-1);
})

Whether x is signed or not we want the first condition. If x is unsigned
and at least as wide as int, the -1 in the second condition will be
promoted to the max value typeof(x) can hold, so gcc should trivially
remove the check, and if x is signed, this keeps the previous
semantics. It's also a pattern I expect gcc to be able to optimize
pretty well (that is, if x has type signed int, I expect it'll recognize
that these tests are better done as a single unsigned comparison). Maybe
it'll still give a -Wtype-limit warning for the unsigned case.

If we go with the above, maybe one should also stick in something which
ensures x is not a pointer; (void)(_x+_x); should do it. 

Rasmus

Re: [PATCH v3] err.h: allow IS_ERR_VALUE to handle properly more types

[Guenter Roeck]

On Fri, Feb 05, 2016 at 12:37:00AM +0100, Rasmus Villemoes wrote:
> On Wed, Feb 03 2016, Andrzej Hajda <a.hajda@samsung.com> wrote:
> 
> > Current implementation of IS_ERR_VALUE works correctly only with
> > following types:
> > - unsigned long,
> > - short, int, long.
> > Other types are handled incorrectly either on 32-bit either on 64-bit
> > either on both architectures.
> > The patch fixes it by comparing argument with MAX_ERRNO casted
> > to argument's type for unsigned types and comparing with zero for signed
> > types. As a result all integer types bigger than char are handled properly.
> >
> >
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> > v3:
> >  - use '<= -1' instead of '< 0' to silence verbose warnings for gcc
> >    older than 4.8,
> > v2:
> >  - use '<= 0' instead of '< 0' to silence gcc verbose warnings,
> >  - expand commit message.
> > ---
> >  include/linux/err.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/err.h b/include/linux/err.h
> > index 56762ab..b7d4a9f 100644
> > --- a/include/linux/err.h
> > +++ b/include/linux/err.h
> > @@ -18,7 +18,9 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> > +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \
> > +				? unlikely((x) <= -1) \
> > +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
> >  
> 
> I'm a bit worried that you consider any negative value an error when x
> is signed - at least that's a change which deserves some comment why
> that's ok. For example, I could imagine someone using e.g. INT_MIN as a
> sentinel return value meaning 'not an error, but something special
> still'.
> 
Theoretically maybe, but I think that is quite unlikely in the real world.

It turns out that

	if (-22 >= (unsigned long)-MAX_ERRNO)
            printf("This is odd\n");

actually does print "This is odd" (because -22 is promoted to unsigned long).

Instead of relying on such behavior, I think it would be better to convert
uses of IS_ERR_VALUE() on integer values to direct comparisons.
A coccinelle script to do that conversion that is already available for
pm functions (scripts/coccinelle/api/pm_runtime.cocci). Such a conversion
would make the code easier to read, and reduce code size instead of
(at least potentially) increasing it.

Guenter

Re: [PATCH] err.h: allow IS_ERR_VALUE to handle properly more types

[Andrzej Hajda]

Hi Andrew,

Have you looked at this patch?
Are there chances to merge it to next?

Two more things I have found:
- current version of IS_ERR_VALUE does not work with unsigned long long
on 32bit arch - commit message should be fixed,
- gcc issues warnings when -Wtype-limits option is enabled, it can be
easily silenced by changing first operator '<' to '<=' in the macro.

Regards
Andrzej

On 01/07/2016 03:58 PM, Andrzej Hajda wrote:
> Current implementation of IS_ERR_VALUE works correctly only with
> following types:
> - unsigned long, unsigned long long,
> - short, int, long.
> Other types are handled incorrectly either on 32-bit either on 64-bit
> either on both architectures.
> The patch fixes it by comparing argument with MAX_ERRNO casted
> to argument's type for unsigned types and comparing with zero for signed
> types. As a result all integer types bigger than char are handled properly.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi,
> 
> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 cases
> it is used incorrectly, ie it can hide errors depending of 32/64 bit architecture.
> Instead of fixing usage I propose to enhance the macro to cover more types.
> And just for record: the macro is used 101 times with signed variables, I am not
> sure if it should be preferred over simple comparison "ret < 0", but the new version
> can do it as well.
> 
> In my previous attempt I have added type checking to the macro, I am not sure which
> approach is better[1]. Anyway the previous patch did not catch developers attention.
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2111532
> 
> And below list of detected potential errors:
> drivers/char/mem.c:698:45-46: WARNING: incorrect argument type in IS_ERR_VALUE(( unsigned long long ) offset)
> drivers/media/platform/soc_camera/atmel-isi.c:1089:21-22: WARNING: incorrect argument type in IS_ERR_VALUE(irq)
> 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/tty/serial/clps711x.c:471:29-30: WARNING: incorrect argument type in IS_ERR_VALUE(s -> port . irq)
> drivers/tty/serial/digicolor-usart.c:485:30-31: WARNING: incorrect argument type in IS_ERR_VALUE(dp -> port . irq)
> drivers/usb/gadget/udc/fsl_qe_udc.c:2369:26-27: WARNING: incorrect argument type in IS_ERR_VALUE(tmp_addr)
> drivers/video/fbdev/exynos/exynos_mipi_dsi.c:406:27-28: WARNING: incorrect argument type in IS_ERR_VALUE(dsim -> irq)
> net/ipv4/netfilter/arp_tables.c:1427:39-40: WARNING: incorrect argument type in IS_ERR_VALUE(iter1 -> counters . pcnt)
> net/ipv4/netfilter/arp_tables.c:530:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
> net/ipv4/netfilter/ip_tables.c:1614:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
> net/ipv4/netfilter/ip_tables.c:674:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
> net/ipv6/netfilter/ip6_tables.c:1624:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
> net/ipv6/netfilter/ip6_tables.c:687:34-35: WARNING: incorrect argument type in IS_ERR_VALUE(e -> counters . pcnt)
> drivers/net/ethernet/freescale/fs_enet/mac-fcc.c:110:35-36: WARNING: unknown argument type in IS_ERR_VALUE(fpi -> dpram_offset)
> 
> Regards
> Andrzej
> 
>  include/linux/err.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/err.h b/include/linux/err.h
> index 56762ab..40eca39 100644
> --- a/include/linux/err.h
> +++ b/include/linux/err.h
> @@ -18,7 +18,9 @@
>  
>  #ifndef __ASSEMBLY__
>  
> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) < 0 \
> +				? unlikely((x) < 0) \
> +				: unlikely((x) >= (typeof(x))-MAX_ERRNO))
>  
>  static inline void * __must_check ERR_PTR(long error)
>  {
>