linux-kernel - Re: [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Dec 2018 21:22:46 -0800
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     helgaas@...nel.org
Cc:     linux-pci@...r.kernel.org, Fabio Estevam <fabio.estevam@....com>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Leonard Crestez <leonard.crestez@....com>,
        Dong Aisheng <aisheng.dong@....com>,
        Richard Zhu <hongxing.zhu@....com>, linux-imx@....com,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, gustavo@...eddedor.com
Subject: Re: [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ

On Fri, Dec 14, 2018 at 12:30 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> [+cc Gustavo for fallthrough annotation]
>
> On Wed, Dec 05, 2018 at 11:35:45PM -0800, Andrey Smirnov wrote:
> > Add code needed to support i.MX8MQ variant.
>
> > @@ -245,7 +253,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> >  {
> >       u32 tmp;
> >
> > -     if (imx6_pcie->variant == IMX7D)
> > +     if (imx6_pcie->variant == IMX7D ||
> > +         imx6_pcie->variant == IMX8MQ)
>
> This style looks like a maintenance problem: the code below is probably
> IMX6-specific, and you should test for *that* instead of adding to this
> list of things that are *not* IMX6, because that list is likely to
> continue growing.  There are more occurrences below.
>

Makes sense, I'll update that patches and send a v3 out.

> > @@ -301,6 +312,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >
> >       switch (imx6_pcie->variant) {
> >       case IMX7D:
> > +     case IMX8MQ: /* FALLTHROUGH */
> >               reset_control_assert(imx6_pcie->pciephy_reset);
> >               reset_control_assert(imx6_pcie->apps_reset);
> >               break;
>
> I'm not an expert on fallthrough annotation (Gustavo, cc'd, is), but
> this looks wrong.  It's the IMX7D case that falls through, not the
> IMX8MQ case.
>
> The recent annotations added by Gustavo are at the point where the
> "break" would normally be, e.g.,
>
>   case IMX7D:
>     /* fall through */                    <--- annotation
>   case IMX8MQ:
>     <code>
>     break;
>
> But in this case there's actually no IMX7D-specific *code* there, so I
> suspect the annotation is unnecessary.  It's obvious that IMX7D and
> IMX8MQ are handled the same, so there's really no opportunity for the
> "forgotten break" mistake -Wimplicit-fallthrough is trying to find.
>
> If we *do* want this annotation, we should spell it the same as
> Gustavo has been, i.e., "fall through".
>
> Again, more occurrences below.

Yes, definitely, same mistake of mine was already caught elsewhere in
the tree https://lore.kernel.org/lkml/20181214144406.0dbffbc8@canb.auug.org.au/

I'll fix it in v3.

Thanks,
Andrey Smirnov