spi: armada-3700: Signedness bug in probe
Subsystems (2)
PATCH 1/3 spi: armada-3700: Signedness bug in probe
We need "irq" to be an int because platform_get_irq() returns negative error codes.
drivers/spi/spi-armada-3700.c +1 -1
@@ -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;
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hello,
Quoted 12 lines · 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 <[email protected]>drivers/spi/spi-armada-3700.c +0
-0
drivers/spi/spi-armada-3700.c struct a3700_spi { · line 114
Quoted 8 lines
drivers/spi/spi-armada-3700.c +1
-1@@ -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öf0083cca66e673cca6fa26b52b107b5570081d
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 kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Romain Perier
Hello,
Quoted 6 lines · 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 <[email protected]>
Quoted 17 lines
Quoted 6 lines
drivers/spi/spi-armada-3700.c +0
-0drivers/spi/spi-armada-3700.c struct a3700_spi { · line 114
Quoted 8 lines
drivers/spi/spi-armada-3700.c +1
-1@@ -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öf0083cca66e673cca6fa26b52b107b5570081d
Ah good.
Quoted 3 lines
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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello,
Quoted 39 lines · Dan Carpenter
Quoted 9 lines · Romain Perier
Hello,
Quoted 6 lines · 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 <[email protected]>Quoted 17 lines
Quoted 6 lines
drivers/spi/spi-armada-3700.c +0
-0drivers/spi/spi-armada-3700.c struct a3700_spi { · line 114
Quoted 8 lines
drivers/spi/spi-armada-3700.c +1
-1@@ -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öf0083cca66e673cca6fa26b52b107b5570081d
Ah good.
Quoted 3 lines
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.
Quoted 4 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 9e5f6a38-6177-4026-7930-150c6b281a99-wi1+55ScJUtKEb57/[email protected]
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