Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling

[Dan Carpenter]

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