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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250807114239.00005b61@huawei.com>
Date: Thu, 7 Aug 2025 11:42:39 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
	<lukas@...ner.de>, Samuel Ortiz <sameo@...osinc.com>, Xu Yilun
	<yilun.xu@...ux.intel.com>
Subject: Re: [PATCH v4 04/10] PCI/TSM: Authenticate devices via platform TSM


> > > > > +	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