All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: Rob Clark <robdclark@gmail.com>, Jordan Crouse <jcrouse@codeaurora.org>
Cc: Niklas Cassel <niklas.cassel@linaro.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	swboyd@chromium.org, linux-arm-msm@vger.kernel.org,
	Akhil P Oommen <akhilpo@codeaurora.org>,
	Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>,
	Douglas Anderson <dianders@chromium.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells
Date: Fri, 26 Feb 2021 16:26:02 -0800	[thread overview]
Message-ID: <20210226162521.2.I7c9190630cf9131b42d521aa1c5b97135012a734@changeid> (raw)
In-Reply-To: <20210227002603.3260599-1-dianders@chromium.org>

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.

Let's solve this by allowing clients to read a "larger" value. We'll
just fill it in with 0. 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@chromium.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.
-- 
2.30.1.766.gb4fecdf3b7-goog


  parent reply	other threads:[~2021-02-27  0:27 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-27  0:26 [PATCH 0/3] nvmem: Bring a tiny bit of sanity to reading 16/32/64 bits from nvmem Douglas Anderson
2021-02-27  0:26 ` Douglas Anderson
2021-02-27  0:26 ` [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory Douglas Anderson
2021-02-27  0:26   ` Douglas Anderson
2021-03-05 10:28   ` Srinivas Kandagatla
2021-03-05 10:28     ` Srinivas Kandagatla
2021-03-05 14:45     ` Doug Anderson
2021-03-05 14:45       ` Doug Anderson
2021-03-05 16:07       ` Srinivas Kandagatla
2021-03-05 16:07         ` Srinivas Kandagatla
2021-02-27  0:26 ` Douglas Anderson [this message]
2021-03-05 10:27   ` [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells Srinivas Kandagatla
2021-03-05 14:58     ` Doug Anderson
2021-03-05 16:07       ` Srinivas Kandagatla
2021-03-06  0:28         ` Doug Anderson
2021-02-27  0:26 ` [PATCH 3/3] nvmem: core: nvmem_cell_read() should return the true size Douglas Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210226162521.2.I7c9190630cf9131b42d521aa1c5b97135012a734@changeid \
    --to=dianders@chromium.org \
    --cc=akhilpo@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=jcrouse@codeaurora.org \
    --cc=jorge.ramirez-ortiz@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=niklas.cassel@linaro.org \
    --cc=robdclark@gmail.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=swboyd@chromium.org \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.