Received-SPF: pass (google.com: domain of linux-kernel+bounces-54724-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 6 Feb 2024 10:56:58 +0000
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Joy Chakraborty <joychakr@google.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Rob Herring <robh@kernel.org>,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	linux-kernel@vger.kernel.org, manugautam@google.com,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] nvmem: rmem: Fix return value of rmem_read()
Message-ID: <2024020637-handpick-pamphlet-bacb@gregkh>
References: <20240206042408.224138-1-joychakr@google.com>
 <2024020647-submarine-lucid-ea7b@gregkh>
 <CAOSNQF3jk+85-P+NB-1w=nQwJr1BBO9OQuLbm6s8PiXrFMQdjg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOSNQF3jk+85-P+NB-1w=nQwJr1BBO9OQuLbm6s8PiXrFMQdjg@mail.gmail.com>

On Tue, Feb 06, 2024 at 04:01:02PM +0530, Joy Chakraborty wrote:
> On Tue, Feb 6, 2024 at 3:00 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Feb 06, 2024 at 04:24:08AM +0000, 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@vger.kernel.org
> > > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > > ---
> > >  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) {
> > > +             dev_err(priv->dev, "Failed read memory (%d)\n", count);
> > > +             return -EINVAL;
> >
> > Why is a "short read" somehow illegal here?  What internal changes need
> > to be made now that this has changed?
> 
> In my opinion "short read" should be illegal for cases where if the
> nvmem core is unable to read the required size of data to fill up a
> nvmem cell then data returned might have truncated value.

But that's kind of against what a read() call normally expects.

> No internal changes should be made since the registered reg_read() is
> called from  __nvmem_reg_read() which eventually passes on the error
> code to nvmem_reg_read() whose return code is already checked and
> passed to nvmem consumers.
> Currently rmem driver is incorrectly passing a positive value for success.

So this is an internal api issue and not a general issue?  Unwinding the
read callbacks here is hard.

Also, in looking at the code, how can this ever be a short read?  You
are using memory_read_from_buffer() which unless the values passed into
it are incorrect, will always return the expected read amount.

> > And what will userspace do with this error message in the kernel log?
> 
> User space currently is not seeing this error for nvmem device/eeprom
> reads due to the following code at nvmem/core.c in
> bin_attr_nvmem_read():
> "
>     rc = nvmem_reg_read(nvmem, pos, buf, count);
> 
>     if (rc)
>         return rc;
> 
>     return count;
> "
> since it expects to return the number of bytes.
> 
> Userspace will see a false error with nvmem cell reads from
> nvmem_cell_attr_read() in current code, which should be fixed on
> returning 0 for success.

So maybe fix this all up to allow the read to return the actual amount
read?  That feels more "correct" to me.

thanks,

greg k-h