| Message ID | 20240321095522.12755-1-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mtd: limit OTP NVMEM Cell parse to non Nand devices | expand |
Hi Christian, ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 10:55:13 +0100: > MTD OTP logic is very fragile and can be problematic with some specific > kind of devices. > > NVMEM across the years had various iteration on how Cells could be > declared in DT and MTD OTP probably was left behind and > add_legacy_fixed_of_cells was enabled without thinking of the consequences. > > That option enables NVMEM to scan the provided of_node and treat each > child as a NVMEM Cell, this was to support legacy NVMEM implementation > and don't cause regression. > > This is problematic if we have devices like Nand where the OTP is > triggered by setting a special mode in the flash. In this context real > partitions declared in the Nand node are registered as OTP Cells and > this cause probe fail with -EINVAL error. > > This was never notice due to the fact that till now, no Nand supported > the OTP feature. With commit e87161321a40 ("mtd: rawnand: macronix: OTP > access for MX30LFxG18AC") this changed and coincidentally this Nand is > used on an FritzBox 7530 supported on OpenWrt. > > Alternative and more robust way to declare OTP Cells are already > prossible by using the fixed-layout node or by declaring a child node > with the compatible set to "otp-user" or "otp-factory". > > To fix this and limit any regression with other MTD that makes use of > declaring OTP as direct child of the dev node, disable > add_legacy_fixed_of_cells if we have a node called nand since it's the > standard property name to identify Nand devices attached to a Nand > Controller. You forgot to update the commit log :-) Thanks, Miquèl
On Thu, Mar 21, 2024 at 11:32:56AM +0100, Miquel Raynal wrote: > Hi Christian, > > ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 10:55:13 +0100: > > > MTD OTP logic is very fragile and can be problematic with some specific > > kind of devices. > > > > NVMEM across the years had various iteration on how Cells could be > > declared in DT and MTD OTP probably was left behind and > > add_legacy_fixed_of_cells was enabled without thinking of the consequences. > > > > That option enables NVMEM to scan the provided of_node and treat each > > child as a NVMEM Cell, this was to support legacy NVMEM implementation > > and don't cause regression. > > > > This is problematic if we have devices like Nand where the OTP is > > triggered by setting a special mode in the flash. In this context real > > partitions declared in the Nand node are registered as OTP Cells and > > this cause probe fail with -EINVAL error. > > > > This was never notice due to the fact that till now, no Nand supported > > the OTP feature. With commit e87161321a40 ("mtd: rawnand: macronix: OTP > > access for MX30LFxG18AC") this changed and coincidentally this Nand is > > used on an FritzBox 7530 supported on OpenWrt. > > > > Alternative and more robust way to declare OTP Cells are already > > prossible by using the fixed-layout node or by declaring a child node > > with the compatible set to "otp-user" or "otp-factory". > > > > To fix this and limit any regression with other MTD that makes use of > > declaring OTP as direct child of the dev node, disable > > add_legacy_fixed_of_cells if we have a node called nand since it's the > > standard property name to identify Nand devices attached to a Nand > > Controller. > > You forgot to update the commit log :-) > Ugh... sorry. Ok to resend or I need to wait 24h similar to the rules on net-next?
Hi, ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 11:34:16 +0100: > On Thu, Mar 21, 2024 at 11:32:56AM +0100, Miquel Raynal wrote: > > Hi Christian, > > > > ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 10:55:13 +0100: > > > > > MTD OTP logic is very fragile and can be problematic with some specific > > > kind of devices. > > > > > > NVMEM across the years had various iteration on how Cells could be > > > declared in DT and MTD OTP probably was left behind and > > > add_legacy_fixed_of_cells was enabled without thinking of the consequences. > > > > > > That option enables NVMEM to scan the provided of_node and treat each > > > child as a NVMEM Cell, this was to support legacy NVMEM implementation > > > and don't cause regression. > > > > > > This is problematic if we have devices like Nand where the OTP is > > > triggered by setting a special mode in the flash. In this context real > > > partitions declared in the Nand node are registered as OTP Cells and > > > this cause probe fail with -EINVAL error. > > > > > > This was never notice due to the fact that till now, no Nand supported > > > the OTP feature. With commit e87161321a40 ("mtd: rawnand: macronix: OTP > > > access for MX30LFxG18AC") this changed and coincidentally this Nand is > > > used on an FritzBox 7530 supported on OpenWrt. > > > > > > Alternative and more robust way to declare OTP Cells are already > > > prossible by using the fixed-layout node or by declaring a child node > > > with the compatible set to "otp-user" or "otp-factory". > > > > > > To fix this and limit any regression with other MTD that makes use of > > > declaring OTP as direct child of the dev node, disable > > > add_legacy_fixed_of_cells if we have a node called nand since it's the > > > standard property name to identify Nand devices attached to a Nand > > > Controller. > > > > You forgot to update the commit log :-) > > > > Ugh... sorry. Ok to resend or I need to wait 24h similar to the rules on > net-next? > You can go ahead, but I'll only queue it after -rc1 is out. Thanks, Miquèl
On Thu, Mar 21, 2024 at 02:19:35PM +0100, Miquel Raynal wrote: > Hi, > > ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 11:34:16 +0100: > > > On Thu, Mar 21, 2024 at 11:32:56AM +0100, Miquel Raynal wrote: > > > Hi Christian, > > > > > > ansuelsmth@gmail.com wrote on Thu, 21 Mar 2024 10:55:13 +0100: > > > > > > > MTD OTP logic is very fragile and can be problematic with some specific > > > > kind of devices. > > > > > > > > NVMEM across the years had various iteration on how Cells could be > > > > declared in DT and MTD OTP probably was left behind and > > > > add_legacy_fixed_of_cells was enabled without thinking of the consequences. > > > > > > > > That option enables NVMEM to scan the provided of_node and treat each > > > > child as a NVMEM Cell, this was to support legacy NVMEM implementation > > > > and don't cause regression. > > > > > > > > This is problematic if we have devices like Nand where the OTP is > > > > triggered by setting a special mode in the flash. In this context real > > > > partitions declared in the Nand node are registered as OTP Cells and > > > > this cause probe fail with -EINVAL error. > > > > > > > > This was never notice due to the fact that till now, no Nand supported > > > > the OTP feature. With commit e87161321a40 ("mtd: rawnand: macronix: OTP > > > > access for MX30LFxG18AC") this changed and coincidentally this Nand is > > > > used on an FritzBox 7530 supported on OpenWrt. > > > > > > > > Alternative and more robust way to declare OTP Cells are already > > > > prossible by using the fixed-layout node or by declaring a child node > > > > with the compatible set to "otp-user" or "otp-factory". > > > > > > > > To fix this and limit any regression with other MTD that makes use of > > > > declaring OTP as direct child of the dev node, disable > > > > add_legacy_fixed_of_cells if we have a node called nand since it's the > > > > standard property name to identify Nand devices attached to a Nand > > > > Controller. > > > > > > You forgot to update the commit log :-) > > > > > > > Ugh... sorry. Ok to resend or I need to wait 24h similar to the rules on > > net-next? > > > > You can go ahead, but I'll only queue it after -rc1 is out. > Doing it right now.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 5887feb347a4..0de87bc63840 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -900,7 +900,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, config.name = compatible; config.id = NVMEM_DEVID_AUTO; config.owner = THIS_MODULE; - config.add_legacy_fixed_of_cells = true; + config.add_legacy_fixed_of_cells = !mtd_type_is_nand(mtd); config.type = NVMEM_TYPE_OTP; config.root_only = true; config.ignore_wp = true;
MTD OTP logic is very fragile and can be problematic with some specific kind of devices. NVMEM across the years had various iteration on how Cells could be declared in DT and MTD OTP probably was left behind and add_legacy_fixed_of_cells was enabled without thinking of the consequences. That option enables NVMEM to scan the provided of_node and treat each child as a NVMEM Cell, this was to support legacy NVMEM implementation and don't cause regression. This is problematic if we have devices like Nand where the OTP is triggered by setting a special mode in the flash. In this context real partitions declared in the Nand node are registered as OTP Cells and this cause probe fail with -EINVAL error. This was never notice due to the fact that till now, no Nand supported the OTP feature. With commit e87161321a40 ("mtd: rawnand: macronix: OTP access for MX30LFxG18AC") this changed and coincidentally this Nand is used on an FritzBox 7530 supported on OpenWrt. Alternative and more robust way to declare OTP Cells are already prossible by using the fixed-layout node or by declaring a child node with the compatible set to "otp-user" or "otp-factory". To fix this and limit any regression with other MTD that makes use of declaring OTP as direct child of the dev node, disable add_legacy_fixed_of_cells if we have a node called nand since it's the standard property name to identify Nand devices attached to a Nand Controller. With the following logic, the OTP NVMEM entry is correctly created with no Cells and the MTD Nand is correctly probed and partitions are correctly exposed. Fixes: 2cc3b37f5b6d ("nvmem: add explicit config option to read old syntax fixed OF cells") Cc: <stable@vger.kernel.org> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- Changes v2: - Use mtd_type_is_nand instead of node name check drivers/mtd/mtdcore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)