Date:   Fri, 5 Mar 2021 10:27:54 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Douglas Anderson <dianders@...omium.org>,
        Rob Clark <robdclark@...il.com>,
        Jordan Crouse <jcrouse@...eaurora.org>
Cc:     Niklas Cassel <niklas.cassel@...aro.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        swboyd@...omium.org, linux-arm-msm@...r.kernel.org,
        Akhil P Oommen <akhilpo@...eaurora.org>,
        Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read
 smaller cells



On 27/02/2021 00:26, Douglas Anderson wrote:
> The current way that cell "length" is specified for nvmem cells is a
> little fuzzy. For instance, let's look at the gpu speed bin currently
> in sc7180.dtsi:
> 
>    gpu_speed_bin: gpu_speed_bin@1d2 {
>      reg = <0x1d2 0x2>;
>      bits = <5 8>;
>    };
> 
> This is an 8-bit value (as specified by the "bits" field). However,
> it has a "length" of 2 (bytes), presumably because the value spans
> across two bytes.
> 
> When querying this value right now, it's hard for a client to know if
> they should be calling nvmem_cell_read_u16() or nvmem_cell_read_u8().
> Today they must call nvmem_cell_read_u16() because the "length" of the
> cell was 2 (bytes). However, if a later SoC ever came around and
> didn't span across 2 bytes it would be unclear.  If a later Soc
> specified, for instance:
> 
>    gpu_speed_bin: gpu_speed_bin@100 {
>      reg = <0x100 0x1>;
>      bits = <0 8>;
>    };
> 
> ...then the caller would need to change to try calling
> nvmem_cell_read_u8() because the u16 version would fail.
> 

If the consumer driver is expecting the sizes to span around byte to 
many bytes, then, Why not just call nvmem_cell_read() which should also 
return you how many bytes it has read!


> Let's solve this by allowing clients to read a "larger" value. We'll
> just fill it in with 0. 

That is misleading the consumer! If the consumer is expecting a u16 or 
u32, cell size should be of that size!!

--srini

If a client truly wants to know exactly how
> big the cell was they can fall back to calling nvmem_cell_read()
> directly.
> 
> NOTE: current implementation assumes that cells are little endian when
> up-casting the size, but that's already pretty implicit in the way
> nvmem works now anyway. See nvmem_shift_read_buffer_in_place(). Let's
> document this but not do any auto-conversions just in case there was
> an instance where someone was using this API to read big endian data
> on a big endian machine and it happened to be working because there
> was no bit offset.
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> I will freely admit that I always end up thinking in circles and
> getting dizzy when I think too much about endian considerations. If
> anyone has better intuition than I do and see that I've goofed this up
> then please yell.
> 
>   drivers/nvmem/core.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index a5ab1e0c74cf..8602390bb124 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1534,12 +1534,14 @@ static int nvmem_cell_read_common(struct device *dev, const char *cell_id,
>   		nvmem_cell_put(cell);
>   		return PTR_ERR(buf);
>   	}
> -	if (len != count) {
> +	if (len > count) {
>   		kfree(buf);
>   		nvmem_cell_put(cell);
>   		return -EINVAL;
> +	} else if (len != count) {
> +		memset(val + len, 0, count - len);
>   	}
> -	memcpy(val, buf, count);
> +	memcpy(val, buf, len);
>   	kfree(buf);
>   	nvmem_cell_put(cell);
>   
> @@ -1564,6 +1566,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u8);
>   /**
>    * nvmem_cell_read_u16() - Read a cell value as a u16
>    *
> + * This function can be used to read any cell value 16-bits or less.  If
> + * this function needs to upcast (or if the cell was stored in nvmem with
> + * a bit offset) it will assume that the cell is little endian.  Clients
> + * should generally call le16_to_cpu() on the returned value.
> + *
>    * @dev: Device that requests the nvmem cell.
>    * @cell_id: Name of nvmem cell to read.
>    * @val: pointer to output value.
> @@ -1579,6 +1586,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u16);
>   /**
>    * nvmem_cell_read_u32() - Read a cell value as a u32
>    *
> + * This function can be used to read any cell value 32-bits or less.  If
> + * this function needs to upcast (or if the cell was stored in nvmem with
> + * a bit offset) it will assume that the cell is little endian.  Clients
> + * should generally call le32_to_cpu() on the returned value.
> + *
>    * @dev: Device that requests the nvmem cell.
>    * @cell_id: Name of nvmem cell to read.
>    * @val: pointer to output value.
> @@ -1594,6 +1606,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u32);
>   /**
>    * nvmem_cell_read_u64() - Read a cell value as a u64
>    *
> + * This function can be used to read any cell value 64-bits or less.  If
> + * this function needs to upcast (or if the cell was stored in nvmem with
> + * a bit offset) it will assume that the cell is little endian.  Clients
> + * should generally call le64_to_cpu() on the returned value.
> + *
>    * @dev: Device that requests the nvmem cell.
>    * @cell_id: Name of nvmem cell to read.
>    * @val: pointer to output value.
> 

Powered by blists - more mailing lists