| Message ID | 1455030539-10798-1-git-send-email-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: > IS_ERR_VALUE() assumes that its parameter is an unsigned long. > It can not be used to check if an unsigned int reflects an error. > Doing so can result in the following build warning. > > drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > include/linux/err.h:21:38: warning: > comparison is always false due to limited range of data type > drivers/tty/serial/digicolor-usart.c:485:6: note: > in expansion of macro ‘IS_ERR_VALUE’ > > If that warning is seen, an error return from platform_get_irq() is missed. > > The patch looks correct to me, but what compiler version and which kernel tree is it that triggered the warning? Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the changes are still under discussion, but I don't see that warning with any of the versions. Arnd
On 02/09/2016 07:26 AM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: >> IS_ERR_VALUE() assumes that its parameter is an unsigned long. >> It can not be used to check if an unsigned int reflects an error. >> Doing so can result in the following build warning. >> >> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: >> include/linux/err.h:21:38: warning: >> comparison is always false due to limited range of data type >> drivers/tty/serial/digicolor-usart.c:485:6: note: >> in expansion of macro ‘IS_ERR_VALUE’ >> >> If that warning is seen, an error return from platform_get_irq() is missed. >> >> > > The patch looks correct to me, but what compiler version and which kernel > tree is it that triggered the warning? > > Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the > changes are still under discussion, but I don't see that warning with > any of the versions. > I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. I agree that fixing the problem in IS_ERR_VALUE() is preferrable. Guenter
On Tuesday 09 February 2016 18:37:46 Guenter Roeck wrote: > On 02/09/2016 07:26 AM, Arnd Bergmann wrote: > > On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: > >> IS_ERR_VALUE() assumes that its parameter is an unsigned long. > >> It can not be used to check if an unsigned int reflects an error. > >> Doing so can result in the following build warning. > >> > >> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > >> include/linux/err.h:21:38: warning: > >> comparison is always false due to limited range of data type > >> drivers/tty/serial/digicolor-usart.c:485:6: note: > >> in expansion of macro ‘IS_ERR_VALUE’ > >> > >> If that warning is seen, an error return from platform_get_irq() is missed. > >> > >> > > > > The patch looks correct to me, but what compiler version and which kernel > > tree is it that triggered the warning? > > > > Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the > > changes are still under discussion, but I don't see that warning with > > any of the versions. > > > I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. > > I agree that fixing the problem in IS_ERR_VALUE() is preferrable. > > Ah, W=1 explains it. We are still debating about the proper solution. Al Viro pointed out that most users of IS_ERR_VALUE() shouldn't be using it at all, so your patch is probably best here after all. Arnd
On 02/10/2016 07:21 AM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 18:37:46 Guenter Roeck wrote: >> On 02/09/2016 07:26 AM, Arnd Bergmann wrote: >>> On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: >>>> IS_ERR_VALUE() assumes that its parameter is an unsigned long. >>>> It can not be used to check if an unsigned int reflects an error. >>>> Doing so can result in the following build warning. >>>> >>>> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: >>>> include/linux/err.h:21:38: warning: >>>> comparison is always false due to limited range of data type >>>> drivers/tty/serial/digicolor-usart.c:485:6: note: >>>> in expansion of macro ‘IS_ERR_VALUE’ >>>> >>>> If that warning is seen, an error return from platform_get_irq() is missed. >>>> >>>> >>> >>> The patch looks correct to me, but what compiler version and which kernel >>> tree is it that triggered the warning? >>> >>> Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the >>> changes are still under discussion, but I don't see that warning with >>> any of the versions. >>> >> I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. >> >> I agree that fixing the problem in IS_ERR_VALUE() is preferrable. >> >> > > Ah, W=1 explains it. We are still debating about the proper solution. Al Viro > pointed out that most users of IS_ERR_VALUE() shouldn't be using it at all, > so your patch is probably best here after all. > Yes, after looking into it some more I agree. Coccinelle should be able to handle most of the conversions automatically. I actually tried to write a script, just for the fun of it, but it misses some of the problem cases in patch mode. Maybe I get it working tonight. Guenter
On 02/11/2016 04:38 AM, Guenter Roeck wrote: > On 02/10/2016 07:21 AM, Arnd Bergmann wrote: >> On Tuesday 09 February 2016 18:37:46 Guenter Roeck wrote: >>> On 02/09/2016 07:26 AM, Arnd Bergmann wrote: >>>> On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: >>>>> IS_ERR_VALUE() assumes that its parameter is an unsigned long. >>>>> It can not be used to check if an unsigned int reflects an error. >>>>> Doing so can result in the following build warning. >>>>> >>>>> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: >>>>> include/linux/err.h:21:38: warning: >>>>> comparison is always false due to limited range of data type >>>>> drivers/tty/serial/digicolor-usart.c:485:6: note: >>>>> in expansion of macro ‘IS_ERR_VALUE’ >>>>> >>>>> If that warning is seen, an error return from platform_get_irq() is missed. >>>>> >>>>> >>>> The patch looks correct to me, but what compiler version and which kernel >>>> tree is it that triggered the warning? >>>> >>>> Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the >>>> changes are still under discussion, but I don't see that warning with >>>> any of the versions. >>>> >>> I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. >>> >>> I agree that fixing the problem in IS_ERR_VALUE() is preferrable. >>> >>> >> Ah, W=1 explains it. We are still debating about the proper solution. Al Viro >> pointed out that most users of IS_ERR_VALUE() shouldn't be using it at all, >> so your patch is probably best here after all. >> > Yes, after looking into it some more I agree. Coccinelle should be able to handle > most of the conversions automatically. I actually tried to write a script, just > for the fun of it, but it misses some of the problem cases in patch mode. > Maybe I get it working tonight. > > Guenter > Maybe it can be helpful. The script which classifies all IS_ERR_VALUE usages as correct, signed, incorrect and unknown (cocci was not able to detect type). Adding patch mode should be quite simple. // Options: --all-includes --include include/uapi/linux/netfilter_ipv6/ip6_tables.h --include include/uapi/linux/netfilter_ipv4/ip_tables.h --include include/uapi/linux/netfilter_arp/arp_tables.h --include include/uapi/linux/netfilter/x_tables.h --include include/linux/mm_types.h virtual context virtual report @r@ typedef bool, u8, u16, u32, u64, s8, s16, s32, s64, __u64, dma_addr_t, Elf32_Addr, Elf64_Addr; {unsigned long, size_t} ev; {short, int, long, s16, s32, ssize_t} ew; {char, unsigned char, unsigned short, unsigned int, long long, unsigned long long, bool, u8, u16, u32, u64, s64, __u64, dma_addr_t, Elf32_Addr, Elf64_Addr} ei; expression eu; position p; @@ IS_ERR_VALUE(\(ev \| ew \| ei \| eu \))@p @script:python depends on report@ p << r.p = []; ev << r.ev = ""; ew << r.ew = ""; ei << r.ei = ""; eu << r.eu = ""; @@ t = "" if ev: t = "correct" if ew: t = "signed" if ei: t = "incorrect" if eu: t = "unknown" e = ev + ew + ei + eu; if t: msg = "WARNING: %s argument type in IS_ERR_VALUE(%s)" % (t, e) coccilib.report.print_report(p[0], msg)
Hi Guenter, On Tue, Feb 09, 2016 at 07:08:59AM -0800, Guenter Roeck wrote: > IS_ERR_VALUE() assumes that its parameter is an unsigned long. > It can not be used to check if an unsigned int reflects an error. > Doing so can result in the following build warning. > > drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > include/linux/err.h:21:38: warning: > comparison is always false due to limited range of data type > drivers/tty/serial/digicolor-usart.c:485:6: note: > in expansion of macro ‘IS_ERR_VALUE’ > > If that warning is seen, an error return from platform_get_irq() is missed. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Acked-by: Baruch Siach <baruch@tkos.co.il> Thanks, baruch > --- > drivers/tty/serial/digicolor-usart.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/digicolor-usart.c b/drivers/tty/serial/digicolor-usart.c > index a80cdad114f3..02ad6953b167 100644 > --- a/drivers/tty/serial/digicolor-usart.c > +++ b/drivers/tty/serial/digicolor-usart.c > @@ -453,7 +453,7 @@ static struct uart_driver digicolor_uart = { > static int digicolor_uart_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > - int ret, index; > + int irq, ret, index; > struct digicolor_port *dp; > struct resource *res; > struct clk *uart_clk; > @@ -481,9 +481,10 @@ static int digicolor_uart_probe(struct platform_device *pdev) > if (IS_ERR(dp->port.membase)) > return PTR_ERR(dp->port.membase); > > - dp->port.irq = platform_get_irq(pdev, 0); > - if (IS_ERR_VALUE(dp->port.irq)) > - return dp->port.irq; > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + dp->port.irq = irq; > > dp->port.iotype = UPIO_MEM; > dp->port.uartclk = clk_get_rate(uart_clk);
On Tue, Feb 09, 2016 at 06:37:46PM -0800, Guenter Roeck wrote: > On 02/09/2016 07:26 AM, Arnd Bergmann wrote: > >On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: > >>IS_ERR_VALUE() assumes that its parameter is an unsigned long. > >>It can not be used to check if an unsigned int reflects an error. > >>Doing so can result in the following build warning. > >> > >>drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > >>include/linux/err.h:21:38: warning: > >> comparison is always false due to limited range of data type > >>drivers/tty/serial/digicolor-usart.c:485:6: note: > >> in expansion of macro ‘IS_ERR_VALUE’ > >> > >>If that warning is seen, an error return from platform_get_irq() is missed. > >> > >> > > > >The patch looks correct to me, but what compiler version and which kernel > >tree is it that triggered the warning? > > > >Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the > >changes are still under discussion, but I don't see that warning with > >any of the versions. > > > I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. > > I agree that fixing the problem in IS_ERR_VALUE() is preferrable. I disagree. What happens if (eg) you decide to do this: u8 irq; irq = platform_get_irq(...); if (IS_ERR_VALUE(irq)) ... is that the fault of IS_ERR_VALUE() that it will never be true? No, it's a programming error in the caller, because the caller is using the wrong type here - in fact, you can't do anything in IS_ERR_VALUE() to correct that. The same is true if you assign an error value to an unsigned int: the problem is in the caller, not in IS_ERR_VALUE(). What would be preferable is if there was some way IS_ERR_VALUE() could detect if it's used on something that isn't at least a short, and is signed, but I suspect coccinelle can do that much better than trying to work out some way to get the C compiler to check that.
On Thu, Feb 11, 2016 at 10:46:50AM +0000, Russell King - ARM Linux wrote: > On Tue, Feb 09, 2016 at 06:37:46PM -0800, Guenter Roeck wrote: > > On 02/09/2016 07:26 AM, Arnd Bergmann wrote: > > >On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: > > >>IS_ERR_VALUE() assumes that its parameter is an unsigned long. > > >>It can not be used to check if an unsigned int reflects an error. > > >>Doing so can result in the following build warning. > > >> > > >>drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > > >>include/linux/err.h:21:38: warning: > > >> comparison is always false due to limited range of data type > > >>drivers/tty/serial/digicolor-usart.c:485:6: note: > > >> in expansion of macro ‘IS_ERR_VALUE’ > > >> > > >>If that warning is seen, an error return from platform_get_irq() is missed. > > >> > > >> > > > > > >The patch looks correct to me, but what compiler version and which kernel > > >tree is it that triggered the warning? > > > > > >Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the > > >changes are still under discussion, but I don't see that warning with > > >any of the versions. > > > > > I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. > > > > I agree that fixing the problem in IS_ERR_VALUE() is preferrable. > > I disagree. What happens if (eg) you decide to do this: > > u8 irq; > > irq = platform_get_irq(...); > if (IS_ERR_VALUE(irq)) > ... > > is that the fault of IS_ERR_VALUE() that it will never be true? No, it's > a programming error in the caller, because the caller is using the wrong > type here - in fact, you can't do anything in IS_ERR_VALUE() to correct > that. > Yes, I think we all reached that conclusion by now. Thanks, Guenter
On Thu, Feb 11, 2016 at 07:45:11AM +0100, Andrzej Hajda wrote: > On 02/11/2016 04:38 AM, Guenter Roeck wrote: > > On 02/10/2016 07:21 AM, Arnd Bergmann wrote: > >> On Tuesday 09 February 2016 18:37:46 Guenter Roeck wrote: > >>> On 02/09/2016 07:26 AM, Arnd Bergmann wrote: > >>>> On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: > >>>>> IS_ERR_VALUE() assumes that its parameter is an unsigned long. > >>>>> It can not be used to check if an unsigned int reflects an error. > >>>>> Doing so can result in the following build warning. > >>>>> > >>>>> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: > >>>>> include/linux/err.h:21:38: warning: > >>>>> comparison is always false due to limited range of data type > >>>>> drivers/tty/serial/digicolor-usart.c:485:6: note: > >>>>> in expansion of macro ‘IS_ERR_VALUE’ > >>>>> > >>>>> If that warning is seen, an error return from platform_get_irq() is missed. > >>>>> > >>>>> > >>>> The patch looks correct to me, but what compiler version and which kernel > >>>> tree is it that triggered the warning? > >>>> > >>>> Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the > >>>> changes are still under discussion, but I don't see that warning with > >>>> any of the versions. > >>>> > >>> I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. > >>> > >>> I agree that fixing the problem in IS_ERR_VALUE() is preferrable. > >>> > >>> > >> Ah, W=1 explains it. We are still debating about the proper solution. Al Viro > >> pointed out that most users of IS_ERR_VALUE() shouldn't be using it at all, > >> so your patch is probably best here after all. > >> > > Yes, after looking into it some more I agree. Coccinelle should be able to handle > > most of the conversions automatically. I actually tried to write a script, just > > for the fun of it, but it misses some of the problem cases in patch mode. > > Maybe I get it working tonight. > > > > Guenter > > > > Maybe it can be helpful. The script which classifies all IS_ERR_VALUE > usages as > correct, signed, incorrect and unknown (cocci was not able to detect type). > Adding patch mode should be quite simple. > > // Options: --all-includes --include > include/uapi/linux/netfilter_ipv6/ip6_tables.h --include > include/uapi/linux/netfilter_ipv4/ip_tables.h --include > include/uapi/linux/netfilter_arp/arp_tables.h --include > include/uapi/linux/netfilter/x_tables.h --include include/linux/mm_types.h > > virtual context > virtual report > > @r@ > typedef bool, u8, u16, u32, u64, s8, s16, s32, s64, __u64, dma_addr_t, > Elf32_Addr, Elf64_Addr; > {unsigned long, size_t} ev; > {short, int, long, s16, s32, ssize_t} ew; > {char, unsigned char, unsigned short, unsigned int, long long, unsigned > long long, bool, u8, u16, u32, u64, s64, __u64, dma_addr_t, Elf32_Addr, > Elf64_Addr} ei; > expression eu; > position p; > @@ > > IS_ERR_VALUE(\(ev \| ew \| ei \| eu \))@p > > @script:python depends on report@ > p << r.p = []; > ev << r.ev = ""; > ew << r.ew = ""; > ei << r.ei = ""; > eu << r.eu = ""; This doesn't work for me; it bails out at the assignments. If I remove the assignments, it does not report anything. Any idea what might be wrong ? Thanks, Guenter > @@ > > t = "" > if ev: t = "correct" > if ew: t = "signed" > if ei: t = "incorrect" > if eu: t = "unknown" > > > e = ev + ew + ei + eu; > if t: > msg = "WARNING: %s argument type in IS_ERR_VALUE(%s)" % (t, e) > coccilib.report.print_report(p[0], msg) > >
On 02/11/2016 04:32 PM, Guenter Roeck wrote: > On Thu, Feb 11, 2016 at 07:45:11AM +0100, Andrzej Hajda wrote: >> On 02/11/2016 04:38 AM, Guenter Roeck wrote: >>> On 02/10/2016 07:21 AM, Arnd Bergmann wrote: >>>> On Tuesday 09 February 2016 18:37:46 Guenter Roeck wrote: >>>>> On 02/09/2016 07:26 AM, Arnd Bergmann wrote: >>>>>> On Tuesday 09 February 2016 07:08:59 Guenter Roeck wrote: >>>>>>> IS_ERR_VALUE() assumes that its parameter is an unsigned long. >>>>>>> It can not be used to check if an unsigned int reflects an error. >>>>>>> Doing so can result in the following build warning. >>>>>>> >>>>>>> drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: >>>>>>> include/linux/err.h:21:38: warning: >>>>>>> comparison is always false due to limited range of data type >>>>>>> drivers/tty/serial/digicolor-usart.c:485:6: note: >>>>>>> in expansion of macro ‘IS_ERR_VALUE’ >>>>>>> >>>>>>> If that warning is seen, an error return from platform_get_irq() is missed. >>>>>>> >>>>>>> >>>>>> The patch looks correct to me, but what compiler version and which kernel >>>>>> tree is it that triggered the warning? >>>>>> >>>>>> Andrzej Hajda just modified the definition of IS_ERR_VALUE(), and the >>>>>> changes are still under discussion, but I don't see that warning with >>>>>> any of the versions. >>>>>> >>>>> I see it with gcc 5.1 and 5.2 (and W=1). I did not see / notice Andrzej's patch. >>>>> >>>>> I agree that fixing the problem in IS_ERR_VALUE() is preferrable. >>>>> >>>>> >>>> Ah, W=1 explains it. We are still debating about the proper solution. Al Viro >>>> pointed out that most users of IS_ERR_VALUE() shouldn't be using it at all, >>>> so your patch is probably best here after all. >>>> >>> Yes, after looking into it some more I agree. Coccinelle should be able to handle >>> most of the conversions automatically. I actually tried to write a script, just >>> for the fun of it, but it misses some of the problem cases in patch mode. >>> Maybe I get it working tonight. >>> >>> Guenter >>> >> Maybe it can be helpful. The script which classifies all IS_ERR_VALUE >> usages as >> correct, signed, incorrect and unknown (cocci was not able to detect type). >> Adding patch mode should be quite simple. >> >> // Options: --all-includes --include >> include/uapi/linux/netfilter_ipv6/ip6_tables.h --include >> include/uapi/linux/netfilter_ipv4/ip_tables.h --include >> include/uapi/linux/netfilter_arp/arp_tables.h --include >> include/uapi/linux/netfilter/x_tables.h --include include/linux/mm_types.h >> >> virtual context >> virtual report >> >> @r@ >> typedef bool, u8, u16, u32, u64, s8, s16, s32, s64, __u64, dma_addr_t, >> Elf32_Addr, Elf64_Addr; >> {unsigned long, size_t} ev; >> {short, int, long, s16, s32, ssize_t} ew; >> {char, unsigned char, unsigned short, unsigned int, long long, unsigned >> long long, bool, u8, u16, u32, u64, s64, __u64, dma_addr_t, Elf32_Addr, >> Elf64_Addr} ei; >> expression eu; >> position p; >> @@ >> >> IS_ERR_VALUE(\(ev \| ew \| ei \| eu \))@p >> >> @script:python depends on report@ >> p << r.p = []; >> ev << r.ev = ""; >> ew << r.ew = ""; >> ei << r.ei = ""; >> eu << r.eu = ""; > This doesn't work for me; it bails out at the assignments. If I remove > the assignments, it does not report anything. > > Any idea what might be wrong ? Old version of spatch. Default values are supported since 1.0.3. Regards Andrzej
diff --git a/drivers/tty/serial/digicolor-usart.c b/drivers/tty/serial/digicolor-usart.c index a80cdad114f3..02ad6953b167 100644 --- a/drivers/tty/serial/digicolor-usart.c +++ b/drivers/tty/serial/digicolor-usart.c @@ -453,7 +453,7 @@ static struct uart_driver digicolor_uart = { static int digicolor_uart_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; - int ret, index; + int irq, ret, index; struct digicolor_port *dp; struct resource *res; struct clk *uart_clk; @@ -481,9 +481,10 @@ static int digicolor_uart_probe(struct platform_device *pdev) if (IS_ERR(dp->port.membase)) return PTR_ERR(dp->port.membase); - dp->port.irq = platform_get_irq(pdev, 0); - if (IS_ERR_VALUE(dp->port.irq)) - return dp->port.irq; + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + dp->port.irq = irq; dp->port.iotype = UPIO_MEM; dp->port.uartclk = clk_get_rate(uart_clk);
IS_ERR_VALUE() assumes that its parameter is an unsigned long. It can not be used to check if an unsigned int reflects an error. Doing so can result in the following build warning. drivers/tty/serial/digicolor-usart.c: In function ‘digicolor_uart_probe’: include/linux/err.h:21:38: warning: comparison is always false due to limited range of data type drivers/tty/serial/digicolor-usart.c:485:6: note: in expansion of macro ‘IS_ERR_VALUE’ If that warning is seen, an error return from platform_get_irq() is missed. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/tty/serial/digicolor-usart.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)