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

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

> +static int pci_tsm_connect(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
> +{
> +	int rc;
> +	struct pci_tsm_pf0 *tsm_pf0;
> +	const struct pci_tsm_ops *ops = tsm_pci_ops(tsm_dev);
> +	struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(pdev);
> +
> +	if (!pci_tsm)
> +		return -ENXIO;
> +
> +	pdev->tsm = pci_tsm;
> +	tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> +
> +	ACQUIRE(mutex_intr, lock)(&tsm_pf0->lock);
> +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
> +		return rc;
> +
> +	rc = ops->connect(pdev);
> +	if (rc)
> +		return rc;
> +
> +	pdev->tsm = no_free_ptr(pci_tsm);
> +
> +	/*
> +	 * Now that the DSM is established, probe() all the potential
> +	 * dependent functions. Failure to probe a function is not fatal
> +	 * to connect(), it just disables subsequent security operations
> +	 * for that function.
> +	 */
> +	pci_tsm_probe_fns(pdev);
> +	return 0;
> +}
> +}
> +

[...]

> +static void pf0_sysfs_enable(struct pci_dev *pdev)
> +{
> +	pci_dbg(pdev, "Device Security Manager detected (%s%s )\n",
> +		pdev->ide_cap ? " ide" : "",
> +		pdev->devcap & PCI_EXP_DEVCAP_TEE ? " tee" : "");
> +
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_auth_attr_group);
> +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_pf0_attr_group);
> +}
> +
> +int pci_tsm_register(struct tsm_dev *tsm_dev)
> +{

[...]

> +	for_each_pci_dev(pdev)
> +		if (is_pci_tsm_pf0(pdev))
> +			pf0_sysfs_enable(pdev);

Now the tsm attributes are exposed to user before ops->probe(), from
user's POV, tsm link operation for this device is already ready ...

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_tsm_register);

[...]

> +struct pci_tsm_ops {
> +	/*
> +	 * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> +	 * @probe: probe device for tsm link operation readiness, setup

So I think the probe callback is losing the meaning of readiness check.
Users see the 'connect/disconnect', they write 'connect' and found
errors no matter ->probe() fails or ->connect() fails.

Maybe just remove the responsibility of readiness check from ->probe(),
I found it simplifies code when implementing tdx-tsm driver.

Thanks,
Yilun

> +	 *	   DSM context
> +	 * @remove: destroy DSM context
> +	 * @connect: establish / validate a secure connection (e.g. IDE)
> +	 *	     with the device
> +	 * @disconnect: teardown the secure link
> +	 *
> +	 * @probe and @remove run in pci_tsm_rwsem held for write context. All
> +	 * other ops run under the @pdev->tsm->lock mutex and pci_tsm_rwsem held
> +	 * for read.
> +	 */
> +	struct_group_tagged(pci_tsm_link_ops, link_ops,
> +		struct pci_tsm *(*probe)(struct pci_dev *pdev);
> +		void (*remove)(struct pci_tsm *tsm);
> +		int (*connect)(struct pci_dev *pdev);
> +		void (*disconnect)(struct pci_dev *pdev);
> +	);