Re: [PATCH 05/11] PCI/TSM: Authenticate devices via platform TSM

[Xu Yilun <yilun.xu@linux.intel.com>]

> +static int pci_tsm_disconnect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm *pci_tsm = pdev->tsm;
> +
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	if_not_guard(mutex_intr, &pci_tsm->lock)
> +		return -EINTR;
> +
> +	if (pci_tsm->state < PCI_TSM_CONNECT)
> +		return 0;
> +	if (pci_tsm->state < PCI_TSM_INIT)
> +		return -ENXIO;

Check PCI_TSM_INIT first, or this condition will never hit.

  if (pci_tsm->state < PCI_TSM_INIT)
	return -ENXIO;
  if (pci_tsm->state < PCI_TSM_CONNECT)
	return 0;

I suggest the same sequence for pci_tsm_connect().

> +
> +	tsm_ops->disconnect(pdev);
> +	pci_tsm->state = PCI_TSM_INIT;
> +
> +	return 0;
> +}
> +
> +static int pci_tsm_connect(struct pci_dev *pdev)
> +{
> +	struct pci_tsm *pci_tsm = pdev->tsm;
> +	int rc;
> +
> +	lockdep_assert_held(&pci_tsm_rwsem);
> +	if_not_guard(mutex_intr, &pci_tsm->lock)
> +		return -EINTR;
> +
> +	if (pci_tsm->state >= PCI_TSM_CONNECT)
> +		return 0;
> +	if (pci_tsm->state < PCI_TSM_INIT)
> +		return -ENXIO;
> +
> +	rc = tsm_ops->connect(pdev);
> +	if (rc)
> +		return rc;
> +	pci_tsm->state = PCI_TSM_CONNECT;
> +	return 0;
> +}
> +

[...]

> +
> +static void __pci_tsm_init(struct pci_dev *pdev)
> +{
> +	bool tee_cap;
> +
> +	if (!is_physical_endpoint(pdev))
> +		return;

This Filters out virtual functions, just because not ready for support,
is it?

> +
> +	tee_cap = pdev->devcap & PCI_EXP_DEVCAP_TEE;
> +
> +	if (!(pdev->ide_cap || tee_cap))
> +		return;
> +
> +	lockdep_assert_held_write(&pci_tsm_rwsem);
> +	if (!tsm_ops)
> +		return;
> +
> +	struct pci_tsm *pci_tsm __free(kfree) = kzalloc(sizeof(*pci_tsm), GFP_KERNEL);
> +	if (!pci_tsm)
> +		return;
> +
> +	/*
> +	 * If a physical device has any security capabilities it may be
> +	 * a candidate to connect with the platform TSM
> +	 */
> +	struct pci_dsm *dsm __free(dsm_remove) = tsm_ops->probe(pdev);

IIUC, pdev->tsm should be for every pci function (physical or virtual),
pdev->tsm->dsm should be only for physical functions, is it?

> +
> +	pci_dbg(pdev, "Device security capabilities detected (%s%s ), TSM %s\n",
> +		pdev->ide_cap ? " ide" : "", tee_cap ? " tee" : "",
> +		dsm ? "attach" : "skip");
> +
> +	if (!dsm)
> +		return;
> +
> +	mutex_init(&pci_tsm->lock);
> +	pci_tsm->doe_mb = pci_find_doe_mailbox(pdev, PCI_VENDOR_ID_PCI_SIG,
> +					       PCI_DOE_PROTO_CMA);
> +	if (!pci_tsm->doe_mb) {
> +		pci_warn(pdev, "TSM init failure, no CMA mailbox\n");
> +		return;
> +	}
> +
> +	pci_tsm->state = PCI_TSM_INIT;
> +	pci_tsm->dsm = no_free_ptr(dsm);
> +	pdev->tsm = no_free_ptr(pci_tsm);
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_auth_attr_group);
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_attr_group);
> +	if (pci_tsm_owner_attr_group)
> +		sysfs_merge_group(&pdev->dev.kobj, pci_tsm_owner_attr_group);
> +}