[PATCH 0/3] nvmem: Bring a tiny bit of sanity to reading 16/32/64 bits from nvmem

[PATCH 0/3] nvmem: Bring a tiny bit of sanity to reading 16/32/64 bits from nvmem

[Douglas Anderson]

This series was inspried by a KASAN warning that I got at bootup caused
by the GPU driver on my system interfacing with the nvmem API incorrectly.

I have posted a fix for the GPU driver but looking at this nvmem entry
made me question how the nvmem API was supposed to work. I've proposed
some improvements and these seem better (to me) than any of:
- Open coding logic like that in "cpr.c" in the GPU driver.
- Ignoring the problem and just forcing everyone in the future to
  always specify a length of "2" for the GPU speed bin cells.
- Ignoring the problem and specifying a length of "4" for the GPU
  speed bin cells.

About applying the patches.
- GPU patch can land on its own.  No need for the nvmem patches.
- nvmem patches can land on their own too.
- If the second nvmem patch lands without the first, however, it will
  break the GPU patch.


Douglas Anderson (3):
  drm/msm: Fix speed-bin support not to access outside valid memory
  nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells
  nvmem: core: nvmem_cell_read() should return the true size

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
 drivers/nvmem/core.c                  | 30 ++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 27 deletions(-)

-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Douglas Anderson]

When running the latest kernel on an sc7180 with KASAN I got this
splat:
  BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
  Read of size 4 at addr ffffff8088f36100 by task kworker/7:1/58
  CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
  Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
  Workqueue: events deferred_probe_work_func
  Call trace:
   dump_backtrace+0x0/0x3a8
   show_stack+0x24/0x30
   dump_stack+0x174/0x1e0
   print_address_description+0x70/0x2e4
   kasan_report+0x178/0x1bc
   __asan_report_load4_noabort+0x44/0x50
   a6xx_gpu_init+0x618/0x644
   adreno_bind+0x26c/0x438

This is because the speed bin is defined like this:
  gpu_speed_bin: gpu_speed_bin@1d2 {
    reg = <0x1d2 0x2>;
    bits = <5 8>;
  };

As you can see the "length" is 2 bytes. That means that the nvmem
subsystem allocates only 2 bytes. The GPU code, however, was casting
the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
not so good.

Let's fix this to just use the nvmem_cell_read_u16() accessor function
which simplifies things and also gets rid of the splat.

Let's also put an explicit conversion from little endian in place just
to make things clear. The nvmem subsystem today is assuming little
endian and this makes it clear. Specifically, the way the above sc7180
cell is interpreted:

NVMEM:
 +--------+--------+--------+--------+--------+
 | ...... | 0x1d3  | 0x1d2  | ...... | 0x000  |
 +--------+--------+--------+--------+--------+
              ^       ^
             msb     lsb

You can see that the least significant data is at the lower address
which is little endian.

NOTE: someone who is truly paying attention might wonder about me
picking the "u16" version of this accessor instead of the "u8" (since
the value is 8 bits big) or the u32 version (just for fun). At the
moment you need to pick the accessor that exactly matches the length
the cell was specified as in the device tree. Hopefully future
patches to the nvmem subsystem will fix this.

Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index ba8e9d3cf0fe..0e2024defd79 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
 		u32 revn)
 {
 	struct opp_table *opp_table;
-	struct nvmem_cell *cell;
 	u32 supp_hw = UINT_MAX;
-	void *buf;
-
-	cell = nvmem_cell_get(dev, "speed_bin");
-	/*
-	 * -ENOENT means that the platform doesn't support speedbin which is
-	 * fine
-	 */
-	if (PTR_ERR(cell) == -ENOENT)
-		return 0;
-	else if (IS_ERR(cell)) {
-		DRM_DEV_ERROR(dev,
-				"failed to read speed-bin. Some OPPs may not be supported by hardware");
-		goto done;
-	}
+	u16 speedbin;
+	int ret;
 
-	buf = nvmem_cell_read(cell, NULL);
-	if (IS_ERR(buf)) {
-		nvmem_cell_put(cell);
+	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
+	if (ret) {
 		DRM_DEV_ERROR(dev,
-				"failed to read speed-bin. Some OPPs may not be supported by hardware");
+			      "failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
+			      ret);
 		goto done;
 	}
+	speedbin = le16_to_cpu(speedbin);
 
-	supp_hw = fuse_to_supp_hw(dev, revn, *((u32 *) buf));
-
-	kfree(buf);
-	nvmem_cell_put(cell);
+	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
 
 done:
 	opp_table = dev_pm_opp_set_supported_hw(dev, &supp_hw, 1);
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Srinivas Kandagatla]

On 27/02/2021 00:26, Douglas Anderson wrote:
> When running the latest kernel on an sc7180 with KASAN I got this
> splat:
>    BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
>    Read of size 4 at addr ffffff8088f36100 by task kworker/7:1/58
>    CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
>    Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>    Workqueue: events deferred_probe_work_func
>    Call trace:
>     dump_backtrace+0x0/0x3a8
>     show_stack+0x24/0x30
>     dump_stack+0x174/0x1e0
>     print_address_description+0x70/0x2e4
>     kasan_report+0x178/0x1bc
>     __asan_report_load4_noabort+0x44/0x50
>     a6xx_gpu_init+0x618/0x644
>     adreno_bind+0x26c/0x438
> 
> This is because the speed bin is defined like this:
>    gpu_speed_bin: gpu_speed_bin@1d2 {
>      reg = <0x1d2 0x2>;
>      bits = <5 8>;
>    };
> 
> As you can see the "length" is 2 bytes. That means that the nvmem
> subsystem allocates only 2 bytes. The GPU code, however, was casting
> the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
> not so good.
> 
> Let's fix this to just use the nvmem_cell_read_u16() accessor function
> which simplifies things and also gets rid of the splat.
> 
> Let's also put an explicit conversion from little endian in place just
> to make things clear. The nvmem subsystem today is assuming little
> endian and this makes it clear. Specifically, the way the above sc7180
> cell is interpreted:
> 
> NVMEM:
>   +--------+--------+--------+--------+--------+
>   | ...... | 0x1d3  | 0x1d2  | ...... | 0x000  |
>   +--------+--------+--------+--------+--------+
>                ^       ^
>               msb     lsb
> 
> You can see that the least significant data is at the lower address
> which is little endian.
> 
> NOTE: someone who is truly paying attention might wonder about me
> picking the "u16" version of this accessor instead of the "u8" (since
> the value is 8 bits big) or the u32 version (just for fun). At the
> moment you need to pick the accessor that exactly matches the length
> the cell was specified as in the device tree. Hopefully future
> patches to the nvmem subsystem will fix this.
> 
> Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
>   1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index ba8e9d3cf0fe..0e2024defd79 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
>   		u32 revn)
>   {
>   	struct opp_table *opp_table;
> -	struct nvmem_cell *cell;
>   	u32 supp_hw = UINT_MAX;
> -	void *buf;
> -
> -	cell = nvmem_cell_get(dev, "speed_bin");
> -	/*
> -	 * -ENOENT means that the platform doesn't support speedbin which is
> -	 * fine
> -	 */
> -	if (PTR_ERR(cell) == -ENOENT)
> -		return 0;
> -	else if (IS_ERR(cell)) {
> -		DRM_DEV_ERROR(dev,
> -				"failed to read speed-bin. Some OPPs may not be supported by hardware");
> -		goto done;
> -	}
> +	u16 speedbin;
> +	int ret;
>   
> -	buf = nvmem_cell_read(cell, NULL);

I think the issue here is not passing len pointer which should return 
how many bytes the cell is!

Then from there we can decide to do le16_to_cpu or le32_to_cpu or not!
This will also future proof the code to handle speed_bins of different 
sizes!

--srini

> -	if (IS_ERR(buf)) {
> -		nvmem_cell_put(cell);

> +	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
> +	if (ret) {
>   		DRM_DEV_ERROR(dev,
> -				"failed to read speed-bin. Some OPPs may not be supported by hardware");
> +			      "failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
> +			      ret);
>   		goto done;
>   	}
> +	speedbin = le16_to_cpu(speedbin);
>   
> -	supp_hw = fuse_to_supp_hw(dev, revn, *((u32 *) buf));
> -
> -	kfree(buf);
> -	nvmem_cell_put(cell);
> +	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
>   
>   done:
>   	opp_table = dev_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> 

Re: [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Doug Anderson]

Hi,

On Fri, Mar 5, 2021 at 2:28 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 27/02/2021 00:26, Douglas Anderson wrote:
> > When running the latest kernel on an sc7180 with KASAN I got this
> > splat:
> >    BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
> >    Read of size 4 at addr ffffff8088f36100 by task kworker/7:1/58
> >    CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
> >    Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
> >    Workqueue: events deferred_probe_work_func
> >    Call trace:
> >     dump_backtrace+0x0/0x3a8
> >     show_stack+0x24/0x30
> >     dump_stack+0x174/0x1e0
> >     print_address_description+0x70/0x2e4
> >     kasan_report+0x178/0x1bc
> >     __asan_report_load4_noabort+0x44/0x50
> >     a6xx_gpu_init+0x618/0x644
> >     adreno_bind+0x26c/0x438
> >
> > This is because the speed bin is defined like this:
> >    gpu_speed_bin: gpu_speed_bin@1d2 {
> >      reg = <0x1d2 0x2>;
> >      bits = <5 8>;
> >    };
> >
> > As you can see the "length" is 2 bytes. That means that the nvmem
> > subsystem allocates only 2 bytes. The GPU code, however, was casting
> > the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
> > not so good.
> >
> > Let's fix this to just use the nvmem_cell_read_u16() accessor function
> > which simplifies things and also gets rid of the splat.
> >
> > Let's also put an explicit conversion from little endian in place just
> > to make things clear. The nvmem subsystem today is assuming little
> > endian and this makes it clear. Specifically, the way the above sc7180
> > cell is interpreted:
> >
> > NVMEM:
> >   +--------+--------+--------+--------+--------+
> >   | ...... | 0x1d3  | 0x1d2  | ...... | 0x000  |
> >   +--------+--------+--------+--------+--------+
> >                ^       ^
> >               msb     lsb
> >
> > You can see that the least significant data is at the lower address
> > which is little endian.
> >
> > NOTE: someone who is truly paying attention might wonder about me
> > picking the "u16" version of this accessor instead of the "u8" (since
> > the value is 8 bits big) or the u32 version (just for fun). At the
> > moment you need to pick the accessor that exactly matches the length
> > the cell was specified as in the device tree. Hopefully future
> > patches to the nvmem subsystem will fix this.
> >
> > Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
> >   1 file changed, 8 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index ba8e9d3cf0fe..0e2024defd79 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
> >               u32 revn)
> >   {
> >       struct opp_table *opp_table;
> > -     struct nvmem_cell *cell;
> >       u32 supp_hw = UINT_MAX;
> > -     void *buf;
> > -
> > -     cell = nvmem_cell_get(dev, "speed_bin");
> > -     /*
> > -      * -ENOENT means that the platform doesn't support speedbin which is
> > -      * fine
> > -      */
> > -     if (PTR_ERR(cell) == -ENOENT)
> > -             return 0;
> > -     else if (IS_ERR(cell)) {
> > -             DRM_DEV_ERROR(dev,
> > -                             "failed to read speed-bin. Some OPPs may not be supported by hardware");
> > -             goto done;
> > -     }
> > +     u16 speedbin;
> > +     int ret;
> >
> > -     buf = nvmem_cell_read(cell, NULL);
>
> I think the issue here is not passing len pointer which should return
> how many bytes the cell is!
>
> Then from there we can decide to do le16_to_cpu or le32_to_cpu or not!
> This will also future proof the code to handle speed_bins of different
> sizes!

I think what you're saying is that you want to copy/paste this code
(or something similar) everywhere that accesses an nvmem cell.  Is
that correct?  ...or maybe you can suggest some smaller / shorter code
that I'm missing?

---

{
  struct nvmem_cell *cell;
  ssize_t len;
  char *ret;
  int i;

  *data = 0;

  cell = nvmem_cell_get(dev, cname);
  if (IS_ERR(cell)) {
    if (PTR_ERR(cell) != -EPROBE_DEFER)
      dev_err(dev, "undefined cell %s\n", cname);
    return PTR_ERR(cell);
  }

  ret = nvmem_cell_read(cell, &len);
  nvmem_cell_put(cell);
  if (IS_ERR(ret)) {
    dev_err(dev, "can't read cell %s\n", cname);
    return PTR_ERR(ret);
  }

  for (i = 0; i < len; i++)
    *data |= ret[i] << (8 * i);

  kfree(ret);
  dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);

  return 0;
}

---

The above code is from cpr_read_efuse() in "cpr.c".  I mentioned in
the cover letter that I thought about doing this and decided it wasn't
a great idea.  There should be _some_ function in the nvmem core that
says: there's an integer that's 32-bits or less stored in nvmem.
Please read it for me.  If you don't think we can use one of the
existing functions for that, would you be opposed to me creating a new
one?

---

In any case, while we discuss what we should do long term, I still
hope that Rob can merge this patch since it fixes the bug.

-Doug

Re: [PATCH 1/3] drm/msm: Fix speed-bin support not to access outside valid memory

[Srinivas Kandagatla]

On 05/03/2021 14:45, Doug Anderson wrote:
> Hi,
> 
> On Fri, Mar 5, 2021 at 2:28 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>
>> On 27/02/2021 00:26, Douglas Anderson wrote:
>>> When running the latest kernel on an sc7180 with KASAN I got this
>>> splat:
>>>     BUG: KASAN: slab-out-of-bounds in a6xx_gpu_init+0x618/0x644
>>>     Read of size 4 at addr ffffff8088f36100 by task kworker/7:1/58
>>>     CPU: 7 PID: 58 Comm: kworker/7:1 Not tainted 5.11.0+ #3
>>>     Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>>>     Workqueue: events deferred_probe_work_func
>>>     Call trace:
>>>      dump_backtrace+0x0/0x3a8
>>>      show_stack+0x24/0x30
>>>      dump_stack+0x174/0x1e0
>>>      print_address_description+0x70/0x2e4
>>>      kasan_report+0x178/0x1bc
>>>      __asan_report_load4_noabort+0x44/0x50
>>>      a6xx_gpu_init+0x618/0x644
>>>      adreno_bind+0x26c/0x438
>>>
>>> This is because the speed bin is defined like this:
>>>     gpu_speed_bin: gpu_speed_bin@1d2 {
>>>       reg = <0x1d2 0x2>;
>>>       bits = <5 8>;
>>>     };
>>>
>>> As you can see the "length" is 2 bytes. That means that the nvmem
>>> subsystem allocates only 2 bytes. The GPU code, however, was casting
>>> the pointer allocated by nvmem to a (u32 *) and dereferencing. That's
>>> not so good.
>>>
>>> Let's fix this to just use the nvmem_cell_read_u16() accessor function
>>> which simplifies things and also gets rid of the splat.
>>>
>>> Let's also put an explicit conversion from little endian in place just
>>> to make things clear. The nvmem subsystem today is assuming little
>>> endian and this makes it clear. Specifically, the way the above sc7180
>>> cell is interpreted:
>>>
>>> NVMEM:
>>>    +--------+--------+--------+--------+--------+
>>>    | ...... | 0x1d3  | 0x1d2  | ...... | 0x000  |
>>>    +--------+--------+--------+--------+--------+
>>>                 ^       ^
>>>                msb     lsb
>>>
>>> You can see that the least significant data is at the lower address
>>> which is little endian.
>>>
>>> NOTE: someone who is truly paying attention might wonder about me
>>> picking the "u16" version of this accessor instead of the "u8" (since
>>> the value is 8 bits big) or the u32 version (just for fun). At the
>>> moment you need to pick the accessor that exactly matches the length
>>> the cell was specified as in the device tree. Hopefully future
>>> patches to the nvmem subsystem will fix this.
>>>
>>> Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu")
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>>    drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 31 +++++++--------------------
>>>    1 file changed, 8 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index ba8e9d3cf0fe..0e2024defd79 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -1350,35 +1350,20 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
>>>                u32 revn)
>>>    {
>>>        struct opp_table *opp_table;
>>> -     struct nvmem_cell *cell;
>>>        u32 supp_hw = UINT_MAX;
>>> -     void *buf;
>>> -
>>> -     cell = nvmem_cell_get(dev, "speed_bin");
>>> -     /*
>>> -      * -ENOENT means that the platform doesn't support speedbin which is
>>> -      * fine
>>> -      */
>>> -     if (PTR_ERR(cell) == -ENOENT)
>>> -             return 0;
>>> -     else if (IS_ERR(cell)) {
>>> -             DRM_DEV_ERROR(dev,
>>> -                             "failed to read speed-bin. Some OPPs may not be supported by hardware");
>>> -             goto done;
>>> -     }
>>> +     u16 speedbin;
>>> +     int ret;
>>>
>>> -     buf = nvmem_cell_read(cell, NULL);
>>
>> I think the issue here is not passing len pointer which should return
>> how many bytes the cell is!
>>
>> Then from there we can decide to do le16_to_cpu or le32_to_cpu or not!
>> This will also future proof the code to handle speed_bins of different
>> sizes!
> 
> I think what you're saying is that you want to copy/paste this code
> (or something similar) everywhere that accesses an nvmem cell.  Is
> that correct?  ...or maybe you can suggest some smaller / shorter code
> that I'm missing?
> 

It depends what the consumer is doing! If it is already aware of what 
size of data its expecting then you can use nvmem_cell_read_u8/16/32/64 
variants, however it wants to do bit more with the data then 
nvmem_cell_read() should give more flexibility!

> ---
> 
> {
>    struct nvmem_cell *cell;
>    ssize_t len;
>    char *ret;
>    int i;
> 
>    *data = 0;
> 
>    cell = nvmem_cell_get(dev, cname);
>    if (IS_ERR(cell)) {
>      if (PTR_ERR(cell) != -EPROBE_DEFER)
>        dev_err(dev, "undefined cell %s\n", cname);
>      return PTR_ERR(cell);
>    }
> 
>    ret = nvmem_cell_read(cell, &len);
>    nvmem_cell_put(cell);
>    if (IS_ERR(ret)) {
>      dev_err(dev, "can't read cell %s\n", cname);
>      return PTR_ERR(ret);
>    }
> 
>    for (i = 0; i < len; i++)
>      *data |= ret[i] << (8 * i);
> 
>    kfree(ret);
>    dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);
> 
>    return 0;
> }
> 
> ---
> 
> The above code is from cpr_read_efuse() in "cpr.c".  I mentioned in
> the cover letter that I thought about doing this and decided it wasn't
> a great idea.  There should be _some_ function in the nvmem core that
> says: there's an integer that's 32-bits or less stored in nvmem.

There is no such helper function other than using the above snippet to do.

> Please read it for me.  If you don't think we can use one of the
> existing functions for that, would you be opposed to me creating a new
> one?

I have no issue in adding a new helper function in nvmem to allow such 
thing!

> 
> ---
> 
> In any case, while we discuss what we should do long term, I still
> hope that Rob can merge this patch since it fixes the bug.

Yes, I agree this definitely fixes the mentioned bug!
> 
> -Doug
> 

[PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Douglas Anderson]

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

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Srinivas Kandagatla]

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.
> 

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Doug Anderson]

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

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Srinivas Kandagatla]

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
> 

Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells

[Doug Anderson]

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

[PATCH 3/3] nvmem: core: nvmem_cell_read() should return the true size

[Douglas Anderson]

If we look at the gpu speed bin currently in sc7180.dtsi:
  gpu_speed_bin: gpu_speed_bin@1d2 {
    reg = <0x1d2 0x2>;
    bits = <5 8>;
  };

We can see that this is an 8-bit value.  However we had to specify the
"reg" as 16 bits because the value was spread out over two bytes.

It doesn't make sense to expose the fact that the value was spread out
over two bytes to the client.  Let's use the number of bits to return
the length to the client.

NOTE: this change has the potential to break clients!  Hopefully this
breakage will be lessened (or eliminated) with the previous patch
("nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller
cells"), but it is possible for anyone directly calling
nvmem_cell_read().  From a quick audit of mainline I don't _see_ any
problems.  Most cases won't change at all (number of bits matched the
length) and the big case that will change is the Qualcomm "CPR" driver
which seems to handle the length properly (it could probably be
simplified now, actually).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/nvmem/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 8602390bb124..00454d841a7f 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1379,6 +1379,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 		      void *buf, size_t *len)
 {
 	int rc;
+	size_t bytes;
 
 	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
 
@@ -1386,11 +1387,15 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 		return rc;
 
 	/* shift bits in-place */
-	if (cell->bit_offset || cell->nbits)
+	if (cell->bit_offset || cell->nbits) {
 		nvmem_shift_read_buffer_in_place(cell, buf);
+		bytes = DIV_ROUND_UP(cell->nbits, 8);
+	} else {
+		bytes = cell->bytes;
+	}
 
 	if (len)
-		*len = cell->bytes;
+		*len = bytes;
 
 	return 0;
 }
-- 
2.30.1.766.gb4fecdf3b7-goog