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

[<Conor.Dooley@microchip.com>]

On 15/06/2022 09:40, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 15, 2022 at 08:33:35AM +0000, Conor.Dooley@microchip.com wrote:
>>>       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.
> 
> It can return NULL if CONFIG_HAVE_CLK is disabled.  I don't know the
> hardware or if that CONFIG_ is essential for booting.

Ehh I guess it is /possible/ that CONFIG_HAVE_CLK could be off
if someone is accessing the FPGA from another device.
In that case, neither option really particularly appeals to me.
Just return -ENODEV I guess?

> 
>>
>> The correct solution seems to me to be remove the !spi->clk check?
> 
> That's the normal solution, yes.  But if you do that, then please add a
> check to prevent the divide by zero:
> `grep -w clk drivers/spi/spi-microchip-core.c`
> 
> regards,
> dan carpenter
>