[PATCH] rtc: abx80x: Fix return value of nvmem callback on read

[PATCH] rtc: abx80x: Fix return value of nvmem callback on read

[Joy Chakraborty]

Read callbacks registered with nvmem core expect 0 to be returned on
success and a negative value to be returned on failure.

abx80x_nvmem_xfer() on read calls i2c_smbus_read_i2c_block_data() which
returns the number of bytes read on success as per its api description,
this return value is handled as an error and returned to nvmem even on
success.

Fix to handle all possible values that would be returned by
i2c_smbus_read_i2c_block_data().

Fixes: e90ff8ede777 ("rtc: abx80x: Add nvmem support")
Cc: stable@vger.kernel.org
Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 drivers/rtc/rtc-abx80x.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
index fde2b8054c2e..0f5847d1ca2a 100644
--- a/drivers/rtc/rtc-abx80x.c
+++ b/drivers/rtc/rtc-abx80x.c
@@ -711,9 +711,16 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
 		else
 			ret = i2c_smbus_read_i2c_block_data(priv->client, reg,
 							    len, val);
-		if (ret)
+		if (ret < 0)
 			return ret;
 
+		if (!write) {
+			if (ret)
+				len = ret;
+			else
+				return -EIO;
+		}
+
 		offset += len;
 		val += len;
 		bytes -= len;
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH] rtc: abx80x: Fix return value of nvmem callback on read

[Dan Carpenter]

On Wed, Jun 12, 2024 at 06:05:54PM +0000, Joy Chakraborty wrote:
> Read callbacks registered with nvmem core expect 0 to be returned on
> success and a negative value to be returned on failure.
> 
> abx80x_nvmem_xfer() on read calls i2c_smbus_read_i2c_block_data() which
> returns the number of bytes read on success as per its api description,
> this return value is handled as an error and returned to nvmem even on
> success.
> 
> Fix to handle all possible values that would be returned by
> i2c_smbus_read_i2c_block_data().
> 
> Fixes: e90ff8ede777 ("rtc: abx80x: Add nvmem support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> ---
>  drivers/rtc/rtc-abx80x.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index fde2b8054c2e..0f5847d1ca2a 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -711,9 +711,16 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
>  		else
>  			ret = i2c_smbus_read_i2c_block_data(priv->client, reg,
>  							    len, val);
> -		if (ret)
> +		if (ret < 0)
>  			return ret;
>  
> +		if (!write) {
> +			if (ret)
> +				len = ret;
> +			else
> +				return -EIO;
> +		}

I guess this is the conservative approach.  Ie.  Don't break things
which aren't already broken.  But I suspect the correct approach is to
say:

	if (ret != len)
		return -EIO;

Ah well.  Being conservative is good.  It probably doesn't ever happen
in real life so it probably doesn't matter either way.

I don't really like the if (write) follow by and if (!write)...  It
would add more lines, but improve readability if we just duplicate the
code a big:

	if (write) {
		ret = write();
		if (ret)
			return ret;
	} else {
		ret = read();
		if (ret <= 0)
			return ret ?: -EIO;
		len = ret;
	}

regards,
dan carpenter

Re: [PATCH] rtc: abx80x: Fix return value of nvmem callback on read

[Joy Chakraborty]

On Thu, Jun 13, 2024 at 11:48 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Jun 12, 2024 at 06:05:54PM +0000, Joy Chakraborty wrote:
> > Read callbacks registered with nvmem core expect 0 to be returned on
> > success and a negative value to be returned on failure.
> >
> > abx80x_nvmem_xfer() on read calls i2c_smbus_read_i2c_block_data() which
> > returns the number of bytes read on success as per its api description,
> > this return value is handled as an error and returned to nvmem even on
> > success.
> >
> > Fix to handle all possible values that would be returned by
> > i2c_smbus_read_i2c_block_data().
> >
> > Fixes: e90ff8ede777 ("rtc: abx80x: Add nvmem support")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  drivers/rtc/rtc-abx80x.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> > index fde2b8054c2e..0f5847d1ca2a 100644
> > --- a/drivers/rtc/rtc-abx80x.c
> > +++ b/drivers/rtc/rtc-abx80x.c
> > @@ -711,9 +711,16 @@ static int abx80x_nvmem_xfer(struct abx80x_priv *priv, unsigned int offset,
> >               else
> >                       ret = i2c_smbus_read_i2c_block_data(priv->client, reg,
> >                                                           len, val);
> > -             if (ret)
> > +             if (ret < 0)
> >                       return ret;
> >
> > +             if (!write) {
> > +                     if (ret)
> > +                             len = ret;
> > +                     else
> > +                             return -EIO;
> > +             }
>
> I guess this is the conservative approach.  Ie.  Don't break things
> which aren't already broken.  But I suspect the correct approach is to
> say:
>
>         if (ret != len)
>                 return -EIO;
>
> Ah well.  Being conservative is good.  It probably doesn't ever happen
> in real life so it probably doesn't matter either way.
>
> I don't really like the if (write) follow by and if (!write)...  It
> would add more lines, but improve readability if we just duplicate the
> code a big:
>
>         if (write) {
>                 ret = write();
>                 if (ret)
>                         return ret;
>         } else {
>                 ret = read();
>                 if (ret <= 0)
>                         return ret ?: -EIO;
>                 len = ret;
>         }
>

Sure, I'll do this in a follow up patch.

Thanks
Joy
> regards,
> dan carpenter
>