Messages in this thread | | | Date | Wed, 11 Nov 2020 12:28:32 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling |
| |
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
| |