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

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> > > You protect against this in the DEFINE_FREE() so probably safe
> > > to assume it is always set if we get here.  
> > 
> > It is safe, but I would rather not require reading other code to
> > understand the expectation that some callers may unconditionally call
> > this routine.
> 
> I think a function like remove being called on 'nothing' should
> pretty much always be a bug, but meh, up to you.

...inspired by kfree(NULL). Potentially saves "if (tsm) tsm_remove(tsm)"
checks down the road, but yes, all of those are obviated by the
DEFINE_FREE() at present.

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

static void pci_tsm_walk_fns(struct pci_dev *pdev,
			     int (*cb)(struct pci_dev *pdev, void *data),
			     void *data)
{
	/* Walk subordinate physical functions */
	for (int i = 0; i < 8; i++) {
		struct pci_dev *pf __free(pci_dev_put) = pci_get_slot(
			pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), i));

		if (!pf)
			continue;

		/* on entry function 0 has already run @cb */
		if (i > 0)
			cb(pf, data);

		/* walk virtual functions of each pf */
		for (int j = 0; j < pci_num_vf(pf); j++) {
			struct pci_dev *vf __free(pci_dev_put) =
				pci_get_domain_bus_and_slot(
					pci_domain_nr(pf->bus),
					pci_iov_virtfn_bus(pf, j),
					pci_iov_virtfn_devfn(pf, j));

			if (!vf)
				continue;

			cb(vf, data);
		}
	}

	/*
	 * Walk downstream devices, assumes that an upstream DSM is
	 * limited to downstream physical functions
	 */
	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM && is_dsm(pdev))
		pci_walk_bus(pdev->subordinate, cb, data);
}

static void pci_tsm_walk_fns_reverse(struct pci_dev *pdev,
				     int (*cb)(struct pci_dev *pdev,
					       void *data),
				     void *data)
{
	/* Reverse walk downstream devices */
	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM && is_dsm(pdev))
		pci_walk_bus_reverse(pdev->subordinate, cb, data);

	/* Reverse walk subordinate physical functions */
	for (int i = 7; i >= 0; i--) {
		struct pci_dev *pf __free(pci_dev_put) = pci_get_slot(
			pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), i));

		if (!pf)
			continue;

		/* reverse walk virtual functions */
		for (int j = pci_num_vf(pf) - 1; j >= 0; j--) {
			struct pci_dev *vf __free(pci_dev_put) =
				pci_get_domain_bus_and_slot(
					pci_domain_nr(pf->bus),
					pci_iov_virtfn_bus(pf, j),
					pci_iov_virtfn_devfn(pf, j));

			if (!vf)
				continue;
			cb(vf, data);
		}

		/* on exit, caller will run @cb on function 0 */
		if (i > 0)
			cb(pf, data);
	}
}

[..]
> 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

[..]
> > > > + * struct pci_tsm_ops - manage confidential links and security state
> > > > + * @link_ops: Coordinate PCIe SPDM and IDE establishment via a platform TSM.
> > > > + * 	      Provide a secure session transport for TDISP state management
> > > > + * 	      (typically bare metal physical function operations).
> > > > + * @sec_ops: Lock, unlock, and interrogate the security state of the
> > > > + *	     function via the platform TSM (typically virtual function
> > > > + *	     operations).
> > > > + * @owner: Back reference to the TSM device that owns this instance.
> > > > + *
> > > > + * This operations are mutually exclusive either a tsm_dev instance
> > > > + * manages phyiscal link properties or it manages function security
> > > > + * states like TDISP lock/unlock.
> > > > + */
> > > > +struct pci_tsm_ops {
> > > > +	/*  
> > > Likewise though I'm not sure if kernel-doc deals with struct groups.  
> > 
> > It does not.
> 
> Hmm. Given they are getting common maybe that's one to address, but
> obviously not in this series.

CXL could use it too...