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
next prev parent reply other threads:[~2021-02-27 0:27 UTC|newest]
Thread overview: 11+ 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 ` [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory Douglas Anderson
2021-03-05 10:28 ` Srinivas Kandagatla
2021-03-05 14:45 ` Doug Anderson
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).