linux-kernel - Re: [PATCH 17/20] PCI: dw-rockchip: switch to HWORD

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]
Message-ID: <12129790.nUPlyArG6x@workhorse>
Date: Fri, 13 Jun 2025 14:08:08 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Yury Norov <yury.norov@...il.com>,
 Rasmus Villemoes <linux@...musvillemoes.dk>,
 Jaehoon Chung <jh80.chung@...sung.com>, Ulf Hansson <ulf.hansson@...aro.org>,
 Heiko Stuebner <heiko@...ech.de>,
 Shreeya Patel <shreeya.patel@...labora.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Sandy Huang <hjc@...k-chips.com>,
 Andy Yan <andy.yan@...k-chips.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Nicolas Frattaroli <frattaroli.nicolas@...il.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Shawn Lin <shawn.lin@...k-chips.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Chanwoo Choi <cw00.choi@...sung.com>,
 MyungJoo Ham <myungjoo.ham@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>, Qin Jian <qinjian@...lus1.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 kernel@...labora.com, linux-kernel@...r.kernel.org,
 linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-phy@...ts.infradead.org,
 linux-sound@...r.kernel.org, netdev@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com, linux-pci@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-clk@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH 17/20] PCI: dw-rockchip: switch to HWORD_UPDATE macro

Hello,

On Friday, 13 June 2025 11:45:22 Central European Summer Time Niklas Cassel wrote:
> Hello Nicolas,
> 
> On Thu, Jun 12, 2025 at 08:56:19PM +0200, Nicolas Frattaroli wrote:
> > 
> > PCIE_CLIENT_RC_MODE/PCIE_CLIENT_EP_MODE was another field that wasn't
> > super clear on what the bit field modification actually is. As far as I
> > can tell, switching to RC mode doesn't actually write the correct value
> > to the field if any of its bits have been set previously, as it only
> > updates one bit of a 4 bit field.
> > 
> > Replace it by actually writing the full values to the field, using the
> > new HWORD_UPDATE macro, which grants us the benefit of better
> > compile-time error checking.
> 
> The current code looks like this:
> #define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE_BIT(0x40)
> #define  PCIE_CLIENT_EP_MODE            HIWORD_UPDATE(0xf0, 0x0)
> 
> The device_type field is defined like this:
> 4'h0: PCI Express endpoint
> 4'h1: Legacy PCI Express endpoint
> 4'h4: Root port of PCI Express root complex
> 
> The reset value of the device_type field is 0x0 (EP mode).
> 
> So switching between RC mode / EP mode should be fine.
> 
> But I agree, theoretically there could be a bug if e.g. bootloader
> has set the device_type to 0x1 (Legacy EP).
> 
> So if you want, you could send a patch:
> -#define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE_BIT(0x40)
> +#define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE(0xf0, 0x40)
> 
> With:
> Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> 
> But I also think that your current patch is fine as-is.
> 
> I do however think that you can drop this line:
> +#define  PCIE_CLIENT_MODE_LEGACY       0x1U
> 
> Since the define is never used.

Will do

> 
> 
> Also, is there any point in adding the U suffix?
> 
> Usually you see UL or ULL suffix, when that is needed, but there actually
> seems to be extremely few hits of simply U suffix:
> $ git grep 0x1U | grep -v UL

Sort of. Literals without the U suffix are considered signed iirc, and
operating with them and then left-shifting the result can run into issues
if you shift their bits into the sign bit. In the patch at [1] I needed to
quell a compiler warning about signed long overflows with a U suffix. This
should only ever really be a problem for anything that gets shifted up to
bit index 31 I believe, and maybe there's a better way to handle this in
the macro itself with an explicit cast to unsigned, but explicit casts
give me the ick. I'm also open to changing it to an UL, which will have
the same effect, and has more precedent.

> 
> 
> Kind regards,
> Niklas
> 

Best Regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/all/20250612-byeword-update-v1-7-f4afb8f6313f@collabora.com/ [1]