| Message ID | 5b03b8f4d2d04cf7e4997c71daee667c73eb427b.1674468099.git.lukas@wunner.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Collection of DOE material | expand |
Lukas Wunner wrote: > Currently a DOE instance cannot be shared by multiple drivers because > each driver creates its own pci_doe_mb struct for a given DOE instance. > For the same reason a DOE instance cannot be shared between the PCI core > and a driver. > > Overcome this limitation by creating mailboxes in the PCI core on device > enumeration. Provide a pci_find_doe_mailbox() API call to allow drivers > to get a pci_doe_mb for a given (pci_dev, vendor, protocol) triple. > > On device removal, tear down mailboxes in two steps: > > In pci_stop_dev(), before the driver is unbound, stop ongoing DOE > exchanges and prevent new ones from being scheduled. This ensures that > a hot-removed device doesn't needlessly wait for a running exchange to > time out. > > In pci_destroy_dev(), after the driver is unbound, free the mailboxes > and their internal data structures. > > Tested-by: Ira Weiny <ira.weiny@intel.com> I rather like this. Much cleaner, thanks! Minor comment below. Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/doe.c | 71 +++++++++++++++++++++++++++++++++++++++++ > drivers/pci/pci.h | 10 ++++++ > drivers/pci/probe.c | 1 + > drivers/pci/remove.c | 2 ++ > include/linux/pci-doe.h | 2 ++ > include/linux/pci.h | 3 ++ > 6 files changed, 89 insertions(+) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index cc1fdd75ad2a..06c57af05570 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -20,6 +20,8 @@ > #include <linux/pci-doe.h> > #include <linux/workqueue.h> > > +#include "pci.h" > + > #define PCI_DOE_PROTOCOL_DISCOVERY 0 > > /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */ > @@ -662,3 +664,72 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type, > return task.rv; > } > EXPORT_SYMBOL_GPL(pci_doe); > + > +/** > + * pci_find_doe_mailbox() - Find Data Object Exchange mailbox > + * > + * @pdev: PCI device > + * @vendor: Vendor ID > + * @type: Data Object Type > + * > + * Find first DOE mailbox of a PCI device which supports the given protocol. > + * > + * RETURNS: Pointer to the DOE mailbox or NULL if none was found. > + */ > +struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, > + u8 type) > +{ > + struct pci_doe_mb *doe_mb; > + unsigned long index; > + > + xa_for_each(&pdev->doe_mbs, index, doe_mb) > + if (pci_doe_supports_prot(doe_mb, vendor, type)) > + return doe_mb; > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(pci_find_doe_mailbox); > + > +void pci_doe_init(struct pci_dev *pdev) > +{ > + struct pci_doe_mb *doe_mb; > + u16 offset = 0; > + int rc; > + > + xa_init(&pdev->doe_mbs); > + > + while ((offset = pci_find_next_ext_capability(pdev, offset, > + PCI_EXT_CAP_ID_DOE))) { > + doe_mb = pci_doe_create_mb(pdev, offset); > + if (IS_ERR(doe_mb)) > + continue; I feel like a pci_dbg() would be nice here. But not needed. Ira > + > + rc = xa_insert(&pdev->doe_mbs, offset, doe_mb, GFP_KERNEL); > + if (rc) { > + pci_doe_flush_mb(doe_mb); > + pci_doe_destroy_mb(doe_mb); > + pci_err(pdev, "[%x] failed to insert mailbox: %d\n", > + offset, rc); > + } > + } > +} > + > +void pci_doe_stop(struct pci_dev *pdev) > +{ > + struct pci_doe_mb *doe_mb; > + unsigned long index; > + > + xa_for_each(&pdev->doe_mbs, index, doe_mb) > + pci_doe_flush_mb(doe_mb); > +} > + > +void pci_doe_destroy(struct pci_dev *pdev) > +{ > + struct pci_doe_mb *doe_mb; > + unsigned long index; > + > + xa_for_each(&pdev->doe_mbs, index, doe_mb) > + pci_doe_destroy_mb(doe_mb); > + > + xa_destroy(&pdev->doe_mbs); > +} > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 9ed3b5550043..94656c1a01c0 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -777,6 +777,16 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) > } > #endif > > +#ifdef CONFIG_PCI_DOE > +void pci_doe_init(struct pci_dev *pdev); > +void pci_doe_stop(struct pci_dev *pdev); > +void pci_doe_destroy(struct pci_dev *pdev); > +#else > +static inline void pci_doe_init(struct pci_dev *pdev) { } > +static inline void pci_doe_stop(struct pci_dev *pdev) { } > +static inline void pci_doe_destroy(struct pci_dev *pdev) { } > +#endif > + > /* > * Config Address for PCI Configuration Mechanism #1 > * > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 1779582fb500..65e60ee50489 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev) > pci_aer_init(dev); /* Advanced Error Reporting */ > pci_dpc_init(dev); /* Downstream Port Containment */ > pci_rcec_init(dev); /* Root Complex Event Collector */ > + pci_doe_init(dev); /* Data Object Exchange */ > > pcie_report_downtraining(dev); > pci_init_reset_methods(dev); > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > index 0145aef1b930..739c7b0f5b91 100644 > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -16,6 +16,7 @@ static void pci_free_resources(struct pci_dev *dev) > > static void pci_stop_dev(struct pci_dev *dev) > { > + pci_doe_stop(dev); > pci_pme_active(dev, false); > > if (pci_dev_is_added(dev)) { > @@ -39,6 +40,7 @@ static void pci_destroy_dev(struct pci_dev *dev) > list_del(&dev->bus_list); > up_write(&pci_bus_sem); > > + pci_doe_destroy(dev); > pcie_aspm_exit_link_state(dev); > pci_bridge_d3_update(dev); > pci_free_resources(dev); > diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h > index 7f16749c6aa3..d6192ee0ac07 100644 > --- a/include/linux/pci-doe.h > +++ b/include/linux/pci-doe.h > @@ -29,6 +29,8 @@ struct pci_doe_mb; > > struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset); > bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type); > +struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, > + u8 type); > > int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type, > const void *request, size_t request_sz, > diff --git a/include/linux/pci.h b/include/linux/pci.h > index adffd65e84b4..254c79f9013a 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -511,6 +511,9 @@ struct pci_dev { > #endif > #ifdef CONFIG_PCI_P2PDMA > struct pci_p2pdma __rcu *p2pdma; > +#endif > +#ifdef CONFIG_PCI_DOE > + struct xarray doe_mbs; /* Data Object Exchange mailboxes */ > #endif > u16 acs_cap; /* ACS Capability offset */ > phys_addr_t rom; /* Physical address if not from BAR */ > -- > 2.39.1 >
On Mon, 23 Jan 2023 11:17:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > Currently a DOE instance cannot be shared by multiple drivers because > each driver creates its own pci_doe_mb struct for a given DOE instance. > For the same reason a DOE instance cannot be shared between the PCI core > and a driver. > > Overcome this limitation by creating mailboxes in the PCI core on device > enumeration. Provide a pci_find_doe_mailbox() API call to allow drivers > to get a pci_doe_mb for a given (pci_dev, vendor, protocol) triple. > > On device removal, tear down mailboxes in two steps: > > In pci_stop_dev(), before the driver is unbound, stop ongoing DOE > exchanges and prevent new ones from being scheduled. This ensures that > a hot-removed device doesn't needlessly wait for a running exchange to > time out. Ah. I didn't have to go far to find answer to my earlier query! This needs to be two step - hence the split in the previous patch. > > In pci_destroy_dev(), after the driver is unbound, free the mailboxes > and their internal data structures. > > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Very nice. One comment on a possible future need inline. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > +/** > + * pci_find_doe_mailbox() - Find Data Object Exchange mailbox > + * > + * @pdev: PCI device > + * @vendor: Vendor ID > + * @type: Data Object Type > + * > + * Find first DOE mailbox of a PCI device which supports the given protocol. > + * > + * RETURNS: Pointer to the DOE mailbox or NULL if none was found. > + */ > +struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, > + u8 type) This is good for now. We may want eventually to be slightly more flexible and allow for a 'find instance X of a DOE that supports Y'. Can solve that when we need it though. > +{ > + struct pci_doe_mb *doe_mb; > + unsigned long index; > + > + xa_for_each(&pdev->doe_mbs, index, doe_mb) > + if (pci_doe_supports_prot(doe_mb, vendor, type)) > + return doe_mb; > + > + return NULL; > +} > +EXPORT_SYMBOL_GPL(pci_find_doe_mailbox);
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index cc1fdd75ad2a..06c57af05570 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -20,6 +20,8 @@ #include <linux/pci-doe.h> #include <linux/workqueue.h> +#include "pci.h" + #define PCI_DOE_PROTOCOL_DISCOVERY 0 /* Timeout of 1 second from 6.30.2 Operation, PCI Spec r6.0 */ @@ -662,3 +664,72 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type, return task.rv; } EXPORT_SYMBOL_GPL(pci_doe); + +/** + * pci_find_doe_mailbox() - Find Data Object Exchange mailbox + * + * @pdev: PCI device + * @vendor: Vendor ID + * @type: Data Object Type + * + * Find first DOE mailbox of a PCI device which supports the given protocol. + * + * RETURNS: Pointer to the DOE mailbox or NULL if none was found. + */ +struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, + u8 type) +{ + struct pci_doe_mb *doe_mb; + unsigned long index; + + xa_for_each(&pdev->doe_mbs, index, doe_mb) + if (pci_doe_supports_prot(doe_mb, vendor, type)) + return doe_mb; + + return NULL; +} +EXPORT_SYMBOL_GPL(pci_find_doe_mailbox); + +void pci_doe_init(struct pci_dev *pdev) +{ + struct pci_doe_mb *doe_mb; + u16 offset = 0; + int rc; + + xa_init(&pdev->doe_mbs); + + while ((offset = pci_find_next_ext_capability(pdev, offset, + PCI_EXT_CAP_ID_DOE))) { + doe_mb = pci_doe_create_mb(pdev, offset); + if (IS_ERR(doe_mb)) + continue; + + rc = xa_insert(&pdev->doe_mbs, offset, doe_mb, GFP_KERNEL); + if (rc) { + pci_doe_flush_mb(doe_mb); + pci_doe_destroy_mb(doe_mb); + pci_err(pdev, "[%x] failed to insert mailbox: %d\n", + offset, rc); + } + } +} + +void pci_doe_stop(struct pci_dev *pdev) +{ + struct pci_doe_mb *doe_mb; + unsigned long index; + + xa_for_each(&pdev->doe_mbs, index, doe_mb) + pci_doe_flush_mb(doe_mb); +} + +void pci_doe_destroy(struct pci_dev *pdev) +{ + struct pci_doe_mb *doe_mb; + unsigned long index; + + xa_for_each(&pdev->doe_mbs, index, doe_mb) + pci_doe_destroy_mb(doe_mb); + + xa_destroy(&pdev->doe_mbs); +} diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 9ed3b5550043..94656c1a01c0 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -777,6 +777,16 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) } #endif +#ifdef CONFIG_PCI_DOE +void pci_doe_init(struct pci_dev *pdev); +void pci_doe_stop(struct pci_dev *pdev); +void pci_doe_destroy(struct pci_dev *pdev); +#else +static inline void pci_doe_init(struct pci_dev *pdev) { } +static inline void pci_doe_stop(struct pci_dev *pdev) { } +static inline void pci_doe_destroy(struct pci_dev *pdev) { } +#endif + /* * Config Address for PCI Configuration Mechanism #1 * diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 1779582fb500..65e60ee50489 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev) pci_aer_init(dev); /* Advanced Error Reporting */ pci_dpc_init(dev); /* Downstream Port Containment */ pci_rcec_init(dev); /* Root Complex Event Collector */ + pci_doe_init(dev); /* Data Object Exchange */ pcie_report_downtraining(dev); pci_init_reset_methods(dev); diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 0145aef1b930..739c7b0f5b91 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -16,6 +16,7 @@ static void pci_free_resources(struct pci_dev *dev) static void pci_stop_dev(struct pci_dev *dev) { + pci_doe_stop(dev); pci_pme_active(dev, false); if (pci_dev_is_added(dev)) { @@ -39,6 +40,7 @@ static void pci_destroy_dev(struct pci_dev *dev) list_del(&dev->bus_list); up_write(&pci_bus_sem); + pci_doe_destroy(dev); pcie_aspm_exit_link_state(dev); pci_bridge_d3_update(dev); pci_free_resources(dev); diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h index 7f16749c6aa3..d6192ee0ac07 100644 --- a/include/linux/pci-doe.h +++ b/include/linux/pci-doe.h @@ -29,6 +29,8 @@ struct pci_doe_mb; struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset); bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type); +struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor, + u8 type); int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type, const void *request, size_t request_sz, diff --git a/include/linux/pci.h b/include/linux/pci.h index adffd65e84b4..254c79f9013a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -511,6 +511,9 @@ struct pci_dev { #endif #ifdef CONFIG_PCI_P2PDMA struct pci_p2pdma __rcu *p2pdma; +#endif +#ifdef CONFIG_PCI_DOE + struct xarray doe_mbs; /* Data Object Exchange mailboxes */ #endif u16 acs_cap; /* ACS Capability offset */ phys_addr_t rom; /* Physical address if not from BAR */