Re: [PATCH v4 04/10] PCI/TSM: Authenticate devices via platform TSM

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

> > > > > +	pdev = tsm->pdev;
> > > > > +	tsm->ops->remove(tsm);
> > > > > +	pdev->tsm = NULL;
> > > > > +}
> > > > > +DEFINE_FREE(tsm_remove, struct pci_tsm *, if (_T) tsm_remove(_T))
> > > > > +
> > > > > +static int call_cb_put(struct pci_dev *pdev, void *data,    
> > > > 
> > > > Is this combination worth while?  I don't like the 'and' aspect of it
> > > > and it only saves a few lines...
> > > > 
> > > > vs
> > > > 	if (pdev) {
> > > > 		rc = cb(pdev, data);
> > > > 		pci_dev_put(pdev);
> > > > 		if (rc)
> > > > 			return;
> > > > 	}    
> > > 
> > > I think it is worth it, but an even better option is to just let
> > > scope-based cleanup handle it by moving the variable inside the loop
> > > declaration.  
> > I don't follow that lat bit, but will look at next version to see
> > what you mean!  
> 
> Here is new approach (only compile tested) after understanding that loop
> declared variables do trigger cleanup on each iteration.

Looks good.


> 
> [..]
> > I agree it's a slightly odd construction and so might cause confusion.
> > So whilst I think I prefer it to the or_reset() pattern I guess I'll just
> > try and remember why this is odd (should I ever read this again after it's
> > merged!) :)  
> 
> However, I am interested in these "the trouble with cleanup.h" style
> discussions.
> 
> I recently suggested this [1] in another thread which indeed uses
> multiple local variables of the same object to represent the different
> phases of the setup. It was easier there because the code was
> straigtforward to convert to an ERR_PTR() organization.
> 
> If there was already an alternative device_add() like this:
> 
> struct device *device_add_or_reset(struct device *dev)
> 
> That handled the put_device() then you could write:
> 
> struct device *devreg __free(device_unregister) = device_add_or_reset(no_free_ptr(dev))
> 
> ...and help that common pattern of 'struct device' setup transitions
> from put_device() to device_unregister() at device_add() time.
> 
> [1]: http://lore.kernel.org/688bcf40215c3_48e5100d6@dwillia2-xfh.jf.intel.com.notmuch

That's definitely interesting (in a fairly good way) as anything to stop people
introducing bugs around the device_add() stuff would be welcome.  It'll take a bit
of getting used to though.  Maybe make it more explicit device_add_or_put()?

Naming hard as normal..
> 
Jonathan