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@xxxxxxxxxxxxxxxx>
> > ---
> > 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 |
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
