Make MTD core tolerate SPI controllers without OTP support and report
an error from MTD core if OTP initialization fails early.
These changes address the issue that occurs when an OTP capable
SPI NOR device is initialized with the Intel SPI controller. The limited
supported opcode set of the SPI controller leads to failure in the OTP
initialization, which in turn fails the probe for the MTD device. By
allowing the MTD core to tolerate unsupported OTP, the rest of the MTD
functionality remains intact even if OTP initialization is unsupported.
Aapo Vienamo (2):
mtd: core: Report error if first mtd_otp_size() call fails in
mtd_otp_nvmem_add()
mtd: core: Don't fail mtd_device_parse_register() if OTP is
unsupported
drivers/mtd/mtdcore.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
--
2.41.0
Jump to the error reporting code in mtd_otp_nvmem_add() if the
mtd_otp_size() call fails. Without this fix, the error is not logged.
Signed-off-by: Aapo Vienamo <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
---
drivers/mtd/mtdcore.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 5887feb347a4..c365c97e7232 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -956,8 +956,10 @@ static int mtd_otp_nvmem_add(struct mtd_info *mtd)
if (mtd->_get_user_prot_info && mtd->_read_user_prot_reg) {
size = mtd_otp_size(mtd, true);
- if (size < 0)
- return size;
+ if (size < 0) {
+ err = size;
+ goto err;
+ }
if (size > 0) {
nvmem = mtd_otp_nvmem_register(mtd, "user-otp", size,
--
2.41.0
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 <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
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.
+ */
ret = mtd_otp_nvmem_add(mtd);
- if (ret)
+ if (ret && ret != -EOPNOTSUPP)
goto out;
if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
--
2.41.0
On Thu Mar 7, 2024 at 2:04 PM CET, Aapo Vienamo wrote:
> Jump to the error reporting code in mtd_otp_nvmem_add() if the
> mtd_otp_size() call fails. Without this fix, the error is not logged.
>
> Signed-off-by: Aapo Vienamo <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
Not sure this qualifies as a Fixes patch, I'll leave that to the MTD
mainterainer.
Anyways,
Reviewed-by: Michael Walle <[email protected]>
-michael
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 <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
>
> ---
> 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.
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
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