Hello Bjorn,
On Tue, Dec 26, 2023 at 04:17:14PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 28, 2023 at 02:22:30PM +0100, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@xxxxxxx>
> >
> > Commit 6f5e193bfb55 ("PCI: dwc: Fix dw_pcie_ep_raise_msix_irq() to get
> > correct MSI-X table address") modified dw_pcie_ep_raise_msix_irq() to
> > support iATUs which require a specific alignment.
> >
> > However, this support cannot have been properly tested.
> >
> > The whole point is for the iATU to map an address that is aligned,
> > using dw_pcie_ep_map_addr(), and then let the writel() write to
> > ep->msi_mem + aligned_offset.
> >
> > Thus, modify the address that is mapped such that it is aligned.
> > With this change, dw_pcie_ep_raise_msix_irq() matches the logic in
> > dw_pcie_ep_raise_msi_irq().
For the record, this patch is already queued up:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/dwc
>
> Was there a problem report for this? Since 6f5e193bfb55 appeared in
> v5.7 (May 31 2020), and this should affect imx6, keystone am654,
> dw-pcie (platform), and keembay, it seems a little weird that this bug
> persisted so long. Maybe nobody really uses endpoint support yet?
>
> But I assume you observed a failure and tested this fix somewhere.
Yes, I verified it on rockchip rk3588.
I'm working on upstreaming rk3588 EP support:
https://github.com/floatious/linux/commits/rockchip-pcie-ep
Right now rk3588 only has support for RC in mainline.
The fix is only needed for platforms which:
1) supports MSI-X
2) has an iATU alignment requirement,
so where epc->mem->window.page_size != 0.
pci_epc_mem_init() calls pci_epc_multi_mem_init() which
initializes epc->mem->window.page_size with ep->page_size.
$ git grep page_size drivers/pci/controller/dwc/
So it will not affect pcie-designware-plat.c, nor pcie-keembay.c,
since they don't set any ep->page_size.
It will not affect pcie-tegra194.c, since it doesn't use
dw_pcie_ep_raise_msix_irq().
Looking at pci-imx6.c, imx6_pcie_ep_raise_irq() calls
dw_pcie_ep_raise_msix_irq(), but:
static const struct pci_epc_features imx8m_pcie_epc_features = {
.msix_capable = false,
}
so while pci-imx6.c will call dw_pcie_ep_raise_msix_irq(),
I assume that it will return early, in this if-statement:
https://github.com/torvalds/linux/blob/v6.7-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L596-L598
That leaves just pci-keystone.c (am654 compatible only).
I don't know why no one has reported this bug before,
I can only assume insufficient testing.
I guess you might be lucky and happen to get an address that is
aligned to the iATU alignment requirement, but that is unlikely
to happen when rebooting and running pcitest.sh multiple times.
>
> And the failure is that we send the wrong MSI-X vector or something
> and get an unexpected IRQ and a driver hang or something? In other
> words, how does the bug manifest to users?
pcitest.sh fails the MSI-X tests.
With this fix the MSI-X tests in pcitest.sh passes.
>
> > Cc: Kishon Vijay Abraham I <kishon@xxxxxxxxxx>
> > Fixes: 6f5e193bfb55 ("PCI: dwc: Fix dw_pcie_ep_raise_msix_irq() to get correct MSI-X table address")
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > ---
> > Changes since v1:
> > -Clarified commit message.
> > -Add a working email for Kishon to CC.
> >
> > drivers/pci/controller/dwc/pcie-designware-ep.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f6207989fc6a..bc94d7f39535 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -615,6 +615,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> > }
> >
> > aligned_offset = msg_addr & (epc->mem->window.page_size - 1);
> > + msg_addr &= ~aligned_offset;
> > ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
> > epc->mem->window.page_size);
>
> Total tangent and I don't know enough to suggest any changes, but it's
> a little strange as a reader that we want to write to ep->msi_mem, and
> the ATU setup with dw_pcie_ep_map_addr() doesn't involve ep->msi_mem
> at all.
>
> I see that ep->msi_mem is allocated and ioremapped far away in
> dw_pcie_ep_init(). It's just a little weird that there's no
> connection *here* with ep->msi_mem.
There is a connection. dw_pcie_ep_raise_msix_irq() uses ep->msi_mem_phys,
which is the physical address of ep->msi_mem:
ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
epc->mem->window.page_size);
>
> I assume dw_pcie_ep_map_addr(), writel(), dw_pcie_ep_unmap_addr() have
> to happen atomically so nobody else uses that piece of the ATU while
> we're doing this? There's no mutex here, so I guess we must know this
> is atomic already because of something else?
Most devices have multiple iATUs (so multiple iATU indexes).
pcie-designware-ep.c:dw_pcie_ep_outbound_atu()
uses find_first_zero_bit() to make sure that a specific iATU (index)
is not reused for something else:
https://github.com/torvalds/linux/blob/v6.7-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L208
A specific iATU (index) is then freed by dw_pcie_ep_unmap_addr(),
which does a clear_bit() for that iATU (index).
It is a bit scary that there is no mutex or anything, since
find_first_zero_bit() is _not_ atomic, so if we have concurrent calls
to dw_pcie_ep_map_addr(), things might break, but that is a separate
issue.
I assume that this patch series will improve the concurrency issue,
if it gets accepted:
https://lore.kernel.org/linux-pci/20231212022749.625238-1-yury.norov@xxxxxxxxx/
Kind regards,
Niklas
