Re: [PATCH v2 4/6] pds_fwctl: initial driver framework

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 3/3/2025 9:29 AM, Jason Gunthorpe wrote:
> 
> On Mon, Mar 03, 2025 at 09:45:52AM -0700, Dave Jiang wrote:
>>> +static int pdsfc_probe(struct auxiliary_device *adev,
>>> +                  const struct auxiliary_device_id *id)
>>> +{
>>> +   struct pds_auxiliary_dev *padev =
>>> +                   container_of(adev, struct pds_auxiliary_dev, aux_dev);
>>> +   struct pdsfc_dev *pdsfc __free(pdsfc_dev) =
>>> +                   fwctl_alloc_device(&padev->vf_pdev->dev, &pdsfc_ops,
>>> +                                      struct pdsfc_dev, fwctl);
>>> +   struct device *dev = &adev->dev;
>>> +   int err;
>>> +
>>
>> It's ok to move the pdsfc declaration inline right before this check
>> below. That would help prevent any accidental usages of pdsfc before
>> the check. This is an exception to the coding style guidelines with
>> the introduction of cleanup mechanisms.
> 
> Yeah.. I'm starting to feel negative about cleanup.h - there are too
> many special style notes that seem to only be known by hearsay :\
> 
>>> +static void pdsfc_remove(struct auxiliary_device *adev)
>>> +{
>>> +   struct pdsfc_dev *pdsfc __free(pdsfc_dev) = auxiliary_get_drvdata(adev);
>>> +
>>> +   fwctl_unregister(&pdsfc->fwctl);
>>
>> Missing fwctl_put(). See fwctl_unregister() header comments. Caller
>> must still call fwctl_put() to free the fwctl after calling
>> fwctl_unregister().
> 
> The code is correct, the put is hidden in the __free
> 
> However I think we decided not to use __free in this context and open
> code the put as a style choice

I'm of the same opinion.  I pulled this pattern out of the other 
functions, but left it here as it seemed less 'different'.  Maybe this 
will go away as well if there are other reasons to rework the code.

sln