| Message ID | 20220615113021.2493586-1-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: microchip-core: fix passing zero to PTR_ERR warning | expand |
On Wed, Jun 15, 2022 at 12:30:22PM +0100, Conor Dooley wrote: > - ret = PTR_ERR(spi->clk); > + ret = !spi->clk ? -ENXIO : PTR_ERR(spi->clk); I think you're looking for PTR_ERR_OR_ZERO() here?
On 15/06/2022 12:40, Mark Brown wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Jun 15, 2022 at 12:30:22PM +0100, Conor Dooley wrote: > >> - ret = PTR_ERR(spi->clk); >> + ret = !spi->clk ? -ENXIO : PTR_ERR(spi->clk); > > I think you're looking for PTR_ERR_OR_ZERO() here? Maybe I don't understand, so let me explain what I think you're suggesting & maybe you can correct me: > - ret = PTR_ERR(spi->clk); > + ret = PTR_ERR_OR_ZERO(spi->clk); But if spi->clk is NULL, this will return 0 from the probe rather than returning an error? If that's not what you meant, lmk Thanks, Conor. > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Jun 15, 2022 at 11:57:37AM +0000, Conor.Dooley@microchip.com wrote: > On 15/06/2022 12:40, Mark Brown wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Jun 15, 2022 at 12:30:22PM +0100, Conor Dooley wrote: > >> - ret = PTR_ERR(spi->clk); > >> + ret = !spi->clk ? -ENXIO : PTR_ERR(spi->clk); > > I think you're looking for PTR_ERR_OR_ZERO() here? > Maybe I don't understand, so let me explain what I think you're > suggesting & maybe you can correct me: > > - ret = PTR_ERR(spi->clk); > > + ret = PTR_ERR_OR_ZERO(spi->clk); > But if spi->clk is NULL, this will return 0 from the probe > rather than returning an error? > If that's not what you meant, lmk Oh, hang on - what error conditions can clk_get() return 0 in? The documentation doesn't mention any...
On 15/06/2022 13:37, Mark Brown wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Jun 15, 2022 at 11:57:37AM +0000, Conor.Dooley@microchip.com wrote: >> On 15/06/2022 12:40, Mark Brown wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> On Wed, Jun 15, 2022 at 12:30:22PM +0100, Conor Dooley wrote: > >>>> - ret = PTR_ERR(spi->clk); >>>> + ret = !spi->clk ? -ENXIO : PTR_ERR(spi->clk); > >>> I think you're looking for PTR_ERR_OR_ZERO() here? > >> Maybe I don't understand, so let me explain what I think you're >> suggesting & maybe you can correct me: > >>> - ret = PTR_ERR(spi->clk); >>> + ret = PTR_ERR_OR_ZERO(spi->clk); > >> But if spi->clk is NULL, this will return 0 from the probe >> rather than returning an error? >> If that's not what you meant, lmk > > Oh, hang on - what error conditions can clk_get() return 0 in? The > documentation doesn't mention any... If !CONFIG_HAVE_CLK, (without which it won't boot on the coreplex) but I don't think I can be sure that CONFIG_HAVE_CLK will /always/ be enabled for other uses of the FPGA.
On Wed, Jun 15, 2022 at 12:42:27PM +0000, Conor.Dooley@microchip.com wrote: > On 15/06/2022 13:37, Mark Brown wrote: > >> But if spi->clk is NULL, this will return 0 from the probe > >> rather than returning an error? > >> If that's not what you meant, lmk > > Oh, hang on - what error conditions can clk_get() return 0 in? The > > documentation doesn't mention any... > If !CONFIG_HAVE_CLK, (without which it won't boot on the coreplex) > but I don't think I can be sure that CONFIG_HAVE_CLK will /always/ > be enabled for other uses of the FPGA. That's not an error, that's returning NULL as a dummy clock. The expectation is that the driver will proceed as though it has a clock.
On 15/06/2022 13:49, Mark Brown wrote: > On Wed, Jun 15, 2022 at 12:42:27PM +0000, Conor.Dooley@microchip.com wrote: >> On 15/06/2022 13:37, Mark Brown wrote: > >>>> But if spi->clk is NULL, this will return 0 from the probe >>>> rather than returning an error? >>>> If that's not what you meant, lmk > >>> Oh, hang on - what error conditions can clk_get() return 0 in? The >>> documentation doesn't mention any... > >> If !CONFIG_HAVE_CLK, (without which it won't boot on the coreplex) >> but I don't think I can be sure that CONFIG_HAVE_CLK will /always/ >> be enabled for other uses of the FPGA. > > That's not an error, that's returning NULL as a dummy clock. The > expectation is that the driver will proceed as though it has a clock. Ahh, cool. I guess I'll just drop the null check entirely so & guard against a div zero when I actually go to use the clock. Thanks! Conor
diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c index 5b2aee30fa04..d44663cca071 100644 --- a/drivers/spi/spi-microchip-core.c +++ b/drivers/spi/spi-microchip-core.c @@ -554,7 +554,7 @@ static int mchp_corespi_probe(struct platform_device *pdev) spi->clk = devm_clk_get(&pdev->dev, NULL); if (!spi->clk || IS_ERR(spi->clk)) { - ret = PTR_ERR(spi->clk); + ret = !spi->clk ? -ENXIO : PTR_ERR(spi->clk); dev_err(&pdev->dev, "could not get clk: %d\n", ret); goto error_release_master; }
It is possible that the error case for devm_clk_get() returns NULL, in which case zero will be passed to PTR_ERR() as shown by the Smatch static checker warning: drivers/spi/spi-microchip-core.c:557 mchp_corespi_probe() warn: passing zero to 'PTR_ERR' Fix the warning by explicitly returning an error if devm_clk_get() returns NULL. Fixes: 9ac8d17694b6 ("spi: add support for microchip fpga spi controllers") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Link: https://lore.kernel.org/linux-spi/20220615091633.GI2168@kadam/ Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- drivers/spi/spi-microchip-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)