* [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address
@ 2023-06-30 13:17 Rick Wertenbroek
2023-07-03 2:05 ` Damien Le Moal
0 siblings, 1 reply; 3+ messages in thread
From: Rick Wertenbroek @ 2023-06-30 13:17 UTC (permalink / raw)
To: alberto.dassatti
Cc: dlemoal, Rick Wertenbroek, Dan Carpenter, stable, Shawn Lin,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Heiko Stuebner, linux-pci, linux-rockchip,
linux-arm-kernel, linux-kernel
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@linaro.org>
Closes: https://lore.kernel.org/linux-pci/8d19e5b7-8fa0-44a4-90e2-9bb06f5eb694@moroto.mountain/
Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Cc: stable@vger.kernel.org
---
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);
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) */
--
2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address
2023-06-30 13:17 [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address Rick Wertenbroek
@ 2023-07-03 2:05 ` Damien Le Moal
2023-07-03 8:54 ` Rick Wertenbroek
0 siblings, 1 reply; 3+ messages in thread
From: Damien Le Moal @ 2023-07-03 2:05 UTC (permalink / raw)
To: Rick Wertenbroek, alberto.dassatti
Cc: Dan Carpenter, stable, Shawn Lin, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel,
linux-kernel
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@linaro.org>
> Closes: https://lore.kernel.org/linux-pci/8d19e5b7-8fa0-44a4-90e2-9bb06f5eb694@moroto.mountain/
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address
2023-07-03 2:05 ` Damien Le Moal
@ 2023-07-03 8:54 ` Rick Wertenbroek
0 siblings, 0 replies; 3+ messages in thread
From: Rick Wertenbroek @ 2023-07-03 8:54 UTC (permalink / raw)
To: Damien Le Moal
Cc: Dan Carpenter, stable, Shawn Lin, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Heiko Stuebner, linux-pci, linux-rockchip, linux-arm-kernel,
linux-kernel
On Mon, Jul 3, 2023 at 4:05 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> 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@linaro.org>
> > Closes: https://lore.kernel.org/linux-pci/8d19e5b7-8fa0-44a4-90e2-9bb06f5eb694@moroto.mountain/
> > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 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.
Indeed this would make the patch much simpler.
I will prepare and send the v2. Thank you for the suggestions
Regards,
Rick
>
> > 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-03 8:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-30 13:17 [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address Rick Wertenbroek
2023-07-03 2:05 ` Damien Le Moal
2023-07-03 8:54 ` Rick Wertenbroek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®