Re: [PATCH v2 06/10] PCI/DOE: Allow mailbox creation without devres management

["Li, Ming" <ming4.li@intel.com>]

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