| Message ID | 20240313171050.3505620-1-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: Fix error code checking in spi_mem_exec_op() | expand |
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? -michael
On Wed, Mar 13, 2024 at 06:33:43PM +0100, Michael Walle wrote: > FWIW in next, there is commit > e63aef9c9121e ("spi: spi-mem: add statistics support to ->exec_op() calls") > that probably will conflict with this one. Indeed, this doesn't apply - please fix and resend.
On 3/13/24 10:40, Mark Brown wrote: > On Wed, Mar 13, 2024 at 06:33:43PM +0100, Michael Walle wrote: > >> FWIW in next, there is commit >> e63aef9c9121e ("spi: spi-mem: add statistics support to ->exec_op() calls") >> that probably will conflict with this one. > > Indeed, this doesn't apply - please fix and resend. Will do, and will also strip the Change-Id tag that sneaked through.
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;
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?
On 3/13/24 10:40, Mark Brown wrote: > On Wed, Mar 13, 2024 at 06:33:43PM +0100, Michael Walle wrote: > >> FWIW in next, there is commit >> e63aef9c9121e ("spi: spi-mem: add statistics support to ->exec_op() calls") >> that probably will conflict with this one. > > Indeed, this doesn't apply - please fix and resend. But this is affecting v6.8 this would have to be fast tracked as a bug fix, do you still want me to be based off for-next?
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. In this case we should return ret when: ret is 0 OR when ret is not -EOPNOTSUPP or -ENOTSUPP. So if we get either of the two we _won't_ return and continue forward. From looking at just this, spi_mem_exec_op() only returns -EOPNOTSUPP so far since it has: if (!spi_mem_internal_supports_op(mem, op)) return -EOPNOTSUPP; But then looking further, it has: ret = spi_sync(mem->spi, &msg); if (ret) return ret; spi_sync() can return -ENOTSUPP if it goes via __spi_async(). I suppose we would need to fix that if we want consistent return codes. But that isn't a problem this patch should fix. So with the merge conflict fixed up, Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
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. > > In this case we should return ret when: > > ret is 0 > OR > when ret is not -EOPNOTSUPP or -ENOTSUPP. > > So if we get either of the two we _won't_ return and continue forward. > > From looking at just this, spi_mem_exec_op() only returns -EOPNOTSUPP so > far since it has: > > if (!spi_mem_internal_supports_op(mem, op)) > return -EOPNOTSUPP; > > But then looking further, it has: > > ret = spi_sync(mem->spi, &msg); > > if (ret) > return ret; > > spi_sync() can return -ENOTSUPP if it goes via __spi_async(). I suppose > we would need to fix that if we want consistent return codes. But that > isn't a problem this patch should fix. So with the merge conflict fixed > up, Thanks, although as I replied to Mark in the other branch of the thread, since this is a regression affecting v6.8, would not we want it to be fast tracked, and not based upon for-next? > > Reviewed-by: Pratyush Yadav <pratyush@kernel.org> >
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.
On Wed, Mar 13 2024, Florian Fainelli wrote: > 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. No it won't. This is what confused me in my earlier reply as well. If ret is either of -ENOTSUPP or -EOPNOTSUPP, the expression (ret != -ENOTSUPP && ret != -EOPNOTSUPP) becomes false (along with !ret also being false). In that case, it will _not_ go in the if statement, and not call spi_mem_add_op_stats(). Instead, it will go via the normal SPI path and that path would do the accounting based on error or success. > > 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. The only questionable thing I see in spi_mem_add_op_stats() is that it increments bytes_{rx,tx} even in case of failure. It mimics what spi_statistics_add_transfer_stats() does but perhaps that also is wrong.
On Wed, Mar 13, 2024 at 12:03:25PM -0700, Florian Fainelli wrote: > On 3/13/24 10:40, Mark Brown wrote: > > Indeed, this doesn't apply - please fix and resend. > But this is affecting v6.8 this would have to be fast tracked as a bug fix, > do you still want me to be based off for-next? We're in the merge window, nothing is getting applied to v6.8 any more and Linus has already merged the v6.9 changes. You need to be testing against the -rcs to get fixes into the release.
On Wed, 13 Mar 2024 10:10:49 -0700, 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 > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: Fix error code checking in spi_mem_exec_op() commit: 29895ce18311ddd702973ddb3a6c687db663e0fb All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 2dc8ceb85374..10b5fa003693 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -339,7 +339,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) * read path) and expect the core to use the regular SPI * interface in other cases. */ - if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) + if (!ret || (ret != -ENOTSUPP && ret != -EOPNOTSUPP)) return ret; }
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 --- drivers/spi/spi-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)