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 for Android: free password hash cracker in your pocket

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

Message-ID: <20181214203042.GC20725@google.com>
Date:   Fri, 14 Dec 2018 14:30:42 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Andrey Smirnov <andrew.smirnov@...il.com>
Cc:     linux-pci@...r.kernel.org, Fabio Estevam <fabio.estevam@....com>,
        cphealy@...il.com, l.stach@...gutronix.de,
        Leonard Crestez <leonard.crestez@....com>,
        "A.s. Dong" <aisheng.dong@....com>,
        Richard Zhu <hongxing.zhu@....com>, linux-imx@....com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Subject: Re: [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ

[+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.

> @@ -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.

Bjorn