Message-ID: <7e85a7fc-46d9-ae6a-25b1-5554e49a8f76@roeck-us.net>
Date:   Tue, 15 Mar 2022 08:57:20 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Agathe Porte <agathe.porte@...ia.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
Cc:     Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Adamski, Krzysztof (Nokia - PL/Wroclaw)" 
        <krzysztof.adamski@...ia.com>
Subject: Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and
 TMP468

Hi Agathe,

On 3/15/22 06:03, Agathe Porte wrote:
> Hi Guenter,
> 
> Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit :
>> If of_property_read_string() returns an error, it will not set the pointer
>> to &data->channel[channel].label, which by default is NULL because the
>> data structure was allocated with devm_kzalloc(). That means tmp464_is_visible()
>> will disable the label attribute. I don't see a problem with the current
>> code.
> 
> Thanks for the explanation. I agree that there is no problem on this point.
> 
>> There are lots of examples in the kernel where the return value from
>> of_property_read_string() is silently ignored. Not a single one of
>> those uses a (void) typecast. I don't really want to start making
>> such changes just to make static analyzers happy.
> 
> I have to disagree here. Because something has always (not) be done in the past should not be a reason to (not) do it in the future out of pure habit. I did not suggest to add the (void) casts in existing code: I agree it would be a burden with no real added value.
> 
> But making static analyzers happy seems justified *for new code*. It also makes *other developers* more confident, because with the cast we are sure that not checking the return value is very intentional.
> 
> Please enlighten me if there are any downsides that I did not think of and that would block this one-line change.
> 

Changing the code now would require either a separate patch or
a rebase of the hwmon-next tree. Rebasing the hwmon-next tree
at this point of the release cycle (a few days before the commit
window opens) is something I really don't want to do, leaving the
option to add a separate patch for the change. That makes it
identical to changing existing code to add the (void).

In addition to that, I do not agree that adding (void) really
adds value here; it just says "this is done on purpose" because
the static analyzer doesn't know better. 0-day stopped reporting
this kind of perceived problem, presumably for good reason.
Since the result of the function call is implied in setting or not
setting the passed pointer, a return value check or adding (void)
is not warranted. This would be different if the property was mandatory,
but that is not the case here.

There are lots of other functions in the kernel where return values
are not checked, for a variety of reasons. Functions where checking
the return value is necessary/mandatory are tagged with __must_check.
For others it is left to the caller to decide if a return value
should be checked, and if it makes sense / adds value to add (void).

I'll give you another example: cancel_work_sync() and related functions.
I am sure your static analyzer will complain about the failure to check
its return value in almost all cases. A counter-example is, say,
platform_driver_register(), where the return value should really be
checked and a (void) typecast should be used if it is not checked on
purpose. The problem is that static analyzers can not determine if
the return value check is necessary, and should either leave it alone
or make reports conditional on some command line option.

Overall we'll have to agree to disagree.

Thanks,
Guenter

Powered by blists - more mailing lists