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