Re: [bug report] spi: add support for microchip fpga spi controllers

[<Conor.Dooley@xxxxxxxxxxxxx>]

On 15/06/2022 09:09, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Conor Dooley,
> 
> The patch 9ac8d17694b6: "spi: add support for microchip fpga spi
> controllers" from Jun 7, 2022, leads to the following Smatch static
> checker warning:
> 
>          drivers/spi/spi-microchip-core.c:557 mchp_corespi_probe()
>          warn: passing zero to 'PTR_ERR'
> 
> drivers/spi/spi-microchip-core.c
>      506 static int mchp_corespi_probe(struct platform_device *pdev)
>      507 {
>      508         struct spi_master *master;
>      509         struct mchp_corespi *spi;
>      510         struct resource *res;
>      511         u32 num_cs;
>      512         int ret = 0;
>      513
>      514         master = spi_alloc_master(&pdev->dev, sizeof(*spi));
>      515         if (!master)
>      516                 return dev_err_probe(&pdev->dev, -ENOMEM,
>      517                                      "unable to allocate master for SPI controller\n");
>      518
>      519         platform_set_drvdata(pdev, master);
>      520
>      521         if (of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs))
>      522                 num_cs = MAX_CS;
>      523
>      524         master->num_chipselect = num_cs;
>      525         master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
>      526         master->setup = mchp_corespi_setup;
>      527         master->bits_per_word_mask = SPI_BPW_MASK(8);
>      528         master->transfer_one = mchp_corespi_transfer_one;
>      529         master->prepare_message = mchp_corespi_prepare_message;
>      530         master->set_cs = mchp_corespi_set_cs;
>      531         master->dev.of_node = pdev->dev.of_node;
>      532
>      533         spi = spi_master_get_devdata(master);
>      534
>      535         spi->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>      536         if (IS_ERR(spi->regs)) {
>      537                 ret = PTR_ERR(spi->regs);
>      538                 goto error_release_master;
>      539         }
>      540
>      541         spi->irq = platform_get_irq(pdev, 0);
>      542         if (spi->irq <= 0) {
>      543                 dev_err(&pdev->dev, "invalid IRQ %d for SPI controller\n", spi->irq);
>      544                 ret = spi->irq;
>      545                 goto error_release_master;
>      546         }
>      547
>      548         ret = devm_request_irq(&pdev->dev, spi->irq, mchp_corespi_interrupt,
>      549                                IRQF_SHARED, dev_name(&pdev->dev), master);
>      550         if (ret) {
>      551                 dev_err(&pdev->dev, "could not request irq: %d\n", ret);
>      552                 goto error_release_master;
>      553         }
>      554
>      555         spi->clk = devm_clk_get(&pdev->dev, NULL);
>      556         if (!spi->clk || IS_ERR(spi->clk)) {
>                       ^^^^^^^^
> NULL
> 
> --> 557                 ret = PTR_ERR(spi->clk);
> 
> ret is 0/success.
> 
> Normally when functions like this return NULL, you're supposed to just
> accept the NULL and add tests for it to avoid NULL related bugs.  In
> this driver if spi->clk is NULL then it leads to spi_hz == 0 which leads
> to a divide by zero bug.  So it's not clear which way to go on this?
> Fix the error code or add more checks for NULL?

Am I being dumb here, or should the null check just be removed like
every other driver? As in, devm_clk_get will only return a valid
clk or an IS_ERR() condition.

The correct solution seems to me to be remove the !spi->clk check?
Thanks for the report!
Conor.

> 
>      558                 dev_err(&pdev->dev, "could not get clk: %d\n", ret);
>      559                 goto error_release_master;
>      560         }
>      561
>      562         ret = clk_prepare_enable(spi->clk);
>      563         if (ret) {
>      564                 dev_err(&pdev->dev, "failed to enable clock\n");
>      565                 goto error_release_master;
>      566         }
>      567
>      568         mchp_corespi_init(master, spi);
>      569
>      570         ret = devm_spi_register_master(&pdev->dev, master);
>      571         if (ret) {
>      572                 dev_err(&pdev->dev,
>      573                         "unable to register master for SPI controller\n");
>      574                 goto error_release_hardware;
>      575         }
>      576
>      577         dev_info(&pdev->dev, "Registered SPI controller %d\n", master->bus_num);
>      578
>      579         return 0;
>      580
>      581 error_release_hardware:
>      582         mchp_corespi_disable(spi);
>      583         clk_disable_unprepare(spi->clk);
>      584 error_release_master:
>      585         spi_master_put(master);
>      586
>      587         return ret;
>      588 }
> 
> regards,
> dan carpenter