[PATCH] PCI: mediatek: Fix operator precedence in PCIE_FTS_NUM_L0 macro

[PATCH] PCI: mediatek: Fix operator precedence in PCIE_FTS_NUM_L0 macro

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The original PCIE_FTS_NUM_L0(x) macro was buggy due to improper operator
precedence, where ((x) & 0xff << 8) was evaluated as ((x) & 0xff00).

Instead of just fixing the parentheses, use the standard FIELD_PREP()
macro. This makes the code more robust by automatically handling masks
and shifts, while also adding compile-time type and range checking to
ensure the value fits within PCIE_FTS_NUM_MASK.

Fixes: 637cfacae96f ("PCI: mediatek: Add MediaTek PCIe host controller support")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/pci/controller/pcie-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 7572252..00f0e29 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -61,7 +61,7 @@
 /* MediaTek specific configuration registers */
 #define PCIE_FTS_NUM		0x70c
 #define PCIE_FTS_NUM_MASK	GENMASK(15, 8)
-#define PCIE_FTS_NUM_L0(x)	((x) & 0xff << 8)
+#define PCIE_FTS_NUM_L0(x)	FIELD_PREP(PCIE_FTS_NUM_MASK, x)
 
 #define PCIE_FC_CREDIT		0x73c
 #define PCIE_FC_CREDIT_MASK	(GENMASK(31, 31) | GENMASK(28, 16))
-- 
2.9.4

Re: [PATCH] PCI: mediatek: Fix operator precedence in PCIE_FTS_NUM_L0 macro

[Krzysztof Wilczyński]

Hello,

> The original PCIE_FTS_NUM_L0(x) macro was buggy due to improper operator
> precedence, where ((x) & 0xff << 8) was evaluated as ((x) & 0xff00).
> 
> Instead of just fixing the parentheses, use the standard FIELD_PREP()
> macro. This makes the code more robust by automatically handling masks
> and shifts, while also adding compile-time type and range checking to
> ensure the value fits within PCIE_FTS_NUM_MASK.
> 
> Fixes: 637cfacae96f ("PCI: mediatek: Add MediaTek PCIe host controller support")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 7572252..00f0e29 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -61,7 +61,7 @@
>  /* MediaTek specific configuration registers */
>  #define PCIE_FTS_NUM		0x70c
>  #define PCIE_FTS_NUM_MASK	GENMASK(15, 8)
> -#define PCIE_FTS_NUM_L0(x)	((x) & 0xff << 8)
> +#define PCIE_FTS_NUM_L0(x)	FIELD_PREP(PCIE_FTS_NUM_MASK, x)

Aww.  Nice catch!  Thank you for fixing this!

Happy to offer:

  Reviewed-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

All the best,

	Krzysztof

Re: [PATCH] PCI: mediatek: Fix operator precedence in PCIE_FTS_NUM_L0 macro

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Fixing the macro changes the configured FTS number from 0 to 80, conflicting with the comment requiring 250 and potentially breaking L0s link recovery.
- [Low] Missing explicit inclusion of `<linux/bitfield.h>` for `FIELD_PREP()`.
--

commit be16aaf8543160340df684dc520cc762167effe3
Author: Li RongQing <lirongqing@baidu.com>

PCI: mediatek: Fix operator precedence in PCIE_FTS_NUM_L0 macro

The original PCIE_FTS_NUM_L0(x) macro was buggy due to improper operator
precedence, where ((x) & 0xff << 8) was evaluated as ((x) & 0xff00).

Instead of just fixing the parentheses, use the standard FIELD_PREP()
macro. This makes the code more robust by automatically handling masks
and shifts, while also adding compile-time type and range checking to
ensure the value fits within PCIE_FTS_NUM_MASK.

> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 75722524fe744..00f0e29183f2b 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -61,7 +61,7 @@
>  /* MediaTek specific configuration registers */
>  #define PCIE_FTS_NUM		0x70c
>  #define PCIE_FTS_NUM_MASK	GENMASK(15, 8)
> -#define PCIE_FTS_NUM_L0(x)	((x) & 0xff << 8)
> +#define PCIE_FTS_NUM_L0(x)	FIELD_PREP(PCIE_FTS_NUM_MASK, x)

Should this file explicitly include <linux/bitfield.h> for FIELD_PREP()?

While the file compiles on ARM/ARM64 due to implicit inclusion via
<linux/irq.h> and <asm/ptrace.h>, missing the explicit include can cause
build failures on other architectures like x86 during COMPILE_TEST.

Does this change introduce a regression in L0s link recovery by altering
the configured Fast Training Sequence (FTS) number?

The driver configures the FTS number here:

drivers/pci/controller/pcie-mediatek.c:mtk_pcie_startup_port() {
    ...
	/* configure RC FTS number to 250 when it leaves L0s */
	writel(PCIE_CONF_ADDR(PCIE_FTS_NUM, func, slot, 0),
	       pcie->base + PCIE_CFG_ADDR);
	val = readl(pcie->base + PCIE_CFG_DATA);
	val &= ~PCIE_FTS_NUM_MASK;
	val |= PCIE_FTS_NUM_L0(0x50);
    ...
}

Before this patch, the macro evaluated to 0 because 0x50 & 0xff00 is 0.

This value of 0 might be safely interpreted by some hardware as 256,
satisfying the comment's requirement of 250.

By fixing the macro, the driver will now actively write 0x50 (80) to the
register instead of 0.

Since the comment explicitly requires 250, could reducing this to 80 cause
link recovery to fail?

Is 0xfa (250) the intended value here instead of 0x50?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515005552.2343-1-lirongqing@baidu.com?part=1