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@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
>
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);
>>
>
