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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 3 Mar 2025 13:29:53 -0400
Jason Gunthorpe <jgg@nvidia.com> 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 :\

I think this one is more about the classic risk of undefined
behavior meaning the compiler removes the check.  So if we had a
bob = pdsfc->fred;

but didn't use bob before the check on pdsfc != NULL then
the compiler is allowed to remove the check on pdsfc existing
as the dereferencce can be assume to mean pdsfc is always
!= NULL.

So not really a cleanup.h related issue at all (and not in my
view suitable for adding to the nice help Dan put in that file).

Doesn't matter too much though.  Personally I'm kind of content
now with cleanup.h usage but it was a bit of time to teach
myself the new way of thinking about things.

Jonathan


> 
> > > +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
> 
> Thanks,
> Jason