On Tue, Nov 10, 2020 at 09:29:45PM +0100, Thierry Reding wrote:
> > + err = dev_pm_opp_of_add_table(dc->dev);
> > + if (err) {
> > + dev_err(dc->dev, "failed to add OPP table: %d\n", err);
> > + goto put_hw;
> > + }
> > +
> > + err = devm_add_action(dc->dev, tegra_dc_deinit_opp_table, dc);
> > + if (err)
> > + goto remove_table;
>
> Do these functions return positive values? If not, I'd prefer if this
> check was more explicit (i.e. err < 0) for consistency with the rest of
> this code.
>
Isn't it the other way around? It's only when the check is explicitly
for "if (ret < 0)" that we have to wonder about positives. If the codes
says "if (ret)" then we know that it doesn't return positive values and
every non-zero is an error.
In the kernel "if (ret)" is way more popular than "if (ret < 0)":
$ git grep 'if (\(ret\|rc\|err\))' | wc -l
92927
$ git grep 'if (\(ret\|rc\|err\) < 0)' | wc -l
36577
And some of those are places where "ret" can be positive so we are
forced to use the "if (ret < 0)" format.
Checking for "if (ret)" is easier from a static analysis perspective.
If it's one style is used consistently then they're the same but when
there is a mismatch the "if (ret < 0) " will trigger a false positive
and the "if (ret) " will not.
int var;
ret = frob(&var);
if (ret < 0)
return ret;
Smatch thinks positive returns are not handled so it complains that
"var can be uninitialized".
regards,
dan carpenter
