On Wed, Dec 27, 2023 at 07:03:41AM -0600, Bjorn Helgaas wrote:
> 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.
>
Agree. I'm planning to switch to genpoll/genalloc instead of using a custom
memory allocation scheme in pci-epc-mem.c. Will try to address this issue also.\
Thanks for spotting!
- Mani
> 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
--
மணிவண்ணன் சதாசிவம்
