by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

On 17/02/2022 08:48, Miquel Raynal wrote:
> Hi Srinivas,
>
> [email protected] wrote on Wed, 16 Feb 2022 09:24:29 +0000:
>
>> On 16/02/2022 08:46, Christophe Kerello wrote:
>>> Hi Miquel, Pratyush, Srinivas,
>>>
>>> On 2/2/22 12:53, Pratyush Yadav wrote:
>>>> + Khouloud, Linus, Bartosz
>>>>
>>>> On 02/02/22 11:44AM, Christophe Kerello wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/1/22 11:47, Pratyush Yadav wrote:
>>>>>> On 31/01/22 02:43PM, Miquel Raynal wrote:
>>>>>>> Hi Vignesh, Tudory, Pratyush,
>>>>>>>
>>>>>>> + Tudor and Pratyush
>>>>>>>
>>>>>>> [email protected] wrote on Mon, 31 Jan 2022 10:57:55 >>>>> +0100:
>>>>>>>
>>>>>>>> Wp-gpios property can be used on NVMEM nodes and the same property >>>>>> can
>>>>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>>>>>>>> defined at NAND level node, the GPIO management is done at NAND >>>>>> driver
>>>>>>>> level. Write protect is disabled when the driver is probed or resumed
>>>>>>>> and is enabled when the driver is released or suspended.
>>>>>>>>
>>>>>>>> When no partitions are defined in the NAND DT node, then the NAND >>>>>> DT node
>>>>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
>>>>>>>> this node, the GPIO resource is taken twice and the NAND controller
>>>>>>>> driver fails to probe.
>>>>>>>>
>>>>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
>>>>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
>>>>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
>>>>>>>> wp_gpios property.
>>>>>>>>
>>>>>>>> Signed-off-by: Christophe Kerello <[email protected]>
>>>>>>>> ---
>>>>>>>>    drivers/mtd/mtdcore.c | 2 ++
>>>>>>>>    1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>>>>>> index 70f492dce158..e6d251594def 100644
>>>>>>>> --- a/drivers/mtd/mtdcore.c
>>>>>>>> +++ b/drivers/mtd/mtdcore.c
>>>>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>>>>>>>>        config.stride = 1;
>>>>>>>>        config.read_only = true;
>>>>>>>>        config.root_only = true;
>>>>>>>> +    config.skip_wp_gpio = true;
>>>>>>>>        config.no_of_node = !of_device_is_compatible(node, >>>>>> "nvmem-cells");
>>>>>>>>        config.priv = mtd;
>>>>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device >>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
>>>>>>>>        config.owner = THIS_MODULE;
>>>>>>>>        config.type = NVMEM_TYPE_OTP;
>>>>>>>>        config.root_only = true;
>>>>>>>> +    config.skip_wp_gpio = true;
>>>>>>>>        config.reg_read = reg_read;
>>>>>>>>        config.size = size;
>>>>>>>>        config.of_node = np;
>>>>>>>
>>>>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
>>>>>>> WP pin when there is one? At least for raw NAND devices, I don't want
>>>>>>> the NVMEM core to handle the wp pin. So we've introduced this
>>>>>>> skip_wp_gpio nvmem config option. But there are two places where this
>>>>>>> boolean can be set and one of these is for otp regions (see above). In
>>>>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
>>>>>>> nvmem protection. Please tell us if you think this is fine for you.
>>>>>>
>>>>>> Why does NVMEM touch hardware write protection in the first place? The
>>>>>> purpose of the framework is to provide a way to retrieve config stored
>>>>>> in memory. It has no business dealing with details of the chip like the
>>>>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
>>>>>> SPI NAND, etc.). If you want to write protect a cell then do it in
>>>>>> software. I don't see why NVMEM should be dealing with hardware >>>> directly
>>>>>> at all.
>>>>>>
>>>>>> That is my mental model of how things _should_ work. I have not spent
>>>>>> much time digging into how things actually work currently.
>>>>>>
>>>>>
>>>>> Wp-gpios property management was added in MVMEM framework in January >>> 2020 =>
>>>>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
>>>>> "
>>>>>      nvmem: add support for the write-protect pin
>>>>>
>>>>>      The write-protect pin handling looks like a standard property that
>>>>>      could benefit other users if available in the core nvmem framework.
>>>>>
>>>>>      Instead of modifying all the memory drivers to check this pin, make
>>>>>      the NVMEM subsystem check if the write-protect GPIO being passed
>>>>>      through the nvmem_config or defined in the device tree and pull it
>>>>>      low whenever writing to the memory.
>>>>> "
>>>>>
>>>>> And this modification was done for EEPROMs flashes => sha1:
>>>>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
>>>>> "
>>>>>      eeprom: at24: remove the write-protect pin support
>>>>>
>>>>>      NVMEM framework is an interface for the at24 EEPROMs as well as for
>>>>>      other drivers, instead of passing the wp-gpios over the different
>>>>>      drivers each time, it would be better to pass it over the NVMEM
>>>>>      subsystem once and for all.
>>>>>
>>>>>      Removing the support for the write-protect pin after adding it to
>>>>>      the NVMEM subsystem.
>>>>> "
>>>>>
>>>>> Current NVMEM framework implementation toggles the WP GPIO when >>> reg_write
>>>>> nvmem_config API is defined. In case of MTD framework, reg_write is not
>>>>> defined in nvmem_config.
>>>>
>>>> Thanks for digging these up. I think this was the wrong decision to
>>>> make. NVMEM should just provide the APIs for handling read/write, and
>>>> leave the rest to the drivers.
>>>>
>>>> It might be convenient for some drivers to put the WP GPIO handling to
>>>> NVMEM core but I just don't think it is the job of the framework to deal
>>>> with this, and it just does not know enough about the device to deal
>>>> with correctly and completely anyway. For example, wp-gpio is only one
>>>> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
>>>> is often toggled via the SPI controller. There could also be registers
>>>> that do the write protection.
>>>>
>>>> One would have to make strong justifications for making nvmem directly
>>>> deal with hardware level details to convince me it is a good idea. IMHO
>>>> if AT24 EEPROM is the only driver relying on this as of now then we
>>>> should just revert the patches and not have to deal with the
>>>> skip_wp_gpio hackery.
>>>>
>>>
>>> Based on the bindings documentation, AT24 EEPROM driver is not the only
>>> driver relying on this implementation. At least, AT25 EEPROM driver will
>>> have to be modified to handle the Write Protect management, and there is
>>> probably others drivers relying on this implementation.
>>>
>>> So, should we keep the legacy and apply the proposal patch to fix this
>>> conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
>>> recommended by Miquel)?
>>> Or should we revert the Write Protect management in NVMEM framework
>>> but in this case I will not be able to handle such modifications (I am
>>> not able to test those drivers).
>>
>> Firstly sorry for such a long delay to reply this thread as I was in travel.
>>
>> I agree with comments from Pratyush but I see no harm in handling simple usecases of write-protect gpio in nvmem core. In fact wp-gpios and read-only are part of nvmem provider bindings.
>>
>> But usecases like the ones described in this patch series which do not want nvmem core to deal with this pin should set this new flag. I think this is a balanced choice.
>>
>> reverting the wp-gpio patch is going to break other providers that are using this bindings.
>
> I am always puzzled when the community deeply cares about non-mainline
> drivers.
>
> On one side I can understand that creating a 'grab-the-wp-line'
> flag would immediately break all external users but this is, as
> reported by Pratyush, the more logical approach IMHO. However we might
> possibly miss situations where the flag is necessary and break these
> devices.
>
> Otherwise the 'ignore-wp' flag is more conservative, it does not

ingore-wp seems to be more sensible flag than skip_wp_gpio flag.

> break the existing users but would just address the MTD case for now, we
> might have other in-tree subsystem that are affected.
>
> I'm fine either way TBH :-) I would just like this patchset to go in

Am okay either way too, It is just that ingore-wp seems a balanced
choice in the current situation :-).

> through the next merge window.
Sure.

I can Ack nvmem patch if you wish to carry it via mtd tree.

or

nvmem patches go via Greg's char-misc tree. I can take 4/4 if you ack it
once V3 is sent

--srini
>
> Thanks,
> Miquèl

2022-02-17 23:43:22

by Miquel Raynal

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

Hi Srinivas,

[email protected] wrote on Thu, 17 Feb 2022 11:00:23 +0000:

> On 17/02/2022 08:48, Miquel Raynal wrote:
> > Hi Srinivas,
> >
> > [email protected] wrote on Wed, 16 Feb 2022 09:24:29 +0000:
> >
> >> On 16/02/2022 08:46, Christophe Kerello wrote:
> >>> Hi Miquel, Pratyush, Srinivas,
> >>>
> >>> On 2/2/22 12:53, Pratyush Yadav wrote:
> >>>> + Khouloud, Linus, Bartosz
> >>>>
> >>>> On 02/02/22 11:44AM, Christophe Kerello wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 2/1/22 11:47, Pratyush Yadav wrote:
> >>>>>> On 31/01/22 02:43PM, Miquel Raynal wrote:
> >>>>>>> Hi Vignesh, Tudory, Pratyush,
> >>>>>>>
> >>>>>>> + Tudor and Pratyush
> >>>>>>>
> >>>>>>> [email protected] wrote on Mon, 31 Jan 2022 10:57:55 >>>>> +0100:
> >>>>>>> >>>>>>>> Wp-gpios property can be used on NVMEM nodes and the same property >>>>>> can
> >>>>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
> >>>>>>>> defined at NAND level node, the GPIO management is done at NAND >>>>>> driver
> >>>>>>>> level. Write protect is disabled when the driver is probed or resumed
> >>>>>>>> and is enabled when the driver is released or suspended.
> >>>>>>>>
> >>>>>>>> When no partitions are defined in the NAND DT node, then the NAND >>>>>> DT node
> >>>>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
> >>>>>>>> this node, the GPIO resource is taken twice and the NAND controller
> >>>>>>>> driver fails to probe.
> >>>>>>>>
> >>>>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> >>>>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
> >>>>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
> >>>>>>>> wp_gpios property.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Christophe Kerello <[email protected]>
> >>>>>>>> ---
> >>>>>>>>    drivers/mtd/mtdcore.c | 2 ++
> >>>>>>>>    1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >>>>>>>> index 70f492dce158..e6d251594def 100644
> >>>>>>>> --- a/drivers/mtd/mtdcore.c
> >>>>>>>> +++ b/drivers/mtd/mtdcore.c
> >>>>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >>>>>>>>        config.stride = 1;
> >>>>>>>>        config.read_only = true;
> >>>>>>>>        config.root_only = true;
> >>>>>>>> +    config.skip_wp_gpio = true;
> >>>>>>>>        config.no_of_node = !of_device_is_compatible(node, >>>>>> "nvmem-cells");
> >>>>>>>>        config.priv = mtd;
> >>>>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device >>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
> >>>>>>>>        config.owner = THIS_MODULE;
> >>>>>>>>        config.type = NVMEM_TYPE_OTP;
> >>>>>>>>        config.root_only = true;
> >>>>>>>> +    config.skip_wp_gpio = true;
> >>>>>>>>        config.reg_read = reg_read;
> >>>>>>>>        config.size = size;
> >>>>>>>>        config.of_node = np;
> >>>>>>>
> >>>>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
> >>>>>>> WP pin when there is one? At least for raw NAND devices, I don't want
> >>>>>>> the NVMEM core to handle the wp pin. So we've introduced this
> >>>>>>> skip_wp_gpio nvmem config option. But there are two places where this
> >>>>>>> boolean can be set and one of these is for otp regions (see above). In
> >>>>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> >>>>>>> nvmem protection. Please tell us if you think this is fine for you.
> >>>>>>
> >>>>>> Why does NVMEM touch hardware write protection in the first place? The
> >>>>>> purpose of the framework is to provide a way to retrieve config stored
> >>>>>> in memory. It has no business dealing with details of the chip like the
> >>>>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
> >>>>>> SPI NAND, etc.). If you want to write protect a cell then do it in
> >>>>>> software. I don't see why NVMEM should be dealing with hardware >>>> directly
> >>>>>> at all.
> >>>>>>
> >>>>>> That is my mental model of how things _should_ work. I have not spent
> >>>>>> much time digging into how things actually work currently.
> >>>>>> >>>>>
> >>>>> Wp-gpios property management was added in MVMEM framework in January >>> 2020 =>
> >>>>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
> >>>>> "
> >>>>>      nvmem: add support for the write-protect pin
> >>>>>
> >>>>>      The write-protect pin handling looks like a standard property that
> >>>>>      could benefit other users if available in the core nvmem framework.
> >>>>>
> >>>>>      Instead of modifying all the memory drivers to check this pin, make
> >>>>>      the NVMEM subsystem check if the write-protect GPIO being passed
> >>>>>      through the nvmem_config or defined in the device tree and pull it
> >>>>>      low whenever writing to the memory.
> >>>>> "
> >>>>>
> >>>>> And this modification was done for EEPROMs flashes => sha1:
> >>>>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
> >>>>> "
> >>>>>      eeprom: at24: remove the write-protect pin support
> >>>>>
> >>>>>      NVMEM framework is an interface for the at24 EEPROMs as well as for
> >>>>>      other drivers, instead of passing the wp-gpios over the different
> >>>>>      drivers each time, it would be better to pass it over the NVMEM
> >>>>>      subsystem once and for all.
> >>>>>
> >>>>>      Removing the support for the write-protect pin after adding it to
> >>>>>      the NVMEM subsystem.
> >>>>> "
> >>>>>
> >>>>> Current NVMEM framework implementation toggles the WP GPIO when >>> reg_write
> >>>>> nvmem_config API is defined. In case of MTD framework, reg_write is not
> >>>>> defined in nvmem_config.
> >>>>
> >>>> Thanks for digging these up. I think this was the wrong decision to
> >>>> make. NVMEM should just provide the APIs for handling read/write, and
> >>>> leave the rest to the drivers.
> >>>>
> >>>> It might be convenient for some drivers to put the WP GPIO handling to
> >>>> NVMEM core but I just don't think it is the job of the framework to deal
> >>>> with this, and it just does not know enough about the device to deal
> >>>> with correctly and completely anyway. For example, wp-gpio is only one
> >>>> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
> >>>> is often toggled via the SPI controller. There could also be registers
> >>>> that do the write protection.
> >>>>
> >>>> One would have to make strong justifications for making nvmem directly
> >>>> deal with hardware level details to convince me it is a good idea. IMHO
> >>>> if AT24 EEPROM is the only driver relying on this as of now then we
> >>>> should just revert the patches and not have to deal with the
> >>>> skip_wp_gpio hackery.
> >>>> >>>
> >>> Based on the bindings documentation, AT24 EEPROM driver is not the only
> >>> driver relying on this implementation. At least, AT25 EEPROM driver will
> >>> have to be modified to handle the Write Protect management, and there is
> >>> probably others drivers relying on this implementation.
> >>>
> >>> So, should we keep the legacy and apply the proposal patch to fix this
> >>> conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
> >>> recommended by Miquel)?
> >>> Or should we revert the Write Protect management in NVMEM framework
> >>> but in this case I will not be able to handle such modifications (I am
> >>> not able to test those drivers).
> >>
> >> Firstly sorry for such a long delay to reply this thread as I was in travel.
> >>
> >> I agree with comments from Pratyush but I see no harm in handling simple usecases of write-protect gpio in nvmem core. In fact wp-gpios and read-only are part of nvmem provider bindings.
> >>
> >> But usecases like the ones described in this patch series which do not want nvmem core to deal with this pin should set this new flag. I think this is a balanced choice.
> >>
> >> reverting the wp-gpio patch is going to break other providers that are using this bindings.
> >
> > I am always puzzled when the community deeply cares about non-mainline
> > drivers.
> >
> > On one side I can understand that creating a 'grab-the-wp-line'
> > flag would immediately break all external users but this is, as
> > reported by Pratyush, the more logical approach IMHO. However we might
> > possibly miss situations where the flag is necessary and break these
> > devices.
> >
> > Otherwise the 'ignore-wp' flag is more conservative, it does not
>
> ingore-wp seems to be more sensible flag than skip_wp_gpio flag.
>
>
> > break the existing users but would just address the MTD case for now, we
> > might have other in-tree subsystem that are affected.
> >
> > I'm fine either way TBH :-) I would just like this patchset to go in
>
> Am okay either way too, It is just that ingore-wp seems a balanced choice in the current situation :-).
>
> > through the next merge window.
> Sure.
>
> I can Ack nvmem patch if you wish to carry it via mtd tree.

Yes, that would be ideal to prevent build issues during the merge
window/in linux-next.

>
> or
>
> nvmem patches go via Greg's char-misc tree. I can take 4/4 if you ack it once V3 is sent

Thanks,
Miquèl