mtd: limit OTP NVMEM Cell parse to non Nand devices
Subsystems (2)
PATCH v3 1/1 mtd: limit OTP NVMEM Cell parse to non Nand devices
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 detect the MTD type is Nand.
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.
--- drivers/mtd/mtdcore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/mtdcore.c +1 -1
@@ -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;
Signature · 1 line
2.43.0
Quoted 29 lines · Christian Marangi
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_cellswas enabled without thinking of the consequences.That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.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_cellsif we detect the MTD type is Nand.
Quoted 7 lines
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 <[email protected]>Signed-off-by Christian Marangi <[email protected]>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.
Miquel
Quoted 6 lines · Christian Marangi
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_cellswas enabled without thinking of the consequences.
Er... thank you?
Quoted 9 lines
That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.
Quoted 4 lines
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.
So as you noticed this problem was *exposed* by adding OTP support for Macronix NAND chips.
Quoted 8 lines
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_cellsif we detect the MTD type is Nand.
Quoted 5 lines
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")
It's not that commit however that introduced the problem. Introducing "add_legacy_fixed_of_cells" just added a clean way of enabling parsing of old cells syntax. Even before my commit NVMEM subsystem was looking for NVMEM cells in NAND devices.
I booted kernel 6.6 which has commit e87161321a40 ("mtd: rawnand:
macronix: OTP > access for MX30LFxG18AC") but does NOT have commit 2cc3b37f5b6d ("nvmem: add explicit config option to read old syntax fixed OF cells").
Look at this log from Broadcom Northstar (Linux 6.6):
[ 0.410107] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [ 0.416531] nand: Macronix MX30LF4G18AC [ 0.420409] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [ 0.428022] iproc_nand 18028000.nand-controller: detected 512MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 [ 0.438991] Scanning device for bad blocks [ 0.873598] Bad eraseblock 738 at 0x000005c40000 [ 1.030279] random: crng init done [ 1.854895] Bad eraseblock 2414 at 0x000012dc0000 [ 2.657354] Bad eraseblock 3783 at 0x00001d8e0000 [ 2.662967] Bad eraseblock 3785 at 0x00001d920000 [ 2.848418] nvmem user-otp1: nvmem: invalid reg on /nand-controller@18028000/nand@0 [ 2.856126] iproc_nand 18028000.nand-controller: error -EINVAL: Failed to register OTP NVMEM device
So to summary it up:
1. Problem exists since much earlier and wasn't introduced by 2cc3b37f5b6d
2. Commit 2cc3b37f5b6d just gives you a clean way of solving this issue
3. Problem was exposed by commit e87161321a40
4. We miss fix for v6.6 which doesn't have 2cc3b37f5b6d (it hit v6.7)
Quoted 2 lines
Cc <[email protected]>Signed-off-by Christian Marangi <[email protected]>
Quoted 11 lines · Rafał Miłecki
Quoted 7 lines · Christian Marangi
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_cellswas enabled without thinking of the consequences.Er... thank you?
Probably made some bad assumption and sorry for it!
Quoted 60 lines
Quoted 10 lines
That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.Quoted 4 lines
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.So as you noticed this problem was *exposed* by adding OTP support for Macronix NAND chips.
Quoted 8 lines
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_cellsif we detect the MTD type is Nand.Quoted 6 lines
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 oldsyntax fixed OF cells")
It's not that commit however that introduced the problem. Introducing "
add_legacy_fixed_of_cells" just added a clean way of enabling parsing of old cells syntax. Even before my commit NVMEM subsystem was looking for NVMEM cells in NAND devices.I booted kernel 6.6 which has commit
e87161321a40("mtd: rawnand:
macronix: OTP > access for MX30LFxG18AC") but does NOT have commit2cc3b37f5b6d("nvmem: add explicit config option to read old syntax fixed OF cells").Look at this log from Broadcom Northstar (Linux 6.6):
[ 0.410107] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [ 0.416531] nand: Macronix MX30LF4G18AC [ 0.420409] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [ 0.428022] iproc_nand 18028000.nand-controller: detected 512MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 [ 0.438991] Scanning device for bad blocks [ 0.873598] Bad eraseblock 738 at 0x000005c40000 [ 1.030279] random: crng init done [ 1.854895] Bad eraseblock 2414 at 0x000012dc0000 [ 2.657354] Bad eraseblock 3783 at 0x00001d8e0000 [ 2.662967] Bad eraseblock 3785 at 0x00001d920000 [ 2.848418] nvmem user-otp1: nvmem: invalid reg on /nand-controller@18028000/nand@0 [ 2.856126] iproc_nand 18028000.nand-controller: error -EINVAL: Failed to
Quoted 8 lines
register OTP NVMEM device
So to summary it up:
1. Problem exists since much earlier and wasn't introduced by2cc3b37f5b6d
2. Commit2cc3b37f5b6djust gives you a clean way of solving this issue
3. Problem was exposed by commite87161321a40
4. We miss fix for v6.6 which doesn't have2cc3b37f5b6d(it hit v6.7)
So the thing was broken all along? Maybe the regression was introduced when OF support for NVMEM cell was introduced? (and OF scan was enabled by default?)
Anyway Sorry for adding the wrong fixes, maybe Miquel can remote the commit from mtd/fixes and fix the problematic fixes tag?
Quoted 3 lines
Quoted 3 lines
Cc <[email protected]>Signed-off-by Christian Marangi <[email protected]>
Signature · 1 line
Ansuel
Miquel Raynal
maintainerHi Christian,
[email protected] wrote on Wed, 27 Mar 2024 15:36:54 +0100:
Quoted 10 lines · Rafał Miłecki
Quoted 9 lines · Christian Marangi
Quoted 7 lines
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_cellswas enabled without thinking of the consequences.
Quoted 4 lines
Quoted 2 lines
Er... thank you?
Probably made some bad assumption and sorry for it!
Well, "not thinking about all consequences" seems always legitimate to me, we are not robots. Anyway, I agree we should drop this sentence.
Quoted 68 lines
Quoted 59 lines
Quoted 9 lines
That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.Quoted 4 lines
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.So as you noticed this problem was *exposed* by adding OTP support for Macronix NAND chips.
Quoted 8 lines
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_cellsif we detect the MTD type is Nand.Quoted 6 lines
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 oldsyntax fixed OF cells")
It's not that commit however that introduced the problem. Introducing "
add_legacy_fixed_of_cells" just added a clean way of enabling parsing of old cells syntax. Even before my commit NVMEM subsystem was looking for NVMEM cells in NAND devices.I booted kernel 6.6 which has commit
e87161321a40("mtd: rawnand:
macronix: OTP > access for MX30LFxG18AC") but does NOT have commit2cc3b37f5b6d("nvmem: add explicit config option to read old syntax fixed OF cells").Look at this log from Broadcom Northstar (Linux 6.6):
[ 0.410107] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [ 0.416531] nand: Macronix MX30LF4G18AC [ 0.420409] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [ 0.428022] iproc_nand 18028000.nand-controller: detected 512MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 [ 0.438991] Scanning device for bad blocks [ 0.873598] Bad eraseblock 738 at 0x000005c40000 [ 1.030279] random: crng init done [ 1.854895] Bad eraseblock 2414 at 0x000012dc0000 [ 2.657354] Bad eraseblock 3783 at 0x00001d8e0000 [ 2.662967] Bad eraseblock 3785 at 0x00001d920000 [ 2.848418] nvmem user-otp1: nvmem: invalid reg on /nand-controller@18028000/nand@0 [ 2.856126] iproc_nand 18028000.nand-controller: error -EINVAL: Failed toQuoted 8 lines
register OTP NVMEM device
So to summary it up:
1. Problem exists since much earlier and wasn't introduced by2cc3b37f5b6d
2. Commit2cc3b37f5b6djust gives you a clean way of solving this issue
3. Problem was exposed by commite87161321a40
4. We miss fix for v6.6 which doesn't have2cc3b37f5b6d(it hit v6.7)
Quoted 6 lines
So the thing was broken all along? Maybe the regression was introduced when OF support for NVMEM cell was introduced? (and OF scan was enabled by default?)
Anyway Sorry for adding the wrong fixes, maybe Miquel can remote the commit from mtd/fixes and fix the problematic fixes tag?
Yes, please send a v4 (with the sentence above updated) and I will drop v3.
Thanks,
Miquèl
Quoted 94 lines · Miquel Raynal
Hi Christian,
[email protected] wrote on Wed, 27 Mar 2024 15:36:54 +0100:
Quoted 10 lines · Rafał Miłecki
Quoted 9 lines · Christian Marangi
Quoted 7 lines
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_cellswas enabled without thinking of the consequences.Quoted 4 lines
Quoted 2 lines
Er... thank you?
Probably made some bad assumption and sorry for it!
Well, "not thinking about all consequences" seems always legitimate to me, we are not robots. Anyway, I agree we should drop this sentence.
Quoted 72 lines
Quoted 59 lines
Quoted 9 lines
That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.Quoted 4 lines
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.So as you noticed this problem was *exposed* by adding OTP support for Macronix NAND chips.
Quoted 8 lines
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_cellsif we detect the MTD type is Nand.Quoted 6 lines
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 oldsyntax fixed OF cells")
It's not that commit however that introduced the problem. Introducing "
add_legacy_fixed_of_cells" just added a clean way of enabling parsing of old cells syntax. Even before my commit NVMEM subsystem was looking for NVMEM cells in NAND devices.I booted kernel 6.6 which has commit
e87161321a40("mtd: rawnand:
macronix: OTP > access for MX30LFxG18AC") but does NOT have commit2cc3b37f5b6d("nvmem: add explicit config option to read old syntax fixed OF cells").Look at this log from Broadcom Northstar (Linux 6.6):
[ 0.410107] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [ 0.416531] nand: Macronix MX30LF4G18AC [ 0.420409] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [ 0.428022] iproc_nand 18028000.nand-controller: detected 512MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 [ 0.438991] Scanning device for bad blocks [ 0.873598] Bad eraseblock 738 at 0x000005c40000 [ 1.030279] random: crng init done [ 1.854895] Bad eraseblock 2414 at 0x000012dc0000 [ 2.657354] Bad eraseblock 3783 at 0x00001d8e0000 [ 2.662967] Bad eraseblock 3785 at 0x00001d920000 [ 2.848418] nvmem user-otp1: nvmem: invalid reg on /nand-controller@18028000/nand@0 [ 2.856126] iproc_nand 18028000.nand-controller: error -EINVAL: Failed toQuoted 8 lines
register OTP NVMEM device
So to summary it up:
1. Problem exists since much earlier and wasn't introduced by2cc3b37f5b6d
2. Commit2cc3b37f5b6djust gives you a clean way of solving this issue
3. Problem was exposed by commite87161321a40
4. We miss fix for v6.6 which doesn't have2cc3b37f5b6d(it hit v6.7)So the thing was broken all along? Maybe the regression was introduced when OF support for NVMEM cell was introduced? (and OF scan was enabled by default?)
Quoted 6 lines
Quoted 2 lines
Anyway Sorry for adding the wrong fixes, maybe Miquel can remote the commit from mtd/fixes and fix the problematic fixes tag?
Yes, please send a v4 (with the sentence above updated) and I will drop v3.
Thanks a lot! I asked Rafal some suggestion for a better fixes tag and I will send v4.
Signature · 1 line
Ansuel
Quoted 10 lines · Christian Marangi
Quoted 9 lines · Rafał Miłecki
Quoted 7 lines · Christian Marangi
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_cellswas enabled without thinking of the consequences.
Quoted 4 lines
Quoted 2 lines
Er... thank you?
Probably made some bad assumption and sorry for it!
No problem :)
Quoted 68 lines
Quoted 59 lines
Quoted 9 lines
That option enables NVMEM to scan the provided
of_nodeand 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
-EINVALerror.Quoted 4 lines
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.So as you noticed this problem was *exposed* by adding OTP support for Macronix NAND chips.
Quoted 8 lines
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_cellsif we detect the MTD type is Nand.Quoted 6 lines
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 oldsyntax fixed OF cells")
It's not that commit however that introduced the problem. Introducing "
add_legacy_fixed_of_cells" just added a clean way of enabling parsing of old cells syntax. Even before my commit NVMEM subsystem was looking for NVMEM cells in NAND devices.I booted kernel 6.6 which has commit
e87161321a40("mtd: rawnand:
macronix: OTP > access for MX30LFxG18AC") but does NOT have commit2cc3b37f5b6d("nvmem: add explicit config option to read old syntax fixed OF cells").Look at this log from Broadcom Northstar (Linux 6.6):
[ 0.410107] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xdc [ 0.416531] nand: Macronix MX30LF4G18AC [ 0.420409] nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [ 0.428022] iproc_nand 18028000.nand-controller: detected 512MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 [ 0.438991] Scanning device for bad blocks [ 0.873598] Bad eraseblock 738 at 0x000005c40000 [ 1.030279] random: crng init done [ 1.854895] Bad eraseblock 2414 at 0x000012dc0000 [ 2.657354] Bad eraseblock 3783 at 0x00001d8e0000 [ 2.662967] Bad eraseblock 3785 at 0x00001d920000 [ 2.848418] nvmem user-otp1: nvmem: invalid reg on /nand-controller@18028000/nand@0 [ 2.856126] iproc_nand 18028000.nand-controller: error -EINVAL: Failed toQuoted 8 lines
register OTP NVMEM device
So to summary it up:
1. Problem exists since much earlier and wasn't introduced by2cc3b37f5b6d
2. Commit2cc3b37f5b6djust gives you a clean way of solving this issue
3. Problem was exposed by commite87161321a40
4. We miss fix for v6.6 which doesn't have2cc3b37f5b6d(it hit v6.7)
Quoted 3 lines
So the thing was broken all along? Maybe the regression was introduced when OF support for NVMEM cell was introduced? (and OF scan was enabled by default?)
I went back to the commit 4b361cfa8624 ("mtd: core: add OTP nvmem provider support") from 2021 (it went into 5.14). It seems that even back then nvmem_register() used to call nvmem_add_cells_from_of(). SO maybe this problem is as old as that?
Quoted 7 lines
Anyway Sorry for adding the wrong fixes, maybe Miquel can remote the commit from mtd/fixes and fix the problematic fixes tag?
Quoted 3 lines
Quoted 3 lines
Cc <[email protected]>Signed-off-by Christian Marangi <[email protected]>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
drivers/mtd/mtdcore.c static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, · line 906
Quoted 13 lines · Christian Marangi
drivers/mtd/mtdcore.c +1
-1@@ -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;
I think there may be even more unwanted behaviour here. If mtd_otp_nvmem_register() fails to find node with "user-otp" / "factory-otp" compatible then it sets "config.of_node" to NULL but that means NVMEM core still looks for NVMEM cells in device's "of_node".
I believe we should not look for OTP NVMEM cells out of the "user-otp" / "factory-otp" compatible nodes.
So maybe what we need in the first place is just:
config.add_legacy_fixed_of_cells = !!np;
?
Any extra limitation of .add_legacy_fixed_of_cells should probably be used only if we want to prevent new users of the legacy syntax. The problem is that mtd.yaml binding allowed "user-otp" and "factory-otp" with old syntax cells. It means every MTD device was allowed to have them.
No in-kernel DTS even used "user-otp" or "factory-otp" with NVMEM legacy cells but I'm not sure about downstream DTS files. Ideally we would do config.add_legacy_fixed_of_cells = false;
but that could break compatibility with some downstream DTS files.
Quoted 33 lines · Rafał Miłecki
drivers/mtd/mtdcore.c static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, · line 906
Quoted 13 lines · Christian Marangi
drivers/mtd/mtdcore.c +1
-1@@ -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;I think there may be even more unwanted behaviour here. If
mtd_otp_nvmem_register()fails to find node with "user-otp" / "factory-otp" compatible then it sets "config.of_node" to NULL but that means NVMEM core still looks for NVMEM cells in device's "of_node".I believe we should not look for OTP NVMEM cells out of the "user-otp" / "factory-otp" compatible nodes.
So maybe what we need in the first place is just:
config.add_legacy_fixed_of_cells = !!np;
?Any extra limitation of .add_legacy_fixed_of_cells should probably be used only if we want to prevent new users of the legacy syntax. The problem is that
mtd.yamlbinding allowed "user-otp" and "factory-otp" with old syntax cells. It means every MTD device was allowed to have them.
Quoted 4 lines
No in-kernel DTS even used "user-otp" or "factory-otp" with NVMEM legacy cells but I'm not sure about downstream DTS files. Ideally we would do config.add_legacy_fixed_of_cells = false;
but that could break compatibility with some downstream DTS files.
Yes the main problem is prevent regression in downstream. I feel for the nand usage, this is 100% of the times broken. For SPI and other corner case MTD devices it's not?
Anyway did you by chance have a suggestion for a better fixes tag?
Signature · 1 line
Ansuel
Quoted 39 lines · Christian Marangi
Quoted 33 lines · Rafał Miłecki
drivers/mtd/mtdcore.c static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, · line 906
Quoted 13 lines · Christian Marangi
drivers/mtd/mtdcore.c +1
-1@@ -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;I think there may be even more unwanted behaviour here. If
mtd_otp_nvmem_register()fails to find node with "user-otp" / "factory-otp" compatible then it sets "config.of_node" to NULL but that means NVMEM core still looks for NVMEM cells in device's "of_node".I believe we should not look for OTP NVMEM cells out of the "user-otp" / "factory-otp" compatible nodes.
So maybe what we need in the first place is just:
config.add_legacy_fixed_of_cells = !!np;
?Any extra limitation of .add_legacy_fixed_of_cells should probably be used only if we want to prevent new users of the legacy syntax. The problem is that
mtd.yamlbinding allowed "user-otp" and "factory-otp" with old syntax cells. It means every MTD device was allowed to have them.Quoted 4 lines
No in-kernel DTS even used "user-otp" or "factory-otp" with NVMEM legacy cells but I'm not sure about downstream DTS files. Ideally we would do config.add_legacy_fixed_of_cells = false;
but that could break compatibility with some downstream DTS files.
Quoted 5 lines
Yes the main problem is prevent regression in downstream. I feel for the nand usage, this is 100% of the times broken. For SPI and other corner case MTD devices it's not?
Anyway did you by chance have a suggestion for a better fixes tag?
My personal idea for that would be to put two Fixes with two commits and describe in commit body that one just exposed existing bug.
You may check my OpenWrt quick patch for an idea how I'd handle that:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/pending-6.6/440-mtd-don-t-look-for-OTP-legacy-NVMEM-cells-if-proper-.patch;h=d9d15a4048c144d8565c8ea38e15a79f7f4a5fe1;hb=dd78a59cd7b029560b33cb3ac0e1aa8b747bd807
Signature · 5 lines
Rafał Miłecki ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Quoted 47 lines · Rafał Miłecki
Quoted 46 lines · Christian Marangi
Quoted 35 lines · Rafał Miłecki
drivers/mtd/mtdcore.c static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, · line 906
Quoted 13 lines · Christian Marangi
drivers/mtd/mtdcore.c +1
-1@@ -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;I think there may be even more unwanted behaviour here. If
mtd_otp_nvmem_register()fails to find node with "user-otp" / "factory-otp" compatible then it sets "config.of_node" to NULL but that
means NVMEM core still looks for NVMEM cells in device's "of_node".I believe we should not look for OTP NVMEM cells out of the "user-otp" /
"factory-otp" compatible nodes.So maybe what we need in the first place is just:
config.add_legacy_fixed_of_cells = !!np;
?Any extra limitation of .add_legacy_fixed_of_cells should probably be used only if we want to prevent new users of the legacy syntax. The problem is that
mtd.yamlbinding allowed "user-otp" and "factory-otp" with old syntax cells. It means every MTD device was allowed to have them.Quoted 5 lines
No in-kernel DTS even used "user-otp" or "factory-otp" with NVMEM legacy
cells but I'm not sure about downstream DTS files. Ideally we would do config.add_legacy_fixed_of_cells = false;
but that could break compatibility with some downstream DTS files.Yes the main problem is prevent regression in downstream. I feel for the nand usage, this is 100% of the times broken. For SPI and other corner case MTD devices it's not?
Quoted 8 lines
Quoted 1 line
Anyway did you by chance have a suggestion for a better fixes tag?
My personal idea for that would be to put two Fixes with two commits and describe in commit body that one just exposed existing bug.
You may check my OpenWrt quick patch for an idea how I'd handle that:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/pending-6.6/440-mtd-don-t-look-for-OTP-legacy-NVMEM-cells-if-proper-.patch;h=d9d15a4048c144d8565c8ea38e15a79f7f4a5fe1;hb=dd78a59cd7b029560b33cb3ac0e1aa8b747bd807
My concern is that using !!np might pose some regression problem. Also I feel adding the macronix commit in fixes tag might be confusing?
Think I will just use the nand check just to be extra safe and add a kernel dependency for when the add_legacy_fixed_of_cells was introduced since before that a different patch is needed. What do you think?
Signature · 1 line
Ansuel