Re: [PATCH v2 4/4] nvmem: core: Implement force_ro sysfs attribute

Marek Vasut
2024-06-24T00:36:11Z

On 6/23/24 8:13 AM, Greg Kroah-Hartman wrote:
> On Sun, Jun 23, 2024 at 05:15:47AM +0200, Marek Vasut wrote:
>> Implement "force_ro" sysfs attribute to allow users to set read-write
>> devices as read-only and back to read-write from userspace. The choice
>> of the name is based on MMC core 'force_ro' attribute.
>>
>> This solves a situation where an AT24 I2C EEPROM with GPIO based nWP
>> signal may have to be occasionally updated. Such I2C EEPROM device is
>> usually set as read-only during most of the regular system operation,
>> but in case it has to be updated in a controlled manner, it could be
>> unlocked using this new "force_ro" sysfs attribute and then re-locked
>> again.
>>
>> The "read-only" DT property and config->read_only configuration is
>> respected and is used to set default state of the device, read-only
>> or read-write, for devices which do implement .reg_write function.
>> For devices which do not implement .reg_write function, the device
>> is unconditionally read-only and the "force_ro" attribute is not
>> visible.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Cc: linux-i2c@vger.kernel.org
>> ---
>> V2: - Document the force_ro attribute
>> - Use sysfs_emit()
>> ---
>> Documentation/ABI/stable/sysfs-bus-nvmem | 13 +++++++
>> drivers/nvmem/core.c | 44 ++++++++++++++++++++++++
>> drivers/nvmem/internals.h | 1 +
>> 3 files changed, 58 insertions(+)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem b/Documentation/ABI/stable/sysfs-bus-nvmem
>> index 0e3a8492a1082..c713c4e6cecb1 100644
>> --- a/Documentation/ABI/stable/sysfs-bus-nvmem
>> +++ b/Documentation/ABI/stable/sysfs-bus-nvmem
>> @@ -1,3 +1,16 @@
>> +What: /sys/bus/nvmem/devices/.../force_ro
>> +Date: June 2024
>> +KernelVersion: 6.11
>> +Contact: Marek Vasut <marex@denx.de>
>> +Description:
>> + This read/write attribute allows users to set read-write
>> + devices as read-only and back to read-write from userspace.
>> + This can be used to unlock and relock write-protection of
>> + devices which are generally locked, except during sporadic
>> + programming operation.
>> + Note: This file is only present if CONFIG_NVMEM_SYSFS
>> + is enabled
>
> And what exactly are the values that can be read or written here?

I expanded it to this:

This read/write attribute allows users to set read-write
devices as read-only and back to read-write from userspace.
This can be used to unlock and relock write-protection of
devices which are generally locked, except during sporadic
programming operation.
+Read returns '0' or '1' for read-write or read-only modes
+respectively.
+Write parses one of 'YyTt1NnFf0', or [oO][NnFf] for "on"
+and "off", i.e. what kstrbool() supports.
Note: This file is only present if CONFIG_NVMEM_SYSFS
is enabled.

But I wonder, wouldn't it be better if write only supported '0' and '1'
too, instead of the whole list of stuff that kstrbool() supports ?