[v3,2/3] nvmem: core: add nvmem_cell_write_variable_u32()

This function allows nvmem consumers to write values of different sizes
(1-4 bytes) to an nvmem cell without knowing the exact size, akin to a
write counterpart to nvmem_cell_read_variable_le_32(). It discards the
higher order bytes of the passed u32 value based on CPU endianness as
necessary before writing to a cell smaller than 4 bytes.

Signed-off-by: Jennifer Berringer <jberring@redhat.com>
---
 drivers/nvmem/core.c           | 24 ++++++++++++++++++++++++
 include/linux/nvmem-consumer.h |  6 ++++++
 2 files changed, 30 insertions(+)

Thanks for the patch,

On 04/11/2024 15:23, Jennifer Berringer wrote:
> This function allows nvmem consumers to write values of different sizes
> (1-4 bytes) to an nvmem cell without knowing the exact size, akin to a
> write counterpart to nvmem_cell_read_variable_le_32(). It discards the
> higher order bytes of the passed u32 value based on CPU endianness as
> necessary before writing to a cell smaller than 4 bytes.
> 
> Signed-off-by: Jennifer Berringer <jberring@redhat.com>
> ---
>   drivers/nvmem/core.c           | 24 ++++++++++++++++++++++++
>   include/linux/nvmem-consumer.h |  6 ++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 4a5a6efe4bab..4c26a5e8c361 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1815,6 +1815,30 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
>   
>   EXPORT_SYMBOL_GPL(nvmem_cell_write);
>   
> +/**
> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number

> + *
> + * @cell: nvmem cell to be written.
> + * @val: Value to be written which may be truncated.
> + *
> + * Return: length of bytes written or negative on failure.
> + */
> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)

This new API is confusing, as writing to cell should in the same order 
of the data. Doing any data manipulation within the api is confusing to 
users.

If the intention is to know the size of cell before writing, then best 
way to address this is to provide the cell size visibility to the consumer.

--srini

> +{
> +	struct nvmem_cell_entry *entry = cell->entry;
> +	u8 *buf = (u8 *) &val;
> +
> +	if (!entry || entry->bytes > sizeof(u32))
> +		return -EINVAL;
> +

> +#ifdef __BIG_ENDIAN
> +	buf += sizeof(u32) - entry->bytes;
> +#endif
> +
> +	return __nvmem_cell_entry_write(entry, buf, entry->bytes);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_write_variable_u32);
> +
>   static int nvmem_cell_read_common(struct device *dev, const char *cell_id,
>   				  void *val, size_t count)
>   {
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index 34c0e58dfa26..955366a07867 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -56,6 +56,7 @@ void nvmem_cell_put(struct nvmem_cell *cell);
>   void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
>   void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
>   int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len);
> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val);
>   int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val);
>   int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val);
>   int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val);
> @@ -128,6 +129,11 @@ static inline int nvmem_cell_write(struct nvmem_cell *cell,
>   	return -EOPNOTSUPP;
>   }
>   
> +static inline int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>   static inline int nvmem_cell_read_u8(struct device *dev,
>   				     const char *cell_id, u8 *val)
>   {

Thanks for the feedback and for accepting my first patch.

On 12/14/24 10:07, Srinivas Kandagatla wrote:
>> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
> 
>> + *
>> + * @cell: nvmem cell to be written.
>> + * @val: Value to be written which may be truncated.
>> + *
>> + * Return: length of bytes written or negative on failure.
>> + */
>> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> 
> This new API is confusing, as writing to cell should in the same order of the data. Doing any data manipulation within the api is confusing to users.
> 
> If the intention is to know the size of cell before writing, then best way to address this is to provide the cell size visibility to the consumer.
> 
> --srini

My intention was to mirror the existing functions in this file, hoping that would make it less confusing rather than more. nvmem_cell_read_variable_le_u32() already similarly has consumers accessing the contents of a cell without knowing its size, silently filling any bytes not read with zero. The function I add doesn't change the order of the bytes. The existing read_variable_le functions (in contrast) byte swap from little-endian to the CPU's endianness and indicate this in the function name. Otherwise, I believe the function I add here is what would be expected from a write equivalent. Its return value also gives the caller the actual cell size upon success.

The only manipulation done to the data here is truncating to ignore the highest-order bytes. This behavior should match the below example unless the size is 3 bytes, which my patch should handle but the below example can't.

if (entry->bytes == sizeof(u32)) {
    return __nvmem_cell_entry_write(entry, &val, sizeof(u32));
} else if (entry->bytes == sizeof(u16)) {
    u16 val_16 = (u16) val;
    return __nvmem_cell_entry_write(entry, &val_16, sizeof(u16));
} else if (entry->bytes == sizeof(u8)) {
    u8 val_8 = (u8) val;
    return __nvmem_cell_entry_write(entry, &val_8, sizeof(u8));
} else {
    return -EINVAL;
}

Still, if you prefer, I can send new patches that add a function to get the cell size and put any truncation behavior needed by nvmem-reboot-mode into that source file instead.