| Message ID | 20210311233142.7900-3-logang@deltatee.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to dma_map_sg for P2PDMA | expand |
On Thu, Mar 11, 2021 at 04:31:32PM -0700, Logan Gunthorpe wrote: > In order to use upstream_bridge_distance_warn() from a dma_map function, > it must not sleep. However, pci_get_slot() takes the pci_bus_sem so it > might sleep. > > In order to avoid this, try to get the host bridge's device from > bus->self, and if that is not set just get the first element in the > list. It should be impossible for the host bridges device to go away > while references are held on child devices, so the first element > should not change and this should be safe. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/pci/p2pdma.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index bd89437faf06..2135fe69bb07 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -311,11 +311,15 @@ static const struct pci_p2pdma_whitelist_entry { > static bool __host_bridge_whitelist(struct pci_host_bridge *host, > bool same_host_bridge) > { > - struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0)); > const struct pci_p2pdma_whitelist_entry *entry; > + struct pci_dev *root = host->bus->self; > unsigned short vendor, device; > > if (!root) > + root = list_first_entry_or_null(&host->bus->devices, > + struct pci_dev, bus_list); Replacing one ugliness (assuming there is a pci_dev for the host bridge, and that it is at 00.0) with another (still assuming a pci_dev and that it is host->bus->self or the first entry). I can't suggest anything better, but maybe a little comment in the code would help future readers. I wish we had a real way to discover this property without the whitelist, at least for future devices. Was there ever any interest in a _DSM or similar interface for this? I *am* very glad to remove a pci_get_slot() usage. > + > + if (!root || root->devfn) > return false; > > vendor = root->vendor; Don't you need to also remove the "pci_dev_put(root)" a few lines below?
On 2021-03-12 1:57 p.m., Bjorn Helgaas wrote: > On Thu, Mar 11, 2021 at 04:31:32PM -0700, Logan Gunthorpe wrote: >> In order to use upstream_bridge_distance_warn() from a dma_map function, >> it must not sleep. However, pci_get_slot() takes the pci_bus_sem so it >> might sleep. >> >> In order to avoid this, try to get the host bridge's device from >> bus->self, and if that is not set just get the first element in the >> list. It should be impossible for the host bridges device to go away >> while references are held on child devices, so the first element >> should not change and this should be safe. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> drivers/pci/p2pdma.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c >> index bd89437faf06..2135fe69bb07 100644 >> --- a/drivers/pci/p2pdma.c >> +++ b/drivers/pci/p2pdma.c >> @@ -311,11 +311,15 @@ static const struct pci_p2pdma_whitelist_entry { >> static bool __host_bridge_whitelist(struct pci_host_bridge *host, >> bool same_host_bridge) >> { >> - struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0)); >> const struct pci_p2pdma_whitelist_entry *entry; >> + struct pci_dev *root = host->bus->self; >> unsigned short vendor, device; >> >> if (!root) >> + root = list_first_entry_or_null(&host->bus->devices, >> + struct pci_dev, bus_list); > > Replacing one ugliness (assuming there is a pci_dev for the host > bridge, and that it is at 00.0) with another (still assuming a pci_dev > and that it is host->bus->self or the first entry). I can't suggest > anything better, but maybe a little comment in the code would help > future readers. Yeah, I struggled to find a solution here; this was the best I could come up with. I'd love it if someone had a better idea. I can add a comment for future iterations. > I wish we had a real way to discover this property without the > whitelist, at least for future devices. Was there ever any interest > in a _DSM or similar interface for this? I'd also like to get rid of the whitelist, but I have no idea how or who would have to lead a fight to get the hardware to self describe in way that we could use. > I *am* very glad to remove a pci_get_slot() usage. > >> + >> + if (!root || root->devfn) >> return false; >> >> vendor = root->vendor; > > Don't you need to also remove the "pci_dev_put(root)" a few lines > below? Yes, right. Good catch! Logan
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index bd89437faf06..2135fe69bb07 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -311,11 +311,15 @@ static const struct pci_p2pdma_whitelist_entry { static bool __host_bridge_whitelist(struct pci_host_bridge *host, bool same_host_bridge) { - struct pci_dev *root = pci_get_slot(host->bus, PCI_DEVFN(0, 0)); const struct pci_p2pdma_whitelist_entry *entry; + struct pci_dev *root = host->bus->self; unsigned short vendor, device; if (!root) + root = list_first_entry_or_null(&host->bus->devices, + struct pci_dev, bus_list); + + if (!root || root->devfn) return false; vendor = root->vendor;
In order to use upstream_bridge_distance_warn() from a dma_map function, it must not sleep. However, pci_get_slot() takes the pci_bus_sem so it might sleep. In order to avoid this, try to get the host bridge's device from bus->self, and if that is not set just get the first element in the list. It should be impossible for the host bridges device to go away while references are held on child devices, so the first element should not change and this should be safe. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/pci/p2pdma.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)