Messages in this thread | | | Date | Wed, 13 Mar 2024 13:06:35 -0700 | Subject | Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op() | From | Florian Fainelli <> |
| |
On 3/13/24 12:34, Florian Fainelli wrote: > On 3/13/24 12:29, Pratyush Yadav wrote: >> On Wed, Mar 13 2024, Florian Fainelli wrote: >> >>> On 3/13/24 11:28, Pratyush Yadav wrote: >>>> On Wed, Mar 13 2024, Michael Walle wrote: >>>> >>>>> On Wed Mar 13, 2024 at 6:10 PM CET, Florian Fainelli wrote: >>>>>> After commit cff49d58f57e ("spi: Unify error codes by replacing >>>>>> -ENOTSUPP with >>>>>> -EOPNOTSUPP"), our SPI NOR flashes would stop probing with the >>>>>> following >>>>>> visible in the kernel log: >>>>>> >>>>>> [ 2.196300] brcmstb_qspi f0440920.qspi: using bspi-mspi mode >>>>>> [ 2.210295] spi-nor: probe of spi1.0 failed with error -95 >>>>>> >>>>>> It turns out that the check in spi_mem_exec_op() was changed to check >>>>>> for -ENOTSUPP (old error code) or -EOPNOTSUPP (new error code), >>>>>> but this >>>>>> means that for drivers that were converted, the second condition >>>>>> is now >>>>>> true, and we stop falling through like we used to. Fix the error to >>>>>> check for neither error being neither -ENOTSUPP *nor* -EOPNOTSUPP. >>>>>> >>>>>> Fixes: cff49d58f57e ("spi: Unify error codes by replacing >>>>>> -ENOTSUPP with -EOPNOTSUPP") >>>>>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> >>>>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872 >>>>> >>>>> Ha, thank you! >>>>> >>>>> Reviewed-by: Michael Walle <mwalle@kernel.org> >>>>> >>>>> FWIW in next, there is commit >>>>> e63aef9c9121e ("spi: spi-mem: add statistics support to ->exec_op() >>>>> calls") >>>>> that probably will conflict with this one. >>>>> >>>>> Also, - not for this patch - but with that logic, spi_mem_exec_op() >>>>> might return EOPNOTSUPP *or* ENOTSUPP, even for drivers which might >>>>> still return ENOTSUPP, because there is one condition in >>>>> spi_mem_exec_op() which will always return EOPNOTSUPP. That is >>>>> somewhat confusing, no? >>>> I agree. I suppose it would be better to do: >>>> if (!ret) >>>> return 0; >>>> if (ret == -ENOTSUPP || ret == -EOPNOTSUPP) >>>> return -EOPNOTSUPP; >>>> >>> >>> But with e63aef9c9121e ("spi: spi-mem: add statistics support to >>> ->exec_op() >>> calls") applied, would not that mean duplicating the statistics >>> gathering, or >>> were the statistics gathering only intended for when ret == 0? >> >> Hmm, I didn't properly understand this. Ignore my suggestion. Your patch >> does the right thing. > > What I meant is that e63aef9c9121e will increment statistics not just > when we return 0 from ctlr->mem_ops->exec_op, but also if we return > -ENOTSUPP or -EOPNOTSUPP, and I am not sure if this is exactly what is > intended. But this is somewhat orthogonal.
It looks like the handling of a non-zero return code will fall either in the -ETIMEDOUT category, or in the general category of an error. I suppose there is a question whether a operation that could not be supported should fall in the "error" category. -- Florian
[unhandled content-type:application/pkcs7-signature] | |