Re: [PATCH v2] mtd: spi-nor: handle unsupported FSR opcodes properly

Tudor.Ambarus
2022-07-19T09:12:52Z

On 6/16/22 13:35, Mika Westerberg wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi,
>
> On Thu, Jun 16, 2022 at 07:40:18AM +0000, Oleksandr Ocheretnyi -X (oocheret - GLOBALLOGIC INC at Cisco) wrote:
>> Hi Mika,
>>
>> > Originally commit 094d3b9 ("mtd: spi-nor: Add USE_FSR flag for
>> n25q*
>> > entries") and following one 8f93826 ("mtd: spi-nor: micron-st:
>> convert
>> > USE_FSR to a manufacturer flag") enabled SPINOR_OP_RDFSR opcode
>> handling
>> > ability, however some controller drivers still cannot handle it
>> properly
>> > in the micron_st_nor_ready() call what breaks some mtd callbacks
>> with
>> > next error logs:
>> >
>> > mtdblock: erase of region [address1, size1] on "BIOS" failed
>> > mtdblock: erase of region [address2, size2] on "BIOS" failed
>> >
>> > The Intel SPI controller does not support low level operations,
>> like
>> > reading the flag status register (FSR). It only exposes a set of
>> high
>> > level operations for software to use. For this reason check the
>> return
>> > value of micron_st_nor_read_fsr() and if the operation was not
>> > supported, use the status register value only. This allows the
>> chip to
>> > work even when attached to Intel SPI controller (there are such
>> systems
>> > out there).
>> >
>>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>> I don't think I signed this off.
>>
>> I thought if I take your case (-EOPNOTSUPP) and update it with
>> (-ENOTSUPP) I need to keep
>>
>> your Sighed-off-by: note as well.
>
> That's not how it typically works. People will give their tag explicitly
> and then you can add those.
>
>> > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com>
>> > Link: [1]https://lore.kernel.org/lkml/YmZUCIE%2FND82BlNh@lahna/
>> > ---
>>
>> What changed between v1 and v2?
>>
>> ​I updated v1 patch taking into account your changes
>> [2]https://lore.kernel.org/linux-mtd/20220506105158.43613-1-mika.wester
>> berg@linux.intel.com to check -EOPNOTSUPP case as well. After I
>> combined both patches I've got v2.
>
> Please put that information after the '---' in the patch.
>
>> And did you take into consideration the comments I gave?
>>
>> ​If you say about keeping -ENOTSUPP as intel driver errorcode - I took
>> it however doubted to use it here because of note about nfs above.
>> There is no problem to restore previous variant with -ENOTSUPP in intel
>> driver errorcode.
>
> Well we would need to get some feedback from SPI-NOR maintainers. I
> would personally keep using ENOTSUPP to be consistent with the rest of
> the code in SPI-NOR code (or convert it to use EOPNOTSUPP everywhere)

SPI NOR does not return -ENOTSUPP, but SPI MEM does. Let's use EOPNOTSUPP
in SPI NOR and verify if we can do a patch to s/ENOTSUPP/EOPNOTSUPP in SPI MEM.

> but it is not up to me ;-)

>
> For Intel driver it is fine to use either (whetever the decision of
> SPI-NOR maintainers' is).