Re: [PATCH v3] mtd: limit OTP NVMEM Cell parse to non Nand devices

From: "Rafał Miłecki" <rafal@milecki.pl>
To: Christian Marangi <ansuelsmth@gmail.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Claudiu Beznea <claudiu.beznea@tuxon.dev>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v3] mtd: limit OTP NVMEM Cell parse to non Nand devices
Date: Thu, 28 Mar 2024 15:44:15 +0100	[thread overview]
Message-ID: <a4632f1ba701f6b333c50a5366723cf4@milecki.pl> (raw)
In-Reply-To: <66057c71.050a0220.e4ba.97dc@mx.google.com>

On 2024-03-28 15:19, Christian Marangi wrote:
> On Wed, Mar 27, 2024 at 11:15:02PM +0100, Rafał Miłecki wrote:
>> On 22.03.2024 05:09, Christian Marangi wrote:
>> > 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;
>> 
>> 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.
> 
> 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

-- 
Rafał Miłecki

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/