Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>]

Hi all,

+Marek

Le 29/11/2016 à 02:32, Florian Fainelli a écrit :
> On 10/14/2016 06:17 AM, Cyrille Pitchen wrote:
>> Le 13/10/2016 à 23:15, Kamal Dasu a écrit :
>>> On Mon, Oct 10, 2016 at 4:29 AM, Cyrille Pitchen
>>> <cyrille.pitchen@xxxxxxxxx> wrote:
>>>> Hi all,
>>>>
>>>>
>>>> Le 10/10/2016 à 10:04, Florian Fainelli a écrit :
>>>>> On 08/24/2016 03:04 PM, Kamal Dasu wrote:
>>>>>> In m25p80_read() even though spi_flash_read() is supported
>>>>>> by some drivers, under certain circumstances like unaligned
>>>>>> buffer, address or address range limitations on certain SoCs
>>>>>> let it fallback to core spi reads. Such drivers are expected
>>>>>> to return -EAGAIN so that the m25p80_read() uses standard
>>>>>> spi transfer.
>>>>>>
>>>>>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>>>>>
>>>>> MTD folks, any comments on this?
>>>>>
>>>>>> ---
>>>>>>  drivers/mtd/devices/m25p80.c | 11 +++++++++--
>>>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>>>> index 9cf7fcd..77c2d2c 100644
>>>>>> --- a/drivers/mtd/devices/m25p80.c
>>>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>>>> @@ -155,9 +155,16 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>>              msg.data_nbits = m25p80_rx_nbits(nor);
>>>>>>
>>>>>>              ret = spi_flash_read(spi, &msg);
>>>>>> -            if (ret < 0)
>>>>>> +
>>>>>> +            if (ret >= 0)
>>>>>> +                    return msg.retlen;
>>>>>> +
>>>>>> +            /*
>>>>>> +             * some spi master drivers might need to fallback to
>>>>>> +             * normal spi transfer
>>>>>> +             */
>>>>>> +            if (ret != -EAGAIN)
>>>> I just wonder whether EINVAL would be a better choice.
>>>
>>>  spi_flash_read calls the down stream controller driver with all
>>> params addresses however  accelerated transfer is not possible by the
>>> controller due to alignment issues, it needs to indicate to m25p call
>>> to try the normal transfer, hence use of EAGAIN seemed appropriate.
>>>
>>>
>>
>> Yes, I think I've understood the purpose of this patch. In the example you
>> gave, the actual implementation of spi_flash_read() works fine with aligned
>> addresses but doesn't support unaligned addresses. Hence, such unaligned
>> addresses are invalid argument for spi_flash_read() and we should fall back
>> to the legacy implementation of m25p80_read().
>>
>> My point is just that EINVAL clearly tells that the SPI controller driver
>> implementation of spi_flash_read() doesn't support the given input
>> parameters, here an unaligned address, whereas EAGAIN suggests that some
>> hardware resource is temporarily unavailable and we could call spi_flash_read()
>> again later. However, in this case, spi_flash_read() would still fail even if
>> called later.
>>
>> That's why I've suggested EINVAL might have been a better choice than EAGAIN,
>> but honestly it's not a big deal, only a detail. So if most people prefer to
>> keep EAGAIN, I'm perfectly fine with it! :)
>>
>> I don't want my comment to delay the integration of this patch.
> 
> So what are we going to do now, should Kamal resubmit and
> s/EGAIN/EINVAL/ or is EAGAIN good enough? If EINVAL needs to be used,
> which I agree with you seems like a valid error code to return, this
> does imply changing the Broadcom QSPI driver though... so we have to
> coordinate the two changes to be merged through the same cycle to avoid
> regressions.
> 

Please replace the EAGAIN error code by EINVAL then I agree to merge this
patch in the github spi-nor tree.

I know the EAGAIN error code has already been introduced in spi-bcm-qspi.c
through the spi tree but since currently m25p280.c handles neither EAGAIN
nor EINVAL as the returned code of spi_flash_read(), I guess no regression
will be introduced. The feature will work as expected once both the m25p80.c
and spi-bcm-qspi.c use the very same error code.

Marek, any comment?

Best regards,

Cyrille

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html