On 6/30/23 22:17, Rick Wertenbroek wrote:
> A 32-bit mask was used on the 64-bit PCI address used for mapping MSIs.
> This would result in the upper 32 bits being unintentionally zeroed and
> MSIs getting mapped to incorrect PCI addresses if the address had any
> of the upper bits set.
>
> Replace 32-bit mask by appropriate 64-bit mask and rename 32-bit mask
> for clarity.
>
> Fixes: dc73ed0f1b8b ("PCI: rockchip: Fix window mapping and address translation for endpoint")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-pci/8d19e5b7-8fa0-44a4-90e2-9bb06f5eb694@moroto.mountain/
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 12 ++++++------
> drivers/pci/controller/pcie-rockchip.h | 6 +++---
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 0af0e965fb57..313face6a87f 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -354,7 +354,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> struct rockchip_pcie *rockchip = &ep->rockchip;
> u32 flags, mme, data, data_mask;
> u8 msi_count;
> - u64 pci_addr;
> + u64 pci_addr, pci_addr_mask = GENMASK(63, 8);
I think you can simplify all this with only the change to the definition of
PCIE_ADDR_MASK macro. Applying a 64 bits mask for low bits to a 32-bits variable
is OK and should not generate any complaints from the compiler.
Also, that "8" in the GENMASK can be replaced by MIN_AXI_ADDR_BITS_PASSED.
So:
#define PCIE_ADDR_MASK GENMASK(63, MIN_AXI_ADDR_BITS_PASSED)
Would fix everything I think.
If it does not, I would prefer you define a macro for that GENMASK(63, 8) mask
instead of using a variable on stack. That would be more efficient code wise,
removing a memory access.
> u32 r;
>
> /* Check MSI enable bit */
> @@ -391,18 +391,18 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> PCI_MSI_ADDRESS_LO);
>
> /* Set the outbound region if needed. */
> - if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> + if (unlikely(ep->irq_pci_addr != (pci_addr & pci_addr_mask) ||
> ep->irq_pci_fn != fn)) {
> r = rockchip_ob_region(ep->irq_phys_addr);
> rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> ep->irq_phys_addr,
> - pci_addr & PCIE_ADDR_MASK,
> - ~PCIE_ADDR_MASK + 1);
> - ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> + pci_addr & pci_addr_mask,
> + ~pci_addr_mask + 1);
> + ep->irq_pci_addr = (pci_addr & pci_addr_mask);
> ep->irq_pci_fn = fn;
> }
>
> - writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> + writew(data, ep->irq_cpu_addr + (pci_addr & ~pci_addr_mask));
> return 0;
> }
>
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index fe0333778fd9..2d7b05f07b7e 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -158,11 +158,11 @@
> #define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)
> #define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)
>
> -#define PCIE_ADDR_MASK 0xffffff00
> +#define PCIE_LO_ADDR_MASK GENMASK(31, 8)
> #define PCIE_CORE_AXI_CONF_BASE 0xc00000
> #define PCIE_CORE_OB_REGION_ADDR0 (PCIE_CORE_AXI_CONF_BASE + 0x0)
> #define PCIE_CORE_OB_REGION_ADDR0_NUM_BITS 0x3f
> -#define PCIE_CORE_OB_REGION_ADDR0_LO_ADDR PCIE_ADDR_MASK
> +#define PCIE_CORE_OB_REGION_ADDR0_LO_ADDR PCIE_LO_ADDR_MASK
> #define PCIE_CORE_OB_REGION_ADDR1 (PCIE_CORE_AXI_CONF_BASE + 0x4)
> #define PCIE_CORE_OB_REGION_DESC0 (PCIE_CORE_AXI_CONF_BASE + 0x8)
> #define PCIE_CORE_OB_REGION_DESC1 (PCIE_CORE_AXI_CONF_BASE + 0xc)
> @@ -170,7 +170,7 @@
> #define PCIE_CORE_AXI_INBOUND_BASE 0xc00800
> #define PCIE_RP_IB_ADDR0 (PCIE_CORE_AXI_INBOUND_BASE + 0x0)
> #define PCIE_CORE_IB_REGION_ADDR0_NUM_BITS 0x3f
> -#define PCIE_CORE_IB_REGION_ADDR0_LO_ADDR PCIE_ADDR_MASK
> +#define PCIE_CORE_IB_REGION_ADDR0_LO_ADDR PCIE_LO_ADDR_MASK
> #define PCIE_RP_IB_ADDR1 (PCIE_CORE_AXI_INBOUND_BASE + 0x4)
>
> /* Size of one AXI Region (not Region 0) */
--
Damien Le Moal
Western Digital Research
