[PATCH] spi: Fix error code checking in spi_mem_exec_op()

[PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

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(-)

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;
 	}
 
-- 
2.34.1

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Michael Walle]

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

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

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.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Pratyush Yadav]

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;

-- 
Regards,
Pratyush Yadav

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

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?
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

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?
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Pratyush Yadav]

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>

-- 
Regards,
Pratyush Yadav

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 3509 bytes --]

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>
> 

-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Fix error code checking in spi_mem_exec_op()

[Mark Brown]

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