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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux