Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()

[Joy Chakraborty]

On Wed, Feb 7, 2024 at 4:06 AM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
>
>
>
> On 06/02/2024 04:24, Joy Chakraborty wrote:
> > reg_read() callback registered with nvmem core expects an integer error
> > as a return value but rmem_read() returns the number of bytes read, as a
> > result error checks in nvmem core fail even when they shouldn't.
> >
> > Return 0 on success where number of bytes read match the number of bytes
> > requested and a negative error -EINVAL on all other cases.
> >
> > Fixes: 5a3fa75a4d9c ("nvmem: Add driver to expose reserved memory as nvmem")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Joy Chakraborty <joychakr@xxxxxxxxxx>
> > ---
> >   drivers/nvmem/rmem.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/rmem.c b/drivers/nvmem/rmem.c
> > index 752d0bf4445e..a74dfa279ff4 100644
> > --- a/drivers/nvmem/rmem.c
> > +++ b/drivers/nvmem/rmem.c
> > @@ -46,7 +46,12 @@ static int rmem_read(void *context, unsigned int offset,
> >
> >       memunmap(addr);
> >
> > -     return count;
> > +     if (count != bytes) {
>
> How can this fail unless the values set in priv->mem->size is incorrect
>

That should be correct since it would be fetched from the reserved
memory definition in the device tree.

> Only case I see this failing with short reads is when offset cross the
> boundary of priv->mem->size.
>
>
> can you provide more details on the failure usecase, may be with actual
> values of offsets, bytes and priv->mem->size?
>

This could very well happen if a fixed-layout defined for the reserved
memory has a cell which defines an offset and size greater than the
actual size of the reserved mem.
For E.g. if the device tree node is as follows
reserved-memory {
    #address-cells = <1>;
    #size-cells = <1>;
    ranges;
    nvmem@1000 {
        compatible = "nvmem-rmem";
        reg = <0x1000 0x400>;
        no-map;
        nvmem-layout {
            compatible = "fixed-layout";
            #address-cells = <1>;
            #size-cells = <1>;
            calibration@13ff {
                reg = <0x13ff 0x2>;
            };
        };
    };
};
If we try to read the cell "calibration" which crosses the boundary of
the reserved memory then it will lead to a short read.
Though, one might argue that the protection against such cell
definition should be there during fixed-layout parsing in core itself
but that is not there now and would not be a fix.

What I am trying to fix here is not exactly short reads but how the
return value of rmem_read() is treated by the nvmem core, where it
treats a non-zero return from read as an error currently. Hence
returning the number of bytes read leads to false failures if we try
to read a cell.


>
> > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > +             return -EINVAL;
> > +     }
> > +
>
> > +     return 0;
>
> thanks,
> srini
>
> >   }
> >
> >   static int rmem_probe(struct platform_device *pdev)