[patch 1/3] spi: armada-3700: Signedness bug in probe

[patch 1/3] spi: armada-3700: Signedness bug in probe

[Dan Carpenter]

We need "irq" to be an int because platform_get_irq() returns negative
error codes.

Fixes: 5762ab71eb24 ("spi: Add support for Armada 3700 SPI Controller")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c
index e89da0af45d2..5c7508dc6f0b 100644
--- a/drivers/spi/spi-armada-3700.c
+++ b/drivers/spi/spi-armada-3700.c
@@ -108,7 +108,7 @@ struct a3700_spi {
 	struct spi_master *master;
 	void __iomem *base;
 	struct clk *clk;
-	unsigned int irq;
+	int irq;
 	unsigned int flags;
 	bool xmit_data;
 	const u8 *tx_buf;

Re: [patch 1/3] spi: armada-3700: Signedness bug in probe

[Romain Perier]

Hello,

Le 16/12/2016 à 10:33, Dan Carpenter a écrit :
> We need "irq" to be an int because platform_get_irq() returns negative
> error codes.
>
> Fixes: 5762ab71eb24 ("spi: Add support for Armada 3700 SPI Controller")
> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c
> index e89da0af45d2..5c7508dc6f0b 100644
> --- a/drivers/spi/spi-armada-3700.c
> +++ b/drivers/spi/spi-armada-3700.c
> @@ -108,7 +108,7 @@ struct a3700_spi {
>  	struct spi_master *master;
>  	void __iomem *base;
>  	struct clk *clk;
> -	unsigned int irq;
> +	int irq;
>  	unsigned int flags;
>  	bool xmit_data;
>  	const u8 *tx_buf;
>

It is already fixed, see 
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=f6f0083cca66e673cca6fa26b52b107b5570081d

And no we cannot just replace the type of the irq field like this 
because devm_request_irq/request_irq require an unsigned int as "irq" 
parameter.

Romain

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/3] spi: armada-3700: Signedness bug in probe

[Dan Carpenter]

On Fri, Dec 16, 2016 at 10:36:29AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 16/12/2016 à 10:33, Dan Carpenter a écrit :
> >We need "irq" to be an int because platform_get_irq() returns negative
> >error codes.
> >
> >Fixes: 5762ab71eb24 ("spi: Add support for Armada 3700 SPI Controller")
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> >diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c
> >index e89da0af45d2..5c7508dc6f0b 100644
> >--- a/drivers/spi/spi-armada-3700.c
> >+++ b/drivers/spi/spi-armada-3700.c
> >@@ -108,7 +108,7 @@ struct a3700_spi {
> > 	struct spi_master *master;
> > 	void __iomem *base;
> > 	struct clk *clk;
> >-	unsigned int irq;
> >+	int irq;
> > 	unsigned int flags;
> > 	bool xmit_data;
> > 	const u8 *tx_buf;
> >
> 
> It is already fixed, see https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=f6f0083cca66e673cca6fa26b52b107b5570081d
> 

Ah good.

> And no we cannot just replace the type of the irq field like this
> because devm_request_irq/request_irq require an unsigned int as
> "irq" parameter.

That's immaterial, my patch works fine.  There are other place that use
a signed IRQ like the irq handler and platform_get_irq() which I
mentioned in my patch description.  It's never going to be perfectly
consistent.

But this is actually important that you thought it mattered.  Is there
something that prints a warning?  These irq signedness bugs are some of
the most common recurring bugs I see so it would help to figure out
why people are doing this...

regards,
dan carpenter

Re: [patch 1/3] spi: armada-3700: Signedness bug in probe

[Romain Perier]

Hello,

Le 16/12/2016 à 10:56, Dan Carpenter a écrit :
> On Fri, Dec 16, 2016 at 10:36:29AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 16/12/2016 à 10:33, Dan Carpenter a écrit :
>>> We need "irq" to be an int because platform_get_irq() returns negative
>>> error codes.
>>>
>>> Fixes: 5762ab71eb24 ("spi: Add support for Armada 3700 SPI Controller")
>>> Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>>
>>> diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c
>>> index e89da0af45d2..5c7508dc6f0b 100644
>>> --- a/drivers/spi/spi-armada-3700.c
>>> +++ b/drivers/spi/spi-armada-3700.c
>>> @@ -108,7 +108,7 @@ struct a3700_spi {
>>> 	struct spi_master *master;
>>> 	void __iomem *base;
>>> 	struct clk *clk;
>>> -	unsigned int irq;
>>> +	int irq;
>>> 	unsigned int flags;
>>> 	bool xmit_data;
>>> 	const u8 *tx_buf;
>>>
>>
>> It is already fixed, see https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=f6f0083cca66e673cca6fa26b52b107b5570081d
>>
>
> Ah good.
>
>> And no we cannot just replace the type of the irq field like this
>> because devm_request_irq/request_irq require an unsigned int as
>> "irq" parameter.
>
> That's immaterial, my patch works fine.  There are other place that use
> a signed IRQ like the irq handler and platform_get_irq() which I
> mentioned in my patch description.  It's never going to be perfectly
> consistent.
>
> But this is actually important that you thought it mattered.  Is there
> something that prints a warning?  These irq signedness bugs are some of
> the most common recurring bugs I see so it would help to figure out
> why people are doing this...

Well, Yes technically it is correct as we exclude the case where 
spi->irq is < 0, so even if you pass an int as request_irq is will be 
always positive. That's just more consistent with API of request_irq, 
nothing more.

Originally, that was a mistake from me (to use an unsigned int with the 
return value returned by platform_get_irq).

Thanks for your feedback,

Romain
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/3] spi: armada-3700: Signedness bug in probe

[Dan Carpenter]

We really should make it int consistently through out.  But it doesn't
matter because request_irq() rarely fails and static checkers are good
at catching the bugs...

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html