linux-kernel - Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

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:   Thu, 1 Sep 2016 12:48:52 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Shawn Lin <shawn.lin@...k-chips.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Marc Zyngier <marc.zyngier@....com>, linux-pci@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        linux-rockchip@...ts.infradead.org,
        Heiko Stuebner <heiko@...ech.de>,
        Doug Anderson <dianders@...omium.org>,
        Wenrui Li <wenrui.li@...k-chips.com>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [v10,2/2] PCI: Rockchip: Add Rockchip PCIe controller support

On Thu, Sep 01, 2016 at 10:14:01AM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2016 at 11:34:55AM -0500, Bjorn Helgaas wrote:
> > In the interest of making progress, I made most of the changes Guenter
> > suggested (thanks very much, Guenter!) and made some more of my own on
> > top of those.
> 
> I'm not sure, but was this change your idea Bjorn?

Yep.

> commit 88cb3f59d73e261a3cd1957ff392f98c9916a5d1
> Author: Bjorn Helgaas <bhelgaas@...gle.com>
> Date:   Thu Sep 1 10:27:44 2016 -0500
> 
>     Simplify the confusing HIWORD_UPDATE scheme.
> 
> [Which I'll summarize here; you're replacing the macro:
> 
> -/*
> -  * The higher 16-bit of this register is used for write protection
> -  * only if BIT(x + 16) set to 1 the BIT(x) can be written.
> -  */
> -#define HIWORD_UPDATE(val, mask, shift) \
> -       ((val) << (shift) | (mask) << ((shift) + 16))
> 
> with its expansions:
> 
> +
> +/*
> + * The higher 16-bit of this register is used for write protection
> + * only if BIT(x + 16) set to 1 the BIT(x) can be written.
> + */
> +#define PCIE_CLIENT_CONF_ENABLE                (0x00010000 | 0x0001)
> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE  (0x00020000 | 0x0002)
> +#define PCIE_CLIENT_ARI_ENABLE         (0x00080000 | 0x0008)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)   (0x00300000 | (((x >> 1) & 3) << 4)
> +#define PCIE_CLIENT_MODE_RC            (0x00400000 | 0x0040)
> +#define PCIE_CLIENT_GEN_SEL(x)         (0x00800000 | ((x & 1) << 7)
> +#define  PCIE_CLIENT_GEN_SEL_0                 0
> +#define  PCIE_CLIENT_GEN_SEL_2                 1
> 
> Full change can be had by:
> git fetch git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/host-rockchip-wip
> ]
> 
> 
> The use of HIWORD_UPDATE can indeed be a bit confusing, IMO, but this is
> really a common Rockchip-ism that, once you read several of their
> drivers, can make a little more sense. If you grep around, it's in at
> least their clock, ethernet, SDHCI, PHY, and DP/DRM drivers. I might
> defer to Heiko (upstream maintainer of Rockchip code) for a decision.
> Maybe there's some intermediate ground where we keep the HIWORK_UPDATE()
> logic (it does make sure we get the 16-bit shift right, I think) while
> still refactoring a few other bits (like PCIE_CLIENT_CONF_LANE_NUM() and
> PCIE_CLIENT_GEN_SEL() for wrapping HIWORK_UPDATE()?).
> 
> Anyway, just a thought. If it really is most understandable to write it
> this way, then maybe it's fine to be different than the other 16
> rockchip files that have this pattern.

Hmm.  I did look at the other files, so I know this isn't a PCIe
special.  HIWORD_UPDATE is ugly as sin, especially in cases like this
where we're always writing a constant value and it takes three
#defines plus HIWORD_UPDATE itself, e.g.,

  -          HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE,
  -                        PCIE_CLIENT_CONF_ENABLE_MASK,
  -                        PCIE_CLIENT_CONF_ENABLE_SHIFT) |

vs this:

  +                   PCIE_CLIENT_CONF_ENABLE |

For these constant cases, a HIWORD_UPDATE that generated the
associated macro names would help readability a bit, though we'd still
need the ugly #defines:

  #define HIWORD_UPDATE_CONSTANT(x) (HIWORD_UPDATE(x, x ## _MASK, x ## _SHIFT)

  #define PCIE_CLIENT_CONF_ENABLE                0x1
  #define PCIE_CLIENT_CONF_ENABLE_SHIFT          0
  #define PCIE_CLIENT_CONF_ENABLE_MASK           0x1

             HIWORD_UPDATE_CONSTANT(PCIE_CLIENT_CONF_ENABLE) |