Re: [PATCH 2/2] mtd: core: Don't fail mtd_device_parse_register() if OTP is unsupported

[Aapo Vienamo]

Hi Michael

On Mon, Mar 11, 2024 at 03:38:17PM +0100, Michael Walle wrote:
> On Thu Mar 7, 2024 at 2:04 PM CET, Aapo Vienamo wrote:
> > Handle the case where -EOPNOTSUPP is returned from OTP driver.
> > +	/*
> > +	 * Don't abort MTD init if OTP functionality is unsupported. The
> > +	 * cleanup of the OTP init is contained within mtd_otp_nvmem_add().
> > +	 * Omitting goto out here is safe since the cleanup code there
> > +	 * should be no-ops.
> > +	 */
> 
> Only if that's true for both the factory and user OTP area.

I'm not sure I follow. I'm not seeing a path in mtd_otp_nvmem_add()
that would result in either mtd->otp_user_nvmem or mtd->otp_factor_nvmem
needing to be cleaned up by the caller, if an error is returned, if
that's what you are referring to.

> Also, you'll print an error message for EOPNOTSUPP, although that is
> not really an error. Is that intended? 

Well, when we hit this, the functionality of the SPI memory itself is
degraded in the sense that the OTP functionality is not available. What
would you suggest?

> 
> >  	ret = mtd_otp_nvmem_add(mtd);
> > -	if (ret)
> > +	if (ret && ret != -EOPNOTSUPP)
> 
> Maybe there is a better way to handle this, like controller
> capabilities instead of putting these EOPNOTSUPP checks
> everywhere? I'm not sure.

Trying to come up with clear semantics for a capabilities flag to solve
this is difficult. The issue is that on the SPI controller side, the
limitation stems from the really strict set of opcodes that are allowed.
For example, we already hit an error with the 0x35 (read configuration
register) not being on the set of allowed opcodes. While this
instruction is used by the OTP code, it's not a strictly OTP specific
operation.

If there was a flag that would signal OTP support, I think it would have
be defined as "the controller supports all operations that are
performed by the OTP code", which sounds brittle. The other way around
would be to have a really fine-grained set of flags that the MTD core
would check, but that feels tedious and error prone as well.

Best,
Aapo