On 1/24/2023 8:15 PM, Jonathan Cameron wrote:
> On Mon, 23 Jan 2023 11:16:00 +0100
> Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx>
>> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> 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);
>
