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 <[email protected]> > > --- > > 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 |
signature.asc
Description: PGP signature
_______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
