Re: [PATCH v2] PCI: dwc: endpoint: Fix dw_pcie_ep_raise_msix_irq() alignment support

[Manivannan Sadhasivam <mani@kernel.org>]

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@wdc.com>
> > > > 
> > > > 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@gmail.com/
> 
> 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

-- 
மணிவண்ணன் சதாசிவம்