| Message ID | 20190917085627.4562-1-mail@aurabindo.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: sifive: check return value for platform_get_resource() | expand |
On Tue, 17 Sep 2019, Aurabindo Jayamohanan wrote: > platform_get_resource() may return NULL. If it is so, return -ENXIO > > Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> Reviewed-by: Paul Walmsley <paul.walmsley@sifive.com> - Paul
On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan <mail@aurabindo.in> wrote: > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> > --- > drivers/spi/spi-sifive.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > index 93ec2c6cdbfd..67485067a694 100644 > --- a/drivers/spi/spi-sifive.c > +++ b/drivers/spi/spi-sifive.c > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, master); > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(&pdev->dev, "no IOMEM resource found\n"); > + ret = -ENXIO; > + goto put_master; > + } Seems unnecessary, the devm_ioremap_resource() already validated if the resource is available.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, September 17, 2019 2:53 PM, Baolin Wang <baolin.wang@linaro.org> wrote: > On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan mail@aurabindo.in wrote: > > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > > > Signed-off-by: Aurabindo Jayamohanan mail@aurabindo.in > > > > ------------------------------------------------------- > > > > drivers/spi/spi-sifive.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > > index 93ec2c6cdbfd..67485067a694 100644 > > --- a/drivers/spi/spi-sifive.c > > +++ b/drivers/spi/spi-sifive.c > > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, master); > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > - if (!res) { > > > > > > - dev_err(&pdev->dev, "no IOMEM resource found\\n"); > > > > > > - ret = -ENXIO; > > > > > > - goto put_master; > > > > > > - } > > > > > > Seems unnecessary, the devm_ioremap_resource() already validated if > the resource is available. > Okay, thanks for the headsup
On Tue, 17 Sep 2019, Baolin Wang wrote: > On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan <mail@aurabindo.in> wrote: > > > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > > > Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> > > --- > > drivers/spi/spi-sifive.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > > index 93ec2c6cdbfd..67485067a694 100644 > > --- a/drivers/spi/spi-sifive.c > > +++ b/drivers/spi/spi-sifive.c > > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, master); > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) { > > + dev_err(&pdev->dev, "no IOMEM resource found\n"); > > + ret = -ENXIO; > > + goto put_master; > > + } > > Seems unnecessary, the devm_ioremap_resource() already validated if > the resource is available. Just doublechecked lib/devres.c and I agree with you. Aurobindo, is this a patch for a real problem that you've encountered? - Paul
Hi Paul, On Tue, Sep 17, 2019 at 2:16 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: > On Tue, 17 Sep 2019, Baolin Wang wrote: > > On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan <mail@aurabindo.in> wrote: > > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > > > > > Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> > > > --- > > > drivers/spi/spi-sifive.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > > > index 93ec2c6cdbfd..67485067a694 100644 > > > --- a/drivers/spi/spi-sifive.c > > > +++ b/drivers/spi/spi-sifive.c > > > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > > > platform_set_drvdata(pdev, master); > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!res) { > > > + dev_err(&pdev->dev, "no IOMEM resource found\n"); > > > + ret = -ENXIO; > > > + goto put_master; > > > + } > > > > Seems unnecessary, the devm_ioremap_resource() already validated if > > the resource is available. > > Just doublechecked lib/devres.c and I agree with you. > > Aurobindo, is this a patch for a real problem that you've encountered? Probably not. The sequence can be replaced by a single call to devm_platform_ioremap_resource(), which BTW also doesn't check the intermediate result, as that's unneeded. Gr{oetje,eeting}s, Geert
Hi Paul, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, September 17, 2019 5:45 PM, Paul Walmsley <paul.walmsley@sifive.com> wrote: > On Tue, 17 Sep 2019, Baolin Wang wrote: > > > On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan mail@aurabindo.in wrote: > > > > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > > > > > Signed-off-by: Aurabindo Jayamohanan mail@aurabindo.in > > > > > > ------------------------------------------------------- > > > > > > drivers/spi/spi-sifive.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > > > index 93ec2c6cdbfd..67485067a694 100644 > > > --- a/drivers/spi/spi-sifive.c > > > +++ b/drivers/spi/spi-sifive.c > > > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > > > platform_set_drvdata(pdev, master); > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > - if (!res) { > > > > > > > > > - dev_err(&pdev->dev, "no IOMEM resource found\\n"); > > > > > > > > > - ret = -ENXIO; > > > > > > > > > - goto put_master; > > > > > > > > > - } > > > > > > > > > > Seems unnecessary, the devm_ioremap_resource() already validated if > > the resource is available. > > Just doublechecked lib/devres.c and I agree with you. > > Aurobindo, is this a patch for a real problem that you've encountered? > > - Paul Geert is right. I was just breezing through the source and found it odd since I noticed other instances checking for return value. Apparently none of those use the managed api. If you didnt already do Geert's suggestion, here is a patch:
Aurobindo, On Tue, 17 Sep 2019, Aurabindo Jayamohanan wrote: > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Tuesday, September 17, 2019 5:45 PM, Paul Walmsley <paul.walmsley@sifive.com> wrote: > > > On Tue, 17 Sep 2019, Baolin Wang wrote: > > > > > On Tue, 17 Sep 2019 at 17:12, Aurabindo Jayamohanan mail@aurabindo.in wrote: > > > > > > > platform_get_resource() may return NULL. If it is so, return -ENXIO > > > > > > > > Signed-off-by: Aurabindo Jayamohanan mail@aurabindo.in > > > > > > > > ------------------------------------------------------- > > > > > > > > drivers/spi/spi-sifive.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > > > > index 93ec2c6cdbfd..67485067a694 100644 > > > > --- a/drivers/spi/spi-sifive.c > > > > +++ b/drivers/spi/spi-sifive.c > > > > @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, master); > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > - if (!res) { > > > > > > > > > > > > - dev_err(&pdev->dev, "no IOMEM resource found\\n"); > > > > > > > > > > > > - ret = -ENXIO; > > > > > > > > > > > > - goto put_master; > > > > > > > > > > > > - } > > > > > > > > > > > > > > Seems unnecessary, the devm_ioremap_resource() already validated if > > > the resource is available. > > > > Just doublechecked lib/devres.c and I agree with you. > > > > Aurobindo, is this a patch for a real problem that you've encountered? > > > > - Paul > > Geert is right. I was just breezing through the source and found it odd since > I noticed other instances checking for return value. Apparently none of those > use the managed api. > > If you didnt already do Geert's suggestion, Mark Brown is the maintainer, so he'd be the one to apply anything touching these drivers. > here is a patch: Could you please send this as a separate E-mail? > ____ > > spi: sifive: use device managed api to get plaform resource "platform resource" > > calls to devm_ioremap_resource() preceeded by platform_get_resource() "Calls" "preceded" > may be replaced by devm_platform_ioremap_resource(). Please explain why. In this case "removing boilerplate from the code" seems like a reasonable explanation. > > Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> > --- > drivers/spi/spi-sifive.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c > index 93ec2c6cdbfd..c0925aa98aad 100644 > --- a/drivers/spi/spi-sifive.c > +++ b/drivers/spi/spi-sifive.c > @@ -292,7 +292,6 @@ sifive_spi_transfer_one(struct spi_master *master, struct spi_device *device, > static int sifive_spi_probe(struct platform_device *pdev) > { > struct sifive_spi *spi; > - struct resource *res; > int ret, irq, num_cs; > u32 cs_bits, max_bits_per_word; > struct spi_master *master; > @@ -307,8 +306,7 @@ static int sifive_spi_probe(struct platform_device *pdev) > init_completion(&spi->done); > platform_set_drvdata(pdev, master); > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - spi->regs = devm_ioremap_resource(&pdev->dev, res); > + spi->regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(spi->regs)) { > ret = PTR_ERR(spi->regs); > goto put_master; > -- > 2.23.0 > - Paul
I just recalled that YueHaibing already posted a patch to do this: https://lore.kernel.org/linux-riscv/alpine.DEB.2.21.9999.1909041520130.13502@viisi.sifive.com/ - Paul
Hi Paul, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, September 17, 2019 8:51 PM, Paul Walmsley <paul.walmsley@sifive.com> wrote: > > > I just recalled that YueHaibing already posted a patch to do this: > > https://lore.kernel.org/linux-riscv/alpine.DEB.2.21.9999.1909041520130.13502@viisi.sifive.com/ > Ah, sorry for the noise.
diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c index 93ec2c6cdbfd..67485067a694 100644 --- a/drivers/spi/spi-sifive.c +++ b/drivers/spi/spi-sifive.c @@ -308,6 +308,12 @@ static int sifive_spi_probe(struct platform_device *pdev) platform_set_drvdata(pdev, master); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "no IOMEM resource found\n"); + ret = -ENXIO; + goto put_master; + } + spi->regs = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(spi->regs)) { ret = PTR_ERR(spi->regs);
platform_get_resource() may return NULL. If it is so, return -ENXIO Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in> --- drivers/spi/spi-sifive.c | 6 ++++++ 1 file changed, 6 insertions(+)