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 <[email protected]>
> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
Ha, thank you!
Reviewed-by: Michael Walle <[email protected]>
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, 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 <[email protected]>
>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>
> Ha, thank you!
>
> Reviewed-by: Michael Walle <[email protected]>
>
> 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;
--
Regards,
Pratyush Yadav
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 <[email protected]>
>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>>
>> Ha, thank you!
>>
>> Reviewed-by: Michael Walle <[email protected]>
>>
>> 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?
--
Florian
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 <[email protected]>
>>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>>>
>>> Ha, thank you!
>>>
>>> Reviewed-by: Michael Walle <[email protected]>
>>>
>>> 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 <[email protected]>
--
Regards,
Pratyush Yadav
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 <[email protected]>
>>>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>>>>
>>>> Ha, thank you!
>>>>
>>>> Reviewed-by: Michael Walle <[email protected]>
>>>>
>>>> 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 <[email protected]>
>
--
Florian
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 <[email protected]>
>>>>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>>>>>
>>>>> Ha, thank you!
>>>>>
>>>>> Reviewed-by: Michael Walle <[email protected]>
>>>>>
>>>>> 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
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 <[email protected]>
>>>>>>> Change-Id: I4159811f6c582c4de2143382473d2000b8755872
>>>>>>
>>>>>> Ha, thank you!
>>>>>>
>>>>>> Reviewed-by: Michael Walle <[email protected]>
>>>>>>
>>>>>> 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.
--
Regards,
Pratyush Yadav