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