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

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
>>
>>      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@xxxxxxxxx>
>>    > 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@xxxxxxxxxxxxxxx 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).