[PATCH] hwmon: (lm93) Return error values on read failure

[PATCH] hwmon: (lm93) Return error values on read failure

[Abhishek Tamboli]

Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
read failure after retries, which could be confused with valid data.

Address the TODO: what to return in case of error?

Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
---
 drivers/hwmon/lm93.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
index be4853fad80f..b76f3c1c6297 100644
--- a/drivers/hwmon/lm93.c
+++ b/drivers/hwmon/lm93.c
@@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
 static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
 {
 	int value, i;
+	int ret;
 
 	/* retry in case of read errors */
 	for (i = 1; i <= MAX_RETRIES; i++) {
@@ -808,14 +809,14 @@ static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
 			dev_warn(&client->dev,
 				 "lm93: read byte data failed, address 0x%02x.\n",
 				 reg);
+			ret = value;
 			mdelay(i + 3);
 		}
 
 	}
 
-	/* <TODO> what to return in case of error? */
 	dev_err(&client->dev, "lm93: All read byte retries failed!!\n");
-	return 0;
+	return ret;
 }
 
 static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
@@ -836,6 +837,7 @@ static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
 static u16 lm93_read_word(struct i2c_client *client, u8 reg)
 {
 	int value, i;
+	int ret;
 
 	/* retry in case of read errors */
 	for (i = 1; i <= MAX_RETRIES; i++) {
@@ -846,14 +848,14 @@ static u16 lm93_read_word(struct i2c_client *client, u8 reg)
 			dev_warn(&client->dev,
 				 "lm93: read word data failed, address 0x%02x.\n",
 				 reg);
+			ret = value;
 			mdelay(i + 3);
 		}
 
 	}
 
-	/* <TODO> what to return in case of error? */
 	dev_err(&client->dev, "lm93: All read word retries failed!!\n");
-	return 0;
+	return ret;
 }
 
 static int lm93_write_word(struct i2c_client *client, u8 reg, u16 value)
-- 
2.34.1

Re: [PATCH] hwmon: (lm93) Return error values on read failure

[Guenter Roeck]

On 8/7/24 11:17, Abhishek Tamboli wrote:
> Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> read failure after retries, which could be confused with valid data.
> 
> Address the TODO: what to return in case of error?
> 
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> ---
>   drivers/hwmon/lm93.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> index be4853fad80f..b76f3c1c6297 100644
> --- a/drivers/hwmon/lm93.c
> +++ b/drivers/hwmon/lm93.c
> @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
>   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)

This is still returning an u8.

>   {
>   	int value, i;
> +	int ret;
>   
>   	/* retry in case of read errors */
>   	for (i = 1; i <= MAX_RETRIES; i++) {
> @@ -808,14 +809,14 @@ static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
>   			dev_warn(&client->dev,
>   				 "lm93: read byte data failed, address 0x%02x.\n",
>   				 reg);
> +			ret = value;
>   			mdelay(i + 3);
>   		}
>   
>   	}
>   
> -	/* <TODO> what to return in case of error? */
>   	dev_err(&client->dev, "lm93: All read byte retries failed!!\n");

Those messages only make sense if there is no error return.

> -	return 0;
> +	return ret;

This is pointless and actually dangerous unless the calling code actually checks
the return value and aborts on error.



>   }
>   
>   static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
> @@ -836,6 +837,7 @@ static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
>   static u16 lm93_read_word(struct i2c_client *client, u8 reg)
>   {
>   	int value, i;
> +	int ret;
>   
>   	/* retry in case of read errors */
>   	for (i = 1; i <= MAX_RETRIES; i++) {
> @@ -846,14 +848,14 @@ static u16 lm93_read_word(struct i2c_client *client, u8 reg)
>   			dev_warn(&client->dev,
>   				 "lm93: read word data failed, address 0x%02x.\n",
>   				 reg);
> +			ret = value;
>   			mdelay(i + 3);
>   		}
>   
>   	}
>   
> -	/* <TODO> what to return in case of error? */
>   	dev_err(&client->dev, "lm93: All read word retries failed!!\n");
> -	return 0;
> +	return ret;

Same as above.

Actually, your patch makes the problem worse because the errors are still ignored
and at the same time report more or less random values to the user (the error code
truncated to an unsigned 8 or 16 bit value).

Is this just a blind patch, submitted as kind of an exercise, or do you have an
actual use case for this driver ? The driver is in such bad shape that it should
be left alone unless someone actually needs it and is able to test any changes.
Otherwise changes like this just increase risk (or, rather, make it even worse)
without real benefit.

Thanks,
Guenter

Re: [PATCH] hwmon: (lm93) Return error values on read failure

[Abhishek Tamboli]

On Wed, Aug 07, 2024 at 11:38:34AM -0700, Guenter Roeck wrote:
Hi Guenter,
Thank you for your feedback.
> On 8/7/24 11:17, Abhishek Tamboli wrote:
> > Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> > read failure after retries, which could be confused with valid data.
> > 
> > Address the TODO: what to return in case of error?
> > 
> > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> > ---
> >   drivers/hwmon/lm93.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> > index be4853fad80f..b76f3c1c6297 100644
> > --- a/drivers/hwmon/lm93.c
> > +++ b/drivers/hwmon/lm93.c
> > @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
> >   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
> 
> This is still returning an u8.
My interpretation of the TODO was to address the error condition while keeping the 
existing logic of the driver intact. I understand that this driver is 
old and that changes should be approached with caution.
> >   {
> >   	int value, i;
> > +	int ret;
> >   	/* retry in case of read errors */
> >   	for (i = 1; i <= MAX_RETRIES; i++) {
> > @@ -808,14 +809,14 @@ static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
> >   			dev_warn(&client->dev,
> >   				 "lm93: read byte data failed, address 0x%02x.\n",
> >   				 reg);
> > +			ret = value;
> >   			mdelay(i + 3);
> >   		}
> >   	}
> > -	/* <TODO> what to return in case of error? */
> >   	dev_err(&client->dev, "lm93: All read byte retries failed!!\n");
> 
> Those messages only make sense if there is no error return.
> 
> > -	return 0;
> > +	return ret;
> 
> This is pointless and actually dangerous unless the calling code actually checks
> the return value and aborts on error.
> 
> 
> 
> >   }
> >   static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
> > @@ -836,6 +837,7 @@ static int lm93_write_byte(struct i2c_client *client, u8 reg, u8 value)
> >   static u16 lm93_read_word(struct i2c_client *client, u8 reg)
> >   {
> >   	int value, i;
> > +	int ret;
> >   	/* retry in case of read errors */
> >   	for (i = 1; i <= MAX_RETRIES; i++) {
> > @@ -846,14 +848,14 @@ static u16 lm93_read_word(struct i2c_client *client, u8 reg)
> >   			dev_warn(&client->dev,
> >   				 "lm93: read word data failed, address 0x%02x.\n",
> >   				 reg);
> > +			ret = value;
> >   			mdelay(i + 3);
> >   		}
> >   	}
> > -	/* <TODO> what to return in case of error? */
> >   	dev_err(&client->dev, "lm93: All read word retries failed!!\n");
> > -	return 0;
> > +	return ret;
> 
> Same as above.
> 
> Actually, your patch makes the problem worse because the errors are still ignored
> and at the same time report more or less random values to the user (the error code
> truncated to an unsigned 8 or 16 bit value).
> 
> Is this just a blind patch, submitted as kind of an exercise, or do you have an
> actual use case for this driver ? 
This was not intended as a blind exercise. I aimed to make a meaningful improvement.
>The driver is in such bad shape that it should
> be left alone unless someone actually needs it and is able to test any changes.
> Otherwise changes like this just increase risk (or, rather, make it even worse)
> without real benefit.
I’m relatively new to kernel development, and I appreciate your insights on how this 
patch may have introduced additional issues rather than resolving the problem. 

I'll take your comments into account and Thank you for pointing out the mistakes.

Regards,
Abhishek

Re: [PATCH] hwmon: (lm93) Return error values on read failure

[Guenter Roeck]

On Thu, Aug 08, 2024 at 07:46:36AM +0530, Abhishek Tamboli wrote:
> On Wed, Aug 07, 2024 at 11:38:34AM -0700, Guenter Roeck wrote:
> Hi Guenter,
> Thank you for your feedback.
> > On 8/7/24 11:17, Abhishek Tamboli wrote:
> > > Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> > > read failure after retries, which could be confused with valid data.
> > > 
> > > Address the TODO: what to return in case of error?
> > > 
> > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> > > ---
> > >   drivers/hwmon/lm93.c | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> > > index be4853fad80f..b76f3c1c6297 100644
> > > --- a/drivers/hwmon/lm93.c
> > > +++ b/drivers/hwmon/lm93.c
> > > @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
> > >   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
> > 
> > This is still returning an u8.
> My interpretation of the TODO was to address the error condition while keeping the 
> existing logic of the driver intact. I understand that this driver is 
> old and that changes should be approached with caution.

Those TODOs are, at this point, pretty much pointless. If you want to help
with improving kernel code, it might be better to pick something from the
drivers/staging/ directory and help improve it.

The only thing that would really help for the lm93 driver would be a
complete overhaul, and that would only make sense if someone has real
hardware to test the resulting code; the driver is too complex to just
rely on unit tests. For example, the excessive retries might be because
the chip is really bad with its communication, or it may have been
observed on a system with a bad i2c controller, making it completely
unnecesssary today. Either case, if those retries are really necessary
due to chip issues, they should be hiddden behind regmap (which should
also be used to replace in-driver caching). And so on.

Really, if you want to get into kenrel development, it would be much
better to help improving code which is actually being used, as mentioned
above.

Thanks,
Guenter

Re: [PATCH] hwmon: (lm93) Return error values on read failure

[Abhishek Tamboli]

On Thu, Aug 08, 2024 at 09:54:40AM -0700, Guenter Roeck wrote:
> On Thu, Aug 08, 2024 at 07:46:36AM +0530, Abhishek Tamboli wrote:
> > On Wed, Aug 07, 2024 at 11:38:34AM -0700, Guenter Roeck wrote:
> > Hi Guenter,
> > Thank you for your feedback.
> > > On 8/7/24 11:17, Abhishek Tamboli wrote:
> > > > Fix the issue of lm93_read_byte() and lm93_read_word() return 0 on
> > > > read failure after retries, which could be confused with valid data.
> > > > 
> > > > Address the TODO: what to return in case of error?
> > > > 
> > > > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> > > > ---
> > > >   drivers/hwmon/lm93.c | 10 ++++++----
> > > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> > > > index be4853fad80f..b76f3c1c6297 100644
> > > > --- a/drivers/hwmon/lm93.c
> > > > +++ b/drivers/hwmon/lm93.c
> > > > @@ -798,6 +798,7 @@ static unsigned LM93_ALARMS_FROM_REG(struct block1_t b1)
> > > >   static u8 lm93_read_byte(struct i2c_client *client, u8 reg)
> > > 
> > > This is still returning an u8.
> > My interpretation of the TODO was to address the error condition while keeping the 
> > existing logic of the driver intact. I understand that this driver is 
> > old and that changes should be approached with caution.
> 
> Those TODOs are, at this point, pretty much pointless. If you want to help
> with improving kernel code, it might be better to pick something from the
> drivers/staging/ directory and help improve it.
> 
> The only thing that would really help for the lm93 driver would be a
> complete overhaul, and that would only make sense if someone has real
> hardware to test the resulting code; the driver is too complex to just
> rely on unit tests. For example, the excessive retries might be because
> the chip is really bad with its communication, or it may have been
> observed on a system with a bad i2c controller, making it completely
> unnecesssary today. Either case, if those retries are really necessary
> due to chip issues, they should be hiddden behind regmap (which should
> also be used to replace in-driver caching). And so on.
> 
> Really, if you want to get into kenrel development, it would be much
> better to help improving code which is actually being used, as mentioned
> above.

Hi Guenter,
Thank you for the feedback. I'll look into the drivers/staging directory
and see where I might be able to contribute effectively.

Regards,
Abhishek