Messages in this thread | | | From | Logan Gunthorpe <> | Date | Thu, 4 Jan 2018 16:06:11 -0700 | Subject | Re: [PATCH 01/12] pci-p2p: Support peer to peer memory |
| |
Thanks for the speedy review!
On 04/01/18 02:40 PM, Bjorn Helgaas wrote: > Run "git log --oneline drivers/pci" and follow the convention. I > think it would make sense to add a new tag like "PCI/P2P", although > "P2P" has historically also been used in the "PCI-to-PCI bridge" > context, so maybe there's something less ambiguous. "P2PDMA"?
Ok, I'll fix this for v2. I'm fine with renaming things to p2pdma
> When you add new files, I guess we're looking for the new SPDX > copyright stuff?
Will do.
> It's more than "non-trivial" or "with good performance" (from Kconfig > help), isn't it? AFAIK, there's no standard way at all to discover > whether P2P DMA is supported between root ports or RCs.
Yup, that's correct. This would have to be done with a white list.
> > s/bars/BARs/ (and similarly below, except in C code) > Similarly, s/dma/DMA/ and s/pci/PCI/ below. > And probably also s/p2p/peer-to-peer DMA/ in messages.
Will do.
> Maybe clarify this domain bit. Using "domain" suggests the common PCI > segment/domain usage, but I think you really mean something like the > part of the hierarchy where peer-to-peer DMA is guaranteed by the PCI > spec to work, i.e., anything below a single PCI bridge.
Ok, I like the wording you proposed.
> > Seems like there should be > > if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) > return -EINVAL; > > or similar here?
That sounds like a good idea. Will add.
> >> + if (WARN_ON(offset >= pci_resource_len(pdev, bar))) >> + return -EINVAL; > > Are these WARN_ONs for debugging purposes, or do you think we need > them in production? Granted, hitting it would probably be a kernel > driver bug, but still, not sure if the PCI core needs to coddle the > driver author that much.
Sure, I'll drop all the WARN_ONs.
> I'm guessing Christoph's dev_pagemap revamp repo must change > pgmap->res from a pointer to a structure, but I don't see the actual > link in your cover letter.
Sorry, the patch set is here:
https://www.mail-archive.com/linux-nvdimm@lists.01.org/msg07323.html
git://git.infradead.org/users/hch/misc.git hch/pgmap-cleanups.3
> I think you should set pgmap->res.flags here, too.
Sure, I don't think it's used and not set by the NVDIMM code; but I agree that it'd be a good idea to set it anyway.
>> + dev_info(&pdev->dev, "added %zdB of p2p memory\n", size); > > Can we add %pR and print pgmap->res itself, too?
Yup.
> You have a bit of a mix of PCI ("pci device", "bridge") and PCIe > ("switch", "switch port") terminology. I haven't read the rest of the > patches yet, so I don't know if you intend to restrict this to > PCIe-only, e.g., so you can use ACS, or if you want to make it > available on conventional PCI as well. > > If the latter, I would use the generic PCI terminology, i.e., "bridge" > instead of "switch".
Ok, I'll change it to use the generic term bridge. There's no restriction in the code to limit it to PCIe only, though I don't expect anybody will ever be using this with legacy PCI.
>> + * pci_virt_to_bus - return the pci bus address for a given virtual >> + * address obtained with pci_alloc_p2pmem >> + * @pdev: the device the memory was allocated from >> + * @addr: address of the memory that was allocated >> + */ >> +pci_bus_addr_t pci_p2pmem_virt_to_bus(struct pci_dev *pdev, void *addr) >> +{ >> + if (!addr) >> + return 0; >> + if (!pdev->p2p) >> + return 0; >> + >> + return gen_pool_virt_to_phys(pdev->p2p->pool, (unsigned long)addr); > > This doesn't seem right. A physical address is not the same as a PCI > bus address. I expected something like pci_bus_address() or > pcibios_resource_to_bus() here. Am I missing something? If so, a > clarifying comment would be helpful.
What you're missing is that when we called gen_pool_add_virt we used the PCI bus address as the physical address and not the CPU physical address (which we don't care about). I'll add a comment explaining this.
> I've been noticing that we're accumulating PCI-related files in > include/linux: pci.h, pci-aspm.h pci-ats.h, pci-dma.h, pcieport_if.h, > etc. I'm not sure there's value in all those and am thinking maybe > they should just be folded into pci.h. What do you think?
We started with that. Once we reached a certain amount of code, Christoph suggested we put it in its own header.
Logan
| |