| Message ID | 291131574c9e625195e9c34591abf5fa75cd1279.1674468099.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Collection of DOE material | expand |
On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > DOE mailbox creation is currently only possible through a devres-managed > API. The lifetime of mailboxes thus ends with driver unbinding. > > An upcoming commit will create DOE mailboxes upon device enumeration by > the PCI core. Their lifetime shall not be limited by a driver. > > Therefore rework pcim_doe_create_mb() into the non-devres-managed > pci_doe_create_mb(). Add pci_doe_destroy_mb() for mailbox destruction > on device removal. > > Provide a devres-managed wrapper under the existing pcim_doe_create_mb() > name. > > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Hi Lukas, A few comments inline. In particular I'd like to understand why flushing in the tear down can't always be done as that makes the code more complex. Might become clear in later patches though as I've not read ahead yet! Jonathan > --- > drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 70 insertions(+), 33 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 066400531d09..cc1fdd75ad2a 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -37,7 +37,7 @@ > * > * This state is used to manage a single DOE mailbox capability. All fields > * should be considered opaque to the consumers and the structure passed into > - * the helpers below after being created by devm_pci_doe_create() > + * the helpers below after being created by pci_doe_create_mb(). > * > * @pdev: PCI device this mailbox belongs to > * @cap_offset: Capability offset > @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb) > return 0; > } > > -static void pci_doe_xa_destroy(void *mb) > -{ > - struct pci_doe_mb *doe_mb = mb; > - > - xa_destroy(&doe_mb->prots); > -} > - > -static void pci_doe_destroy_workqueue(void *mb) > -{ > - struct pci_doe_mb *doe_mb = mb; > - > - destroy_workqueue(doe_mb->work_queue); > -} > - > static void pci_doe_flush_mb(void *mb) > { > struct pci_doe_mb *doe_mb = mb; > @@ -442,7 +428,7 @@ static void pci_doe_flush_mb(void *mb) > } > > /** > - * pcim_doe_create_mb() - Create a DOE mailbox object > + * pci_doe_create_mb() - Create a DOE mailbox object > * > * @pdev: PCI device to create the DOE mailbox for > * @cap_offset: Offset of the DOE mailbox > @@ -453,24 +439,20 @@ static void pci_doe_flush_mb(void *mb) > * RETURNS: created mailbox object on success > * ERR_PTR(-errno) on failure > */ > -struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > +static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev, > + u16 cap_offset) > { > struct pci_doe_mb *doe_mb; > - struct device *dev = &pdev->dev; > int rc; > > - doe_mb = devm_kzalloc(dev, sizeof(*doe_mb), GFP_KERNEL); > + doe_mb = kzalloc(sizeof(*doe_mb), GFP_KERNEL); > if (!doe_mb) > return ERR_PTR(-ENOMEM); > > doe_mb->pdev = pdev; > doe_mb->cap_offset = cap_offset; > init_waitqueue_head(&doe_mb->wq); > - > xa_init(&doe_mb->prots); See below - I'd move xa_init() down to just above the pci_doe_cache_protocols() call. > - rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb); > - if (rc) > - return ERR_PTR(rc); > > doe_mb->work_queue = alloc_ordered_workqueue("%s %s DOE [%x]", 0, > dev_driver_string(&pdev->dev), > @@ -479,35 +461,90 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > if (!doe_mb->work_queue) { > pci_err(pdev, "[%x] failed to allocate work queue\n", > doe_mb->cap_offset); > - return ERR_PTR(-ENOMEM); > + rc = -ENOMEM; > + goto err_free; > } > - rc = devm_add_action_or_reset(dev, pci_doe_destroy_workqueue, doe_mb); > - if (rc) > - return ERR_PTR(rc); > > /* Reset the mailbox by issuing an abort */ > rc = pci_doe_abort(doe_mb); > if (rc) { > pci_err(pdev, "[%x] failed to reset mailbox with abort command : %d\n", > doe_mb->cap_offset, rc); > - return ERR_PTR(rc); > + goto err_destroy_wq; > } > > /* > * The state machine and the mailbox should be in sync now; > - * Set up mailbox flush prior to using the mailbox to query protocols. > + * Use the mailbox to query protocols. > */ > - rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, doe_mb); > - if (rc) > - return ERR_PTR(rc); > - > rc = pci_doe_cache_protocols(doe_mb); > if (rc) { > pci_err(pdev, "[%x] failed to cache protocols : %d\n", > doe_mb->cap_offset, rc); > + goto err_flush; > + } > + > + return doe_mb; > + > +err_flush: > + pci_doe_flush_mb(doe_mb); > + xa_destroy(&doe_mb->prots); Why the reorder wrt to the original devm managed cleanup? I'd expect this to happen on any error path after the xa_init. It doesn't matter in practice because there isn't anything to do until after pci_doe_cache_protocols though. Maybe simplest option would be move xa_init() down to just above the call to pci_doe_cache_protocols()? That way the order you have here would meet the 'obviously correct' test. > +err_destroy_wq: > + destroy_workqueue(doe_mb->work_queue); > +err_free: > + kfree(doe_mb); > + return ERR_PTR(rc); > +} > + > +/** > + * pci_doe_destroy_mb() - Destroy a DOE mailbox object > + * > + * @ptr: Pointer to DOE mailbox > + * > + * Destroy all internal data structures created for the DOE mailbox. Could you comment on why it doesn't make sense to flush the mb on this path? Perhaps add a comment here to say what state we should be in before calling this? Not flushing here means you need more complex handling in error paths. > + */ > +static void pci_doe_destroy_mb(void *ptr) > +{ > + struct pci_doe_mb *doe_mb = ptr; > + > + xa_destroy(&doe_mb->prots); If making the change above, also push the xa_destroy() below the destroy_workqueue() here. > + destroy_workqueue(doe_mb->work_queue); > + kfree(doe_mb); > +} > + > +/** > + * pcim_doe_create_mb() - Create a DOE mailbox object > + * > + * @pdev: PCI device to create the DOE mailbox for > + * @cap_offset: Offset of the DOE mailbox > + * > + * Create a single mailbox object to manage the mailbox protocol at the > + * cap_offset specified. The mailbox will automatically be destroyed on > + * driver unbinding from @pdev. > + * > + * RETURNS: created mailbox object on success > + * ERR_PTR(-errno) on failure > + */ > +struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > +{ > + struct pci_doe_mb *doe_mb; > + int rc; > + > + doe_mb = pci_doe_create_mb(pdev, cap_offset); > + if (IS_ERR(doe_mb)) > + return doe_mb; > + > + rc = devm_add_action(&pdev->dev, pci_doe_destroy_mb, doe_mb); > + if (rc) { > + pci_doe_flush_mb(doe_mb); > + pci_doe_destroy_mb(doe_mb); > return ERR_PTR(rc); > } > > + rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb); > + if (rc) > + return ERR_PTR(rc); > + > return doe_mb; > } > EXPORT_SYMBOL_GPL(pcim_doe_create_mb);
On Tue, 24 Jan 2023 12:15:43 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 23 Jan 2023 11:16:00 +0100 > Lukas Wunner <lukas@wunner.de> wrote: > > > DOE mailbox creation is currently only possible through a devres-managed > > API. The lifetime of mailboxes thus ends with driver unbinding. > > > > An upcoming commit will create DOE mailboxes upon device enumeration by > > the PCI core. Their lifetime shall not be limited by a driver. > > > > Therefore rework pcim_doe_create_mb() into the non-devres-managed > > pci_doe_create_mb(). Add pci_doe_destroy_mb() for mailbox destruction > > on device removal. > > > > Provide a devres-managed wrapper under the existing pcim_doe_create_mb() > > name. > > > > Tested-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Hi Lukas, > > A few comments inline. > > In particular I'd like to understand why flushing in the tear down > can't always be done as that makes the code more complex. > > Might become clear in later patches though as I've not read ahead yet! Ah.. It's in the patch description of the next patch. So ignore this question. Thanks, Jonathan > > Jonathan > > > --- > > drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++--------------- > > 1 file changed, 70 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > > index 066400531d09..cc1fdd75ad2a 100644 > > --- a/drivers/pci/doe.c > > +++ b/drivers/pci/doe.c > > @@ -37,7 +37,7 @@ > > * > > * This state is used to manage a single DOE mailbox capability. All fields > > * should be considered opaque to the consumers and the structure passed into > > - * the helpers below after being created by devm_pci_doe_create() > > + * the helpers below after being created by pci_doe_create_mb(). > > * > > * @pdev: PCI device this mailbox belongs to > > * @cap_offset: Capability offset > > @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb) > > return 0; > > } > > > > -static void pci_doe_xa_destroy(void *mb) > > -{ > > - struct pci_doe_mb *doe_mb = mb; > > - > > - xa_destroy(&doe_mb->prots); > > -} > > - > > -static void pci_doe_destroy_workqueue(void *mb) > > -{ > > - struct pci_doe_mb *doe_mb = mb; > > - > > - destroy_workqueue(doe_mb->work_queue); > > -} > > - > > static void pci_doe_flush_mb(void *mb) > > { > > struct pci_doe_mb *doe_mb = mb; > > @@ -442,7 +428,7 @@ static void pci_doe_flush_mb(void *mb) > > } > > > > /** > > - * pcim_doe_create_mb() - Create a DOE mailbox object > > + * pci_doe_create_mb() - Create a DOE mailbox object > > * > > * @pdev: PCI device to create the DOE mailbox for > > * @cap_offset: Offset of the DOE mailbox > > @@ -453,24 +439,20 @@ static void pci_doe_flush_mb(void *mb) > > * RETURNS: created mailbox object on success > > * ERR_PTR(-errno) on failure > > */ > > -struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > > +static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev, > > + u16 cap_offset) > > { > > struct pci_doe_mb *doe_mb; > > - struct device *dev = &pdev->dev; > > int rc; > > > > - doe_mb = devm_kzalloc(dev, sizeof(*doe_mb), GFP_KERNEL); > > + doe_mb = kzalloc(sizeof(*doe_mb), GFP_KERNEL); > > if (!doe_mb) > > return ERR_PTR(-ENOMEM); > > > > doe_mb->pdev = pdev; > > doe_mb->cap_offset = cap_offset; > > init_waitqueue_head(&doe_mb->wq); > > - > > xa_init(&doe_mb->prots); > See below - I'd move xa_init() down to just above the pci_doe_cache_protocols() > call. > > > - rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb); > > - if (rc) > > - return ERR_PTR(rc); > > > > doe_mb->work_queue = alloc_ordered_workqueue("%s %s DOE [%x]", 0, > > dev_driver_string(&pdev->dev), > > @@ -479,35 +461,90 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > > if (!doe_mb->work_queue) { > > pci_err(pdev, "[%x] failed to allocate work queue\n", > > doe_mb->cap_offset); > > - return ERR_PTR(-ENOMEM); > > + rc = -ENOMEM; > > + goto err_free; > > } > > - rc = devm_add_action_or_reset(dev, pci_doe_destroy_workqueue, doe_mb); > > - if (rc) > > - return ERR_PTR(rc); > > > > /* Reset the mailbox by issuing an abort */ > > rc = pci_doe_abort(doe_mb); > > if (rc) { > > pci_err(pdev, "[%x] failed to reset mailbox with abort command : %d\n", > > doe_mb->cap_offset, rc); > > - return ERR_PTR(rc); > > + goto err_destroy_wq; > > } > > > > /* > > * The state machine and the mailbox should be in sync now; > > - * Set up mailbox flush prior to using the mailbox to query protocols. > > + * Use the mailbox to query protocols. > > */ > > - rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, doe_mb); > > - if (rc) > > - return ERR_PTR(rc); > > - > > rc = pci_doe_cache_protocols(doe_mb); > > if (rc) { > > pci_err(pdev, "[%x] failed to cache protocols : %d\n", > > doe_mb->cap_offset, rc); > > + goto err_flush; > > + } > > + > > + return doe_mb; > > + > > +err_flush: > > + pci_doe_flush_mb(doe_mb); > > + xa_destroy(&doe_mb->prots); > > Why the reorder wrt to the original devm managed cleanup? > I'd expect this to happen on any error path after the xa_init. > > It doesn't matter in practice because there isn't anything to > do until after pci_doe_cache_protocols though. Maybe > simplest option would be move xa_init() down to just above > the call to pci_doe_cache_protocols()? That way the order > you have here would meet the 'obviously correct' test. > > > > +err_destroy_wq: > > + destroy_workqueue(doe_mb->work_queue); > > +err_free: > > + kfree(doe_mb); > > + return ERR_PTR(rc); > > +} > > + > > +/** > > + * pci_doe_destroy_mb() - Destroy a DOE mailbox object > > + * > > + * @ptr: Pointer to DOE mailbox > > + * > > + * Destroy all internal data structures created for the DOE mailbox. > > Could you comment on why it doesn't make sense to flush the > mb on this path? Perhaps add a comment here to say what state > we should be in before calling this? > > Not flushing here means you need more complex handling in > error paths. > > > + */ > > +static void pci_doe_destroy_mb(void *ptr) > > +{ > > + struct pci_doe_mb *doe_mb = ptr; > > + > > + xa_destroy(&doe_mb->prots); > > If making the change above, also push the xa_destroy() below > the destroy_workqueue() here. > > > + destroy_workqueue(doe_mb->work_queue); > > + kfree(doe_mb); > > +} > > + > > +/** > > + * pcim_doe_create_mb() - Create a DOE mailbox object > > + * > > + * @pdev: PCI device to create the DOE mailbox for > > + * @cap_offset: Offset of the DOE mailbox > > + * > > + * Create a single mailbox object to manage the mailbox protocol at the > > + * cap_offset specified. The mailbox will automatically be destroyed on > > + * driver unbinding from @pdev. > > + * > > + * RETURNS: created mailbox object on success > > + * ERR_PTR(-errno) on failure > > + */ > > +struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) > > +{ > > + struct pci_doe_mb *doe_mb; > > + int rc; > > + > > + doe_mb = pci_doe_create_mb(pdev, cap_offset); > > + if (IS_ERR(doe_mb)) > > + return doe_mb; > > + > > + rc = devm_add_action(&pdev->dev, pci_doe_destroy_mb, doe_mb); > > + if (rc) { > > + pci_doe_flush_mb(doe_mb); > > + pci_doe_destroy_mb(doe_mb); > > return ERR_PTR(rc); > > } > > > > + rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb); > > + if (rc) > > + return ERR_PTR(rc); > > + > > return doe_mb; > > } > > EXPORT_SYMBOL_GPL(pcim_doe_create_mb); >
On 1/24/2023 8:15 PM, Jonathan Cameron wrote: > On Mon, 23 Jan 2023 11:16:00 +0100 > Lukas Wunner <lukas@wunner.de> wrote: > >> DOE mailbox creation is currently only possible through a devres-managed >> API. The lifetime of mailboxes thus ends with driver unbinding. >> >> An upcoming commit will create DOE mailboxes upon device enumeration by >> the PCI core. Their lifetime shall not be limited by a driver. >> >> Therefore rework pcim_doe_create_mb() into the non-devres-managed >> pci_doe_create_mb(). Add pci_doe_destroy_mb() for mailbox destruction >> on device removal. >> >> Provide a devres-managed wrapper under the existing pcim_doe_create_mb() >> name. >> >> Tested-by: Ira Weiny <ira.weiny@intel.com> >> Signed-off-by: Lukas Wunner <lukas@wunner.de> > Hi Lukas, > > A few comments inline. > > In particular I'd like to understand why flushing in the tear down > can't always be done as that makes the code more complex. > > Might become clear in later patches though as I've not read ahead yet! > > Jonathan > >> --- >> drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++--------------- >> 1 file changed, 70 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c >> index 066400531d09..cc1fdd75ad2a 100644 >> --- a/drivers/pci/doe.c >> +++ b/drivers/pci/doe.c >> @@ -37,7 +37,7 @@ >> * >> * This state is used to manage a single DOE mailbox capability. All fields >> * should be considered opaque to the consumers and the structure passed into >> - * the helpers below after being created by devm_pci_doe_create() >> + * the helpers below after being created by pci_doe_create_mb(). >> * >> * @pdev: PCI device this mailbox belongs to >> * @cap_offset: Capability offset >> @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb) >> return 0; >> } >> ...... >> +/** >> + * pci_doe_destroy_mb() - Destroy a DOE mailbox object >> + * >> + * @ptr: Pointer to DOE mailbox >> + * >> + * Destroy all internal data structures created for the DOE mailbox. > >> + */ >> +static void pci_doe_destroy_mb(void *ptr) Sorry, didn't find the original patch, reply on here. I don't get why uses "void *ptr" as the parameter of this function, maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb" as the parameter. Thanks Ming >> +{ >> + struct pci_doe_mb *doe_mb = ptr; >> + >> + xa_destroy(&doe_mb->prots); >> + destroy_workqueue(doe_mb->work_queue); >> + kfree(doe_mb); >> +} >> + >> +/** >> + * pcim_doe_create_mb() - Create a DOE mailbox object >> + * >> + * @pdev: PCI device to create the DOE mailbox for >> + * @cap_offset: Offset of the DOE mailbox >> + * >> + * Create a single mailbox object to manage the mailbox protocol at the >> + * cap_offset specified. The mailbox will automatically be destroyed on >> + * driver unbinding from @pdev. >> + * >> + * RETURNS: created mailbox object on success >> + * ERR_PTR(-errno) on failure >> + */ >> +struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) >> +{ >> + struct pci_doe_mb *doe_mb; >> + int rc; >> + >> + doe_mb = pci_doe_create_mb(pdev, cap_offset); >> + if (IS_ERR(doe_mb)) >> + return doe_mb; >> + >> + rc = devm_add_action(&pdev->dev, pci_doe_destroy_mb, doe_mb); >> + if (rc) { >> + pci_doe_flush_mb(doe_mb); >> + pci_doe_destroy_mb(doe_mb); >> return ERR_PTR(rc); >> } >> >> + rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb); >> + if (rc) >> + return ERR_PTR(rc); >> + >> return doe_mb; >> } >> EXPORT_SYMBOL_GPL(pcim_doe_create_mb); >
On 2/3/2023 5:06 PM, Li, Ming wrote: > On 1/24/2023 8:15 PM, Jonathan Cameron wrote: >> On Mon, 23 Jan 2023 11:16:00 +0100 >> Lukas Wunner <lukas@wunner.de> wrote: >> >>> DOE mailbox creation is currently only possible through a devres-managed >>> API. The lifetime of mailboxes thus ends with driver unbinding. >>> >>> An upcoming commit will create DOE mailboxes upon device enumeration by >>> the PCI core. Their lifetime shall not be limited by a driver. >>> >>> Therefore rework pcim_doe_create_mb() into the non-devres-managed >>> pci_doe_create_mb(). Add pci_doe_destroy_mb() for mailbox destruction >>> on device removal. >>> >>> Provide a devres-managed wrapper under the existing pcim_doe_create_mb() >>> name. >>> >>> Tested-by: Ira Weiny <ira.weiny@intel.com> >>> Signed-off-by: Lukas Wunner <lukas@wunner.de> >> Hi Lukas, >> >> A few comments inline. >> >> In particular I'd like to understand why flushing in the tear down >> can't always be done as that makes the code more complex. >> >> Might become clear in later patches though as I've not read ahead yet! >> >> Jonathan >> >>> --- >>> drivers/pci/doe.c | 103 +++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 70 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c >>> index 066400531d09..cc1fdd75ad2a 100644 >>> --- a/drivers/pci/doe.c >>> +++ b/drivers/pci/doe.c >>> @@ -37,7 +37,7 @@ >>> * >>> * This state is used to manage a single DOE mailbox capability. All fields >>> * should be considered opaque to the consumers and the structure passed into >>> - * the helpers below after being created by devm_pci_doe_create() >>> + * the helpers below after being created by pci_doe_create_mb(). >>> * >>> * @pdev: PCI device this mailbox belongs to >>> * @cap_offset: Capability offset >>> @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb) >>> return 0; >>> } >>> > > ...... > >>> +/** >>> + * pci_doe_destroy_mb() - Destroy a DOE mailbox object >>> + * >>> + * @ptr: Pointer to DOE mailbox >>> + * >>> + * Destroy all internal data structures created for the DOE mailbox. >>>> + */ >>> +static void pci_doe_destroy_mb(void *ptr) > > Sorry, didn't find the original patch, reply on here. > I don't get why uses "void *ptr" as the parameter of this function, maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb" as the parameter. > > Thanks > Ming > Please ignore my comment, I saw it has been changed by PATCH #9 Thanks Ming > >>> +{ >>> + struct pci_doe_mb *doe_mb = ptr; >>> + >>> + xa_destroy(&doe_mb->prots); >>> + destroy_workqueue(doe_mb->work_queue); >>> + kfree(doe_mb); >>> +} >>> + >>> +/** >>> + * pcim_doe_create_mb() - Create a DOE mailbox object >>> + * >>> + * @pdev: PCI device to create the DOE mailbox for >>> + * @cap_offset: Offset of the DOE mailbox >>> + * >>> + * Create a single mailbox object to manage the mailbox protocol at the >>> + * cap_offset specified. The mailbox will automatically be destroyed on >>> + * driver unbinding from @pdev. >>> + * >>> + * RETURNS: created mailbox object on success >>> + * ERR_PTR(-errno) on failure >>> + */ >>> +struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) >>> +{ >>> + struct pci_doe_mb *doe_mb; >>> + int rc; >>> + >>> + doe_mb = pci_doe_create_mb(pdev, cap_offset); >>> + if (IS_ERR(doe_mb)) >>> + return doe_mb; >>> + >>> + rc = devm_add_action(&pdev->dev, pci_doe_destroy_mb, doe_mb); >>> + if (rc) { >>> + pci_doe_flush_mb(doe_mb); >>> + pci_doe_destroy_mb(doe_mb); >>> return ERR_PTR(rc); >>> } >>> >>> + rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb); >>> + if (rc) >>> + return ERR_PTR(rc); >>> + >>> return doe_mb; >>> } >>> EXPORT_SYMBOL_GPL(pcim_doe_create_mb); >> >
On Fri, Feb 03, 2023 at 05:06:21PM +0800, Li, Ming wrote: > On 1/24/2023 8:15 PM, Jonathan Cameron wrote: > > On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > > +/** > > > + * pci_doe_destroy_mb() - Destroy a DOE mailbox object > > > + * > > > + * @ptr: Pointer to DOE mailbox > > > + * > > > + * Destroy all internal data structures created for the DOE mailbox. > > > + */ > > > +static void pci_doe_destroy_mb(void *ptr) > > I don't get why uses "void *ptr" as the parameter of this function, > maybe I miss something. I guess we can use "struct pci_doe_mb *doe_mb" > as the parameter. It's a "void *ptr" argument so that pci_doe_destroy_mb() can serve as a callback for devm_add_action(). But as you've correctly noted, devm_add_action() is removed in a subsequent commit and then the argument is adjusted to "struct pci_doe_mb *doe_mb". Thanks, Lukas
On Tue, Jan 24, 2023 at 12:15:43PM +0000, Jonathan Cameron wrote: > On Mon, 23 Jan 2023 11:16:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > DOE mailbox creation is currently only possible through a devres-managed > > API. The lifetime of mailboxes thus ends with driver unbinding. > > > > An upcoming commit will create DOE mailboxes upon device enumeration by > > the PCI core. Their lifetime shall not be limited by a driver. > > > > Therefore rework pcim_doe_create_mb() into the non-devres-managed > > pci_doe_create_mb(). Add pci_doe_destroy_mb() for mailbox destruction > > on device removal. [...] > I'd like to understand why flushing in the tear down > can't always be done as that makes the code more complex. After sending out v2, I realized I had made a mistake: In v2, on device removal I canceled any ongoing DOE exchanges and declared the DOE mailbox dead before unbinding the driver from a device. That's the right thing to do for surprise removal because you don't want to wait for an ongoing exchange to time out. However we also have a code path for orderly device removal, either via sysfs or by pressing the Attention Button (if present). In that case, it should be legal for the driver to still perform DOE exchanges in its ->remove() hook. So in v3 I've changed the behavior to only cancel requests on surprise removal. By doing so, I was able to always flush on mailbox destruction, as you've requested, and thereby simplify the error paths. > > +err_flush: > > + pci_doe_flush_mb(doe_mb); > > + xa_destroy(&doe_mb->prots); > > Why the reorder wrt to the original devm managed cleanup? > I'd expect this to happen on any error path after the xa_init. > > It doesn't matter in practice because there isn't anything to > do until after pci_doe_cache_protocols though. Right, it's unnecessary to call xa_destroy() if alloc_ordered_workqueue() failed because the xarray is still empty at that point. It doesn't need to be destroyed until it's been populated by pci_doe_cache_protocols(). I've amended the commit message to explain that, but otherwise did not change the code in v3. Let me know if you have any objections or feel strongly about moving xa_init(). Thanks, Lukas
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index 066400531d09..cc1fdd75ad2a 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -37,7 +37,7 @@ * * This state is used to manage a single DOE mailbox capability. All fields * should be considered opaque to the consumers and the structure passed into - * the helpers below after being created by devm_pci_doe_create() + * the helpers below after being created by pci_doe_create_mb(). * * @pdev: PCI device this mailbox belongs to * @cap_offset: Capability offset @@ -412,20 +412,6 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb) return 0; } -static void pci_doe_xa_destroy(void *mb) -{ - struct pci_doe_mb *doe_mb = mb; - - xa_destroy(&doe_mb->prots); -} - -static void pci_doe_destroy_workqueue(void *mb) -{ - struct pci_doe_mb *doe_mb = mb; - - destroy_workqueue(doe_mb->work_queue); -} - static void pci_doe_flush_mb(void *mb) { struct pci_doe_mb *doe_mb = mb; @@ -442,7 +428,7 @@ static void pci_doe_flush_mb(void *mb) } /** - * pcim_doe_create_mb() - Create a DOE mailbox object + * pci_doe_create_mb() - Create a DOE mailbox object * * @pdev: PCI device to create the DOE mailbox for * @cap_offset: Offset of the DOE mailbox @@ -453,24 +439,20 @@ static void pci_doe_flush_mb(void *mb) * RETURNS: created mailbox object on success * ERR_PTR(-errno) on failure */ -struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) +static struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev, + u16 cap_offset) { struct pci_doe_mb *doe_mb; - struct device *dev = &pdev->dev; int rc; - doe_mb = devm_kzalloc(dev, sizeof(*doe_mb), GFP_KERNEL); + doe_mb = kzalloc(sizeof(*doe_mb), GFP_KERNEL); if (!doe_mb) return ERR_PTR(-ENOMEM); doe_mb->pdev = pdev; doe_mb->cap_offset = cap_offset; init_waitqueue_head(&doe_mb->wq); - xa_init(&doe_mb->prots); - rc = devm_add_action(dev, pci_doe_xa_destroy, doe_mb); - if (rc) - return ERR_PTR(rc); doe_mb->work_queue = alloc_ordered_workqueue("%s %s DOE [%x]", 0, dev_driver_string(&pdev->dev), @@ -479,35 +461,90 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) if (!doe_mb->work_queue) { pci_err(pdev, "[%x] failed to allocate work queue\n", doe_mb->cap_offset); - return ERR_PTR(-ENOMEM); + rc = -ENOMEM; + goto err_free; } - rc = devm_add_action_or_reset(dev, pci_doe_destroy_workqueue, doe_mb); - if (rc) - return ERR_PTR(rc); /* Reset the mailbox by issuing an abort */ rc = pci_doe_abort(doe_mb); if (rc) { pci_err(pdev, "[%x] failed to reset mailbox with abort command : %d\n", doe_mb->cap_offset, rc); - return ERR_PTR(rc); + goto err_destroy_wq; } /* * The state machine and the mailbox should be in sync now; - * Set up mailbox flush prior to using the mailbox to query protocols. + * Use the mailbox to query protocols. */ - rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, doe_mb); - if (rc) - return ERR_PTR(rc); - rc = pci_doe_cache_protocols(doe_mb); if (rc) { pci_err(pdev, "[%x] failed to cache protocols : %d\n", doe_mb->cap_offset, rc); + goto err_flush; + } + + return doe_mb; + +err_flush: + pci_doe_flush_mb(doe_mb); + xa_destroy(&doe_mb->prots); +err_destroy_wq: + destroy_workqueue(doe_mb->work_queue); +err_free: + kfree(doe_mb); + return ERR_PTR(rc); +} + +/** + * pci_doe_destroy_mb() - Destroy a DOE mailbox object + * + * @ptr: Pointer to DOE mailbox + * + * Destroy all internal data structures created for the DOE mailbox. + */ +static void pci_doe_destroy_mb(void *ptr) +{ + struct pci_doe_mb *doe_mb = ptr; + + xa_destroy(&doe_mb->prots); + destroy_workqueue(doe_mb->work_queue); + kfree(doe_mb); +} + +/** + * pcim_doe_create_mb() - Create a DOE mailbox object + * + * @pdev: PCI device to create the DOE mailbox for + * @cap_offset: Offset of the DOE mailbox + * + * Create a single mailbox object to manage the mailbox protocol at the + * cap_offset specified. The mailbox will automatically be destroyed on + * driver unbinding from @pdev. + * + * RETURNS: created mailbox object on success + * ERR_PTR(-errno) on failure + */ +struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset) +{ + struct pci_doe_mb *doe_mb; + int rc; + + doe_mb = pci_doe_create_mb(pdev, cap_offset); + if (IS_ERR(doe_mb)) + return doe_mb; + + rc = devm_add_action(&pdev->dev, pci_doe_destroy_mb, doe_mb); + if (rc) { + pci_doe_flush_mb(doe_mb); + pci_doe_destroy_mb(doe_mb); return ERR_PTR(rc); } + rc = devm_add_action_or_reset(&pdev->dev, pci_doe_flush_mb, doe_mb); + if (rc) + return ERR_PTR(rc); + return doe_mb; } EXPORT_SYMBOL_GPL(pcim_doe_create_mb);