On Wed, Nov 21, 2018 at 10:16:58AM +0100, Lucas Stach wrote:
> Am Dienstag, den 20.11.2018, 21:07 +0100 schrieb Oleksij Rempel:
> > The code is correct but it takes more seconds for me to understand.
> > And static code analyzer do not understand it at all.
> > 
> > > Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> > ---
> >  drivers/pinctrl/pinctrl-tegra30.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/pinctrl-tegra30.c b/drivers/pinctrl/pinctrl-tegra30.c
> > index d9b49c57d..ffb04eebb 100644
> > --- a/drivers/pinctrl/pinctrl-tegra30.c
> > +++ b/drivers/pinctrl/pinctrl-tegra30.c
> > @@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,
> > >  			break;
> > >  		}
> > >  	}
> > > -	/* if no matching drivegroup is found */
> > > -	if (i == ctrl->drvdata->num_drvgrps)
> > +
> > > +	if (!group)
> > >  		return 0;
> 
> Huh? This is a pretty standard idiom in C codebases to check if we
> broke out of a loop early.
> 
> Actually this change breaks the code, as this check is inside of an
> outer loop that doesn't reinitialize the group variable. So while the
> code as-is correctly checks if a group was found in the current
> iteration of the outer loop, after this patch it also matches a group
> that was found on a previous iteration of the outer loop.

Probably I still do not understand it:

static const struct pinctrl_tegra30_drvdata tegra124_drvdata = {
	.pingrps = tegra124_pin_groups,
	.num_pingrps = ARRAY_SIZE(tegra124_pin_groups),
	.drvgrps = tegra124_drive_groups,
	.num_drvgrps = ARRAY_SIZE(tegra124_drive_groups), 
	^^^^^ this is constant.
};

static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,
                                        struct device_node *np)
{
	const char *pins = NULL;
	const struct tegra_drive_pingroup *group = NULL;
	^^^^ here we init *group to NULL

	int hsm = -1, schmitt = -1, pds = -1, pus = -1, srr = -1, srf = -1;
	int i;
	u32 __iomem *regaddr;
	u32 val;

	if (of_property_read_string(np, "nvidia,pins", &pins))
		return 0;

	for (i = 0; i < ctrl->drvdata->num_drvgrps; i++) {
	^^^^ here we init i
		if (!strcmp(pins, ctrl->drvdata->drvgrps[i].name)) {
			group = &ctrl->drvdata->drvgrps[i];
			^^^^^ -- only here group is not NULL
			break;
		}
	}
	/* if no matching drivegroup is found */
	if (i == ctrl->drvdata->num_drvgrps)
	^^^^^ if i == num_drvgrps, group is also NULL..
		return 0;

I don't see any technical problems. Or i do oversee some thing?

> This is a prime example where static checker warnings can prompt wrong
> fixes. Frankly codacy should smart up to correctly analyze the
> controlflow interdependency.

Well, amount of real problems found by this code check is still higher.
So why not to use it?

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |