On Wed, Dec 27, 2023 at 12:57:31PM +0100, Niklas Cassel wrote:
> 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
Yes, of course. I was writing the merge commit log for merging that
branch into the PCI "next" branch.
> ...
> > > @@ -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);
Right, that's the connection I mentioned as "far away in
dw_pcie_ep_init()". It's not the usual pattern of "map X, write X".
Here we have "map X, write Y", and it's up to the reader to do the
research to figure out whether and how X and Y are related.
> > 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/
This totally seems non-obvious and scary, regardless of Yury's patch.
If we're relying on the mem->bitmap to permanently assign an iATU
index for ep->msi_mem, it's not obvious why we need to use
dw_pcie_ep_map_addr() to enable that address each time we need it.
But all this is completely unrelated to your patch, which is fine and
now in -next (well, it *will* be the next time there is a linux-next
release, which looks like Jan 2 or so).
Bjorn
