| Message ID | 20210226162521.2.I7c9190630cf9131b42d521aa1c5b97135012a734@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | nvmem: Bring a tiny bit of sanity to reading 16/32/64 bits from nvmem | expand |
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@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. >
Hi, On Fri, Mar 5, 2021 at 2:27 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > > 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 I guess in my mind that's outside of the scope of what the consumer should need to know. The consumer wants a number and they know it's stored in nvmem. They shouldn't need to consider the bit packing within nvmem. Imagine that have a structure definition: struct example { int num1:6; int num2:6; int num3:6; int num4:6; }; struct example e; What I think you're saying is that you should need a different syntax for accessing "e.num1" and "e.num4" (because they happen not to span bytes) compared to accessing "e.num2" and "e.num3". As it is, C abstracts this out and allows you not to care. You can just do: e.num1 + e.num2 + e.num3 + e.num4 ...and it works fine even though some of those span bytes and some don't. I want the same thing. > , then, Why not just call nvmem_cell_read() which should also > return you how many bytes it has read! See my response to patch #1. This requires open-coding a small but still non-trivial bit of code for all consumers. It should be in the core. > > 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!! If you think it's confusing to change the behavior of the existing functions, would you be opposed to me adding a new function like nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that would be flexible like this? -Doug
On 05/03/2021 14:58, Doug Anderson wrote: > Hi, > > On Fri, Mar 5, 2021 at 2:27 AM Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> >> >> >> 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 > > I guess in my mind that's outside of the scope of what the consumer > should need to know. The consumer wants a number and they know it's > stored in nvmem. They shouldn't need to consider the bit packing > within nvmem. Imagine that have a structure definition: > > struct example { > int num1:6; > int num2:6; > int num3:6; > int num4:6; > }; > struct example e; > > What I think you're saying is that you should need a different syntax > for accessing "e.num1" and "e.num4" (because they happen not to span > bytes) compared to accessing "e.num2" and "e.num3". As it is, C > abstracts this out and allows you not to care. You can just do: > > e.num1 + e.num2 + e.num3 + e.num4 > > ...and it works fine even though some of those span bytes and some > don't. I want the same thing. > > >> , then, Why not just call nvmem_cell_read() which should also >> return you how many bytes it has read! > > See my response to patch #1. This requires open-coding a small but > still non-trivial bit of code for all consumers. It should be in the > core. I agree with that this should be in core! But changing the exiting behavior of the apis is the one am against! For example if we are reading a fixed size UUID or some cell like that we would want to validate it, allowing flexible sizes would not catch errors. Also if its variable size then which apis should consumer use, should he use u32 or u16 based, this adds more confusion to this! > > >>> 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!! > > If you think it's confusing to change the behavior of the existing > functions, would you be opposed to me adding a new function like > nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that > would be flexible like this? This should be perfectly okay! may be something like: int nvmem_read_variable_cell(struct device *dev, const char *cell_id, void *buf, size_t sz_min, size_t sz_max); It should return number of bytes it read and fail if cell size is less then sz_min! --srini > > -Doug >
Hi, On Fri, Mar 5, 2021 at 8:07 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > If you think it's confusing to change the behavior of the existing > > functions, would you be opposed to me adding a new function like > > nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that > > would be flexible like this? > > This should be perfectly okay! > may be something like: > > int nvmem_read_variable_cell(struct device *dev, const char *cell_id, > void *buf, size_t sz_min, size_t sz_max); > > It should return number of bytes it read and fail if cell size is less > then sz_min! The above API isn't really what I want, though. Specifically I want the API to acknowledge that it's a number that is being read. The client just wants a number and any conversion / zero-padding / whatever needs to be abstracted out. The client also doesn't really care how big the cell actually was as long as the data fits, so I wouldn't want to return it. OK, I've come up with a new proposal, so maybe we can continue the conversation there. The API for my new function actually matches exactly with the old cpr_read_efuse() which makes me feel like it's the right way to go... https://lore.kernel.org/r/20210305162546.1.I323dad4343256b48af2be160b84b1e87985cc9be@changeid -Doug
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.
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(-)