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

Handle the case where -EOPNOTSUPP is returned from OTP driver.

This addresses an issue that occurs with the Intel SPI flash controller,
which has a limited supported opcode set. Whilst the OTP functionality
is not available due to this restriction, other parts of the MTD
functionality of the device are intact. This change allows the driver
to gracefully handle the restriction by allowing the supported
functionality to remain available instead of failing the probe
altogether.

Signed-off-by: Aapo Vienamo <aapo.vienamo@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/mtd/mtdcore.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

On Thu Mar 7, 2024 at 2:04 PM CET, Aapo Vienamo wrote:
> Handle the case where -EOPNOTSUPP is returned from OTP driver.
>
> This addresses an issue that occurs with the Intel SPI flash controller,
> which has a limited supported opcode set. Whilst the OTP functionality
> is not available due to this restriction, other parts of the MTD
> functionality of the device are intact. This change allows the driver
> to gracefully handle the restriction by allowing the supported
> functionality to remain available instead of failing the probe
> altogether.
>
> Signed-off-by: Aapo Vienamo <aapo.vienamo@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> ---
>  drivers/mtd/mtdcore.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c365c97e7232..1cfc8bb5187d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1054,8 +1054,14 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	/*
> +	 * 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.
Also, you'll print an error message for EOPNOTSUPP, although that is
not really an error. Is that intended? 

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

-michael
>  		goto out;
>  
>  	if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {

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

Hi,

On Mon Mar 11, 2024 at 5:20 PM CET, Aapo Vienamo wrote:
> 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.

Yes you're right, sorry for the noise.
>
> > 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?

But it's not really an error, I mean, we are ignoring that one on
purpose now :) I'd just guard it with "if (ret != -EOPNOTSUPP)".

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

I see. It's just that due to this (very) restricted SPI contoller
all this EOPNOTSUPP handling is creeping into more an more places in
spi-nor core and now mtdcore :)

Anyway, I don't have any better idea right now. So I think this is
fine.

-michael


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

On Wed, Mar 13, 2024 at 10:24:13AM +0100, Michael Walle wrote:
> On Mon Mar 11, 2024 at 5:20 PM CET, Aapo Vienamo wrote:
> > On Mon, Mar 11, 2024 at 03:38:17PM +0100, Michael Walle wrote:
> > > 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?
> 
> But it's not really an error, I mean, we are ignoring that one on
> purpose now :) I'd just guard it with "if (ret != -EOPNOTSUPP)".

To clarify, are you suggesting a modification like this to the code at
the end of mtd_otp_nvmem_add()?

err:
	nvmem_unregister(...);
	if (ret != EOPNOTSUPP)
		return dev_err_probe(...);
	return 0;

Best,
Aapo

Hi,

On Wed Mar 13, 2024 at 2:59 PM CET, Aapo Vienamo wrote:
> On Wed, Mar 13, 2024 at 10:24:13AM +0100, Michael Walle wrote:
> > On Mon Mar 11, 2024 at 5:20 PM CET, Aapo Vienamo wrote:
> > > On Mon, Mar 11, 2024 at 03:38:17PM +0100, Michael Walle wrote:
> > > > 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?
> > 
> > But it's not really an error, I mean, we are ignoring that one on
> > purpose now :) I'd just guard it with "if (ret != -EOPNOTSUPP)".
>
> To clarify, are you suggesting a modification like this to the code at
> the end of mtd_otp_nvmem_add()?
>
> err:
> 	nvmem_unregister(...);
> 	if (ret != EOPNOTSUPP)
> 		return dev_err_probe(...);
> 	return 0;

Yes, either this variant, then you don't need to fix the caller or
return EOPNOTSUPP but just don't print the message.

-michael