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 <[email protected]>
---
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);
+
+ if (!root || root->devfn)
return false;
vendor = root->vendor;
--
2.20.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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