| Message ID | 20161213102812.9029-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f6f0083cca66e673cca6fa26b52b107b5570081d |
| Headers | show |
Hello, Le 13/12/2016 à 11:28, Colin King a écrit : > From: Colin Ian King <colin.king@canonical.com> > > spi->irq is an unsigned integer hence the check if status is less than > zero has no effect. Fix this by replacing spi->irq with an int irq > so the less than zero compare will correctly detect errors. > > Issue found with static analysis with CoverityScan, CID1388567 > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/spi/spi-armada-3700.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c > index e89da0a..4e92178 100644 > --- a/drivers/spi/spi-armada-3700.c > +++ b/drivers/spi/spi-armada-3700.c > @@ -800,7 +800,7 @@ static int a3700_spi_probe(struct platform_device *pdev) > struct spi_master *master; > struct a3700_spi *spi; > u32 num_cs = 0; > - int ret = 0; > + int irq, ret = 0; > > master = spi_alloc_master(dev, sizeof(*spi)); > if (!master) { > @@ -846,12 +846,13 @@ static int a3700_spi_probe(struct platform_device *pdev) > goto error; > } > > - spi->irq = platform_get_irq(pdev, 0); > - if (spi->irq < 0) { > - dev_err(dev, "could not get irq: %d\n", spi->irq); > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "could not get irq: %d\n", irq); > ret = -ENXIO; > goto error; > } > + spi->irq = irq; > > init_completion(&spi->done); > > Why don't you change only the type of "irq" in struct a3700_spi ? Thanks, Romain -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/12/16 10:55, Romain Perier wrote: > Hello, > > Le 13/12/2016 à 11:28, Colin King a écrit : >> From: Colin Ian King <colin.king@canonical.com> >> >> spi->irq is an unsigned integer hence the check if status is less than >> zero has no effect. Fix this by replacing spi->irq with an int irq >> so the less than zero compare will correctly detect errors. >> >> Issue found with static analysis with CoverityScan, CID1388567 >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/spi/spi-armada-3700.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/spi-armada-3700.c >> b/drivers/spi/spi-armada-3700.c >> index e89da0a..4e92178 100644 >> --- a/drivers/spi/spi-armada-3700.c >> +++ b/drivers/spi/spi-armada-3700.c >> @@ -800,7 +800,7 @@ static int a3700_spi_probe(struct platform_device >> *pdev) >> struct spi_master *master; >> struct a3700_spi *spi; >> u32 num_cs = 0; >> - int ret = 0; >> + int irq, ret = 0; >> >> master = spi_alloc_master(dev, sizeof(*spi)); >> if (!master) { >> @@ -846,12 +846,13 @@ static int a3700_spi_probe(struct >> platform_device *pdev) >> goto error; >> } >> >> - spi->irq = platform_get_irq(pdev, 0); >> - if (spi->irq < 0) { >> - dev_err(dev, "could not get irq: %d\n", spi->irq); >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(dev, "could not get irq: %d\n", irq); >> ret = -ENXIO; >> goto error; >> } >> + spi->irq = irq; >> >> init_completion(&spi->done); >> >> > > Why don't you change only the type of "irq" in struct a3700_spi ? > > Thanks, > Romain ..because I've observed that a lot of driver seems to use a unsigned int for the irq in their driver specific structs and I also didn't want to modify this to an int in case it some subtle signed/unsigned logic somewhere else that I was not aware of. Just seemed like the least risky fix to me. Colin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Le 13/12/2016 à 11:55, Romain Perier a écrit : >> > > Why don't you change only the type of "irq" in struct a3700_spi ? > > Thanks, > Romain In fact, it cannot work with devmem_request_irq/request_irq which require an unsigned int as second argument... So it's okay for me. Acked-by: Romain Perier <romain.perier@free-electrons.com> -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c index e89da0a..4e92178 100644 --- a/drivers/spi/spi-armada-3700.c +++ b/drivers/spi/spi-armada-3700.c @@ -800,7 +800,7 @@ static int a3700_spi_probe(struct platform_device *pdev) struct spi_master *master; struct a3700_spi *spi; u32 num_cs = 0; - int ret = 0; + int irq, ret = 0; master = spi_alloc_master(dev, sizeof(*spi)); if (!master) { @@ -846,12 +846,13 @@ static int a3700_spi_probe(struct platform_device *pdev) goto error; } - spi->irq = platform_get_irq(pdev, 0); - if (spi->irq < 0) { - dev_err(dev, "could not get irq: %d\n", spi->irq); + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "could not get irq: %d\n", irq); ret = -ENXIO; goto error; } + spi->irq = irq; init_completion(&spi->done);