linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
@ 2016-02-09 15:08 Guenter Roeck
       [not found] ` <1455030539-10798-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
  2016-02-11  6:50 ` Baruch Siach
  0 siblings, 2 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-02-09 15:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Baruch Siach, linux-serial, linux-arm-kernel,
	linux-kernel, kernel-testers, Guenter Roeck

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(-)

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);
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
       [not found] ` <1455030539-10798-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2016-02-09 15:26   ` Arnd Bergmann
  2016-02-10  2:37     ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-02-09 15:26 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Guenter Roeck, Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA, Andrzej Hajda

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
  2016-02-09 15:26   ` Arnd Bergmann
@ 2016-02-10  2:37     ` Guenter Roeck
       [not found]       ` <56BAA27A.9050605-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-02-10  2:37 UTC (permalink / raw)
  To: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA, Andrzej Hajda

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
       [not found]       ` <56BAA27A.9050605-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2016-02-10 15:21         ` Arnd Bergmann
  2016-02-11  3:38           ` Guenter Roeck
  2016-02-11 10:46         ` Russell King - ARM Linux
  1 sibling, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2016-02-10 15:21 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA, Andrzej Hajda

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
  2016-02-10 15:21         ` Arnd Bergmann
@ 2016-02-11  3:38           ` Guenter Roeck
       [not found]             ` <56BC022B.7030004-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-02-11  3:38 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA, Andrzej Hajda

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
       [not found]             ` <56BC022B.7030004-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2016-02-11  6:45               ` Andrzej Hajda
       [not found]                 ` <56BC2DF7.20803-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 11+ messages in thread
From: Andrzej Hajda @ 2016-02-11  6:45 UTC (permalink / raw)
  To: Guenter Roeck, Arnd Bergmann
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA

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)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
  2016-02-09 15:08 [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE Guenter Roeck
       [not found] ` <1455030539-10798-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2016-02-11  6:50 ` Baruch Siach
  1 sibling, 0 replies; 11+ messages in thread
From: Baruch Siach @ 2016-02-11  6:50 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-arm-kernel,
	linux-kernel, kernel-testers

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);

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
       [not found]       ` <56BAA27A.9050605-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
  2016-02-10 15:21         ` Arnd Bergmann
@ 2016-02-11 10:46         ` Russell King - ARM Linux
  2016-02-11 15:13           ` Guenter Roeck
  1 sibling, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux @ 2016-02-11 10:46 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Baruch Siach, Greg Kroah-Hartman,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrzej Hajda,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA

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.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
  2016-02-11 10:46         ` Russell King - ARM Linux
@ 2016-02-11 15:13           ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-02-11 15:13 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Arnd Bergmann, linux-arm-kernel, Baruch Siach, Greg Kroah-Hartman,
	linux-kernel, Andrzej Hajda, linux-serial, Jiri Slaby,
	kernel-testers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
       [not found]                 ` <56BC2DF7.20803-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2016-02-11 15:32                   ` Guenter Roeck
  2016-02-12  6:38                     ` Andrzej Hajda
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-02-11 15:32 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Greg Kroah-Hartman, Baruch Siach,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA

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)
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE
  2016-02-11 15:32                   ` Guenter Roeck
@ 2016-02-12  6:38                     ` Andrzej Hajda
  0 siblings, 0 replies; 11+ messages in thread
From: Andrzej Hajda @ 2016-02-12  6:38 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Arnd Bergmann, linux-arm-kernel, Greg Kroah-Hartman, Baruch Siach,
	linux-kernel, linux-serial, Jiri Slaby, kernel-testers

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-02-12  6:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-09 15:08 [PATCH] tty/serial: digicolor: Fix bad usage of IS_ERR_VALUE Guenter Roeck
     [not found] ` <1455030539-10798-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2016-02-09 15:26   ` Arnd Bergmann
2016-02-10  2:37     ` Guenter Roeck
     [not found]       ` <56BAA27A.9050605-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2016-02-10 15:21         ` Arnd Bergmann
2016-02-11  3:38           ` Guenter Roeck
     [not found]             ` <56BC022B.7030004-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2016-02-11  6:45               ` Andrzej Hajda
     [not found]                 ` <56BC2DF7.20803-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2016-02-11 15:32                   ` Guenter Roeck
2016-02-12  6:38                     ` Andrzej Hajda
2016-02-11 10:46         ` Russell King - ARM Linux
2016-02-11 15:13           ` Guenter Roeck
2016-02-11  6:50 ` Baruch Siach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).