On 10/15/2021 1:11 AM, Alex Williamson wrote:
On Wed, 13 Oct 2021 12:46:58 +0300
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
From: Jason Gunthorpe <jgg@xxxxxxxxxx>
There are some cases where a SRIOV VF driver will need to reach into and
interact with the PF driver. This requires accessing the drvdata of the PF.
Provide a function pci_iov_get_pf_drvdata() to return this PF drvdata in a
safe way. Normally accessing a drvdata of a foreign struct device would be
done using the device_lock() to protect against device driver
probe()/remove() races.
However, due to the design of pci_enable_sriov() this will result in a
ABBA deadlock on the device_lock as the PF's device_lock is held during PF
sriov_configure() while calling pci_enable_sriov() which in turn holds the
VF's device_lock while calling VF probe(), and similarly for remove.
This means the VF driver can never obtain the PF's device_lock.
Instead use the implicit locking created by pci_enable/disable_sriov(). A
VF driver can access its PF drvdata only while its own driver is attached,
and the PF driver can control access to its own drvdata based on when it
calls pci_enable/disable_sriov().
To use this API the PF driver will setup the PF drvdata in the probe()
function. pci_enable_sriov() is only called from sriov_configure() which
cannot happen until probe() completes, ensuring no VF races with drvdata
setup.
For removal, the PF driver must call pci_disable_sriov() in its remove
function before destroying any of the drvdata. This ensures that all VF
drivers are unbound before returning, fencing concurrent access to the
drvdata.
The introduction of a new function to do this access makes clear the
special locking scheme and the documents the requirements on the PF/VF
drivers using this.
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxx>
---
drivers/pci/iov.c | 29 +++++++++++++++++++++++++++++
include/linux/pci.h | 7 +++++++
2 files changed, 36 insertions(+)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index e7751fa3fe0b..ca696730f761 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -47,6 +47,35 @@ int pci_iov_vf_id(struct pci_dev *dev)
}
EXPORT_SYMBOL_GPL(pci_iov_vf_id);
+/**
+ * pci_iov_get_pf_drvdata - Return the drvdata of a PF
+ * @dev - VF pci_dev
+ * @pf_driver - Device driver required to own the PF
+ *
+ * This must be called from a context that ensures that a VF driver is attached.
+ * The value returned is invalid once the VF driver completes its remove()
+ * callback.
+ *
+ * Locking is achieved by the driver core. A VF driver cannot be probed until
+ * pci_enable_sriov() is called and pci_disable_sriov() does not return until
+ * all VF drivers have completed their remove().
+ *
+ * The PF driver must call pci_disable_sriov() before it begins to destroy the
+ * drvdata.
+ */
+void *pci_iov_get_pf_drvdata(struct pci_dev *dev, struct pci_driver *pf_driver)
+{
+ struct pci_dev *pf_dev;
+
+ if (dev->is_physfn)
+ return ERR_PTR(-EINVAL);
I think we're trying to make this only accessible to VFs, so shouldn't
we test (!dev->is_virtfn)? is_physfn will be zero for either a PF with
failed SR-IOV configuration or for a non-SR-IOV device afaict. Thanks,
Alex
Yes, this should be accessible only for VFs.
We can go with your suggestion to explicitly check (!dev->is_virtfn) as
this seems cleaner and safer as you mentioned.
We already got ACK on this patch from Bjorn but as your suggestion seems
straight forward I may put the Acked-by as part of V2 in any case.
Yishai
