1. Patchew
  2. linux
  3. [PATCH RFC 00/33] Compile-time thread-safety checking
  4. View patch

[PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value

Bart Van Assche posted 33 patches 10 months, 1 week ago
[PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Bart Van Assche 10 months, 1 week ago 
Return early in it87_resume() if it87_lock() fails instead of ignoring the
return value of that function. This patch suppresses a Clang thread-safety
warning.

Cc: Frank Crawford <frank@crawford.emu.id.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Fixes: 376e1a937b30 ("hwmon: (it87) Add calls to smbus_enable/smbus_disable as required")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/hwmon/it87.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e233aafa8856..8e3935089fca 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -3593,7 +3593,9 @@ static int it87_resume(struct device *dev)
 
 	it87_resume_sio(pdev);
 
-	it87_lock(data);
+	int err = it87_lock(data);
+	if (err)
+		return err;
 
 	it87_check_pwm(dev);
 	it87_check_limit_regs(data);
Re: [PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Guenter Roeck 10 months, 1 week ago 
On 2/6/25 09:51, Bart Van Assche wrote:
> Return early in it87_resume() if it87_lock() fails instead of ignoring the
> return value of that function. This patch suppresses a Clang thread-safety
> warning.
> 
> Cc: Frank Crawford <frank@crawford.emu.id.au>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Fixes: 376e1a937b30 ("hwmon: (it87) Add calls to smbus_enable/smbus_disable as required")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/hwmon/it87.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index e233aafa8856..8e3935089fca 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -3593,7 +3593,9 @@ static int it87_resume(struct device *dev)
>   
>   	it87_resume_sio(pdev);
>   
> -	it87_lock(data);
> +	int err = it87_lock(data);

I am not going to accept patches with inline variable declarations
if the patch is fixing an earlier problem, sorry. This only results
in unnecessary backport failures.

Guenter
Re: [PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Christoph Hellwig 10 months, 1 week ago 
On Thu, Feb 06, 2025 at 02:51:59PM -0800, Guenter Roeck wrote:
>>   -	it87_lock(data);
>> +	int err = it87_lock(data);
>
> I am not going to accept patches with inline variable declarations
> if the patch is fixing an earlier problem, sorry. This only results
> in unnecessary backport failures.

In fact the inline variable declarations should never be used in
"normal" code by Linux coding standards.  The only reason the
warning is disabled is for some of the magic scoping macros.
Re: [PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Bart Van Assche 10 months, 1 week ago 
On 2/6/25 2:51 PM, Guenter Roeck wrote:
> On 2/6/25 09:51, Bart Van Assche wrote:
>> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
>> index e233aafa8856..8e3935089fca 100644
>> --- a/drivers/hwmon/it87.c
>> +++ b/drivers/hwmon/it87.c
>> @@ -3593,7 +3593,9 @@ static int it87_resume(struct device *dev)
>>       it87_resume_sio(pdev);
>> -    it87_lock(data);
>> +    int err = it87_lock(data);
> 
> I am not going to accept patches with inline variable declarations
> if the patch is fixing an earlier problem, sorry. This only results
> in unnecessary backport failures.

Hi Guenter,

Is this perhaps how you want me to format this patch?

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e233aafa8856..5cfb98a0512f 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -3590,10 +3590,13 @@ static int it87_resume(struct device *dev)
  {
         struct platform_device *pdev = to_platform_device(dev);
         struct it87_data *data = dev_get_drvdata(dev);
+       int err;

         it87_resume_sio(pdev);

-       it87_lock(data);
+       err = it87_lock(data);
+       if (err)
+               return err;

         it87_check_pwm(dev);
         it87_check_limit_regs(data);

Thanks,

Bart.
Re: [PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Guenter Roeck 10 months, 1 week ago 
On 2/6/25 15:34, Bart Van Assche wrote:
> On 2/6/25 2:51 PM, Guenter Roeck wrote:
>> On 2/6/25 09:51, Bart Van Assche wrote:
>>> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
>>> index e233aafa8856..8e3935089fca 100644
>>> --- a/drivers/hwmon/it87.c
>>> +++ b/drivers/hwmon/it87.c
>>> @@ -3593,7 +3593,9 @@ static int it87_resume(struct device *dev)
>>>       it87_resume_sio(pdev);
>>> -    it87_lock(data);
>>> +    int err = it87_lock(data);
>>
>> I am not going to accept patches with inline variable declarations
>> if the patch is fixing an earlier problem, sorry. This only results
>> in unnecessary backport failures.
> 
> Hi Guenter,
> 
> Is this perhaps how you want me to format this patch?
> 

Yes, that would be ok.

Guenter

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index e233aafa8856..5cfb98a0512f 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -3590,10 +3590,13 @@ static int it87_resume(struct device *dev)
>   {
>          struct platform_device *pdev = to_platform_device(dev);
>          struct it87_data *data = dev_get_drvdata(dev);
> +       int err;
> 
>          it87_resume_sio(pdev);
> 
> -       it87_lock(data);
> +       err = it87_lock(data);
> +       if (err)
> +               return err;
> 
>          it87_check_pwm(dev);
>          it87_check_limit_regs(data);
> 
> Thanks,
> 
> Bart.
Re: [PATCH RFC 29/33] hwmon: (it87) Check the it87_lock() return value
Posted by Frank Crawford 10 months, 1 week ago 
Bart,

On Thu, 2025-02-06 at 15:41 -0800, Guenter Roeck wrote:
> On 2/6/25 15:34, Bart Van Assche wrote:
> > On 2/6/25 2:51 PM, Guenter Roeck wrote:
> > > On 2/6/25 09:51, Bart Van Assche wrote:
> > > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > > > index e233aafa8856..8e3935089fca 100644
> > > > --- a/drivers/hwmon/it87.c
> > > > +++ b/drivers/hwmon/it87.c
> > > > @@ -3593,7 +3593,9 @@ static int it87_resume(struct device
> > > > *dev)
> > > >       it87_resume_sio(pdev);
> > > > -    it87_lock(data);
> > > > +    int err = it87_lock(data);
> > > 
> > > I am not going to accept patches with inline variable
> > > declarations
> > > if the patch is fixing an earlier problem, sorry. This only
> > > results
> > > in unnecessary backport failures.
> > 
> > Hi Guenter,
> > 
> > Is this perhaps how you want me to format this patch?
> > 
> 
> Yes, that would be ok.
> 
> Guenter
> 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index e233aafa8856..5cfb98a0512f 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -3590,10 +3590,13 @@ static int it87_resume(struct device *dev)
> >   {
> >          struct platform_device *pdev = to_platform_device(dev);
> >          struct it87_data *data = dev_get_drvdata(dev);
> > +       int err;
> > 
> >          it87_resume_sio(pdev);
> > 
> > -       it87_lock(data);
> > +       err = it87_lock(data);
> > +       if (err)
> > +               return err;
> > 
> >          it87_check_pwm(dev);
> >          it87_check_limit_regs(data);
> > 

Thanks for that (and to Guenter to get it correctly formatted).

I will check it doesn't cause any other problems, but given the
location, I think it should be all fine.

> > Thanks,
> > 
> > Bart.
> 
Regards
Frank

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.