Message-ID: <8ccfd660-791a-6cf1-2982-b4f09ae6d3e1@linux.intel.com>
Date: Mon, 30 Jun 2025 15:06:38 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "David E. Box" <david.e.box@...ux.intel.com>
cc: LKML <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org, 
    srinivas.pandruvada@...ux.intel.com, 
    Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, tony.luck@...el.com, 
    xi.pardee@...ux.intel.com, Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH V2 04/15] platform/x86/intel/vsec: Add device links to
 enforce dependencies

On Mon, 16 Jun 2025, David E. Box wrote:

> New Intel VSEC features will have dependencies on other features, requiring
> certain supplier drivers to be probed before their consumers. To enforce
> this dependency ordering, introduce device links using device_link_add(),
> ensuring that suppliers are fully registered before consumers are probed.
> 
> - Add device link tracking by storing supplier devices and tracking their
>   state.
> - Implement intel_vsec_link_devices() to establish links between suppliers
>   and consumers based on feature dependencies.
> - Add get_consumer_dependencies() to retrieve supplier-consumer
>   relationships.
> - Modify feature registration logic:
>   * Consumers now check that all required suppliers are registered before
>     being initialized.
>   * suppliers_ready() verifies that all required supplier devices are
>     available.
> - Prevent potential null consumer name issue in sysfs:
>   - Use dev_set_name() when creating auxiliary devices to ensure a
>     unique, non-null consumer name.
> - Update intel_vsec_pci_probe() to loop up to the number of possible
>   features or when all devices are registered, whichever comes first.
> - Introduce VSEC_CAP_UNUSED to prevent sub-features (registered via
>   exported APIs) from being mistakenly linked.
> 
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> ---
> 
> Changes in v2:
>   - Simply dependency search in get_consumer_dependencies() per comments
>     from Ilpo.
>   - Add rollback for auxiliary_device_uninit() in intel_vsec_add_aux().
>   - In suppliers_ready() clarify that for_each_set_bit() is searching for
>     all *ready* suppliers, not all suppliers. If any is not ready and not
>     ignored, it immediately returns.
>   - In suppliers_ready() check device_link_add() return status.
>   - In intel_vsec_probe() uses info->caps consistently.
>   - Fix spelling errors and remove unrelated changes.
> 
>  drivers/platform/x86/intel/vsec.c | 219 ++++++++++++++++++++++++++++--
>  include/linux/intel_vsec.h        |  28 +++-
>  2 files changed, 232 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
> index f01651f498ca..e497b570f814 100644
> --- a/drivers/platform/x86/intel/vsec.c
> +++ b/drivers/platform/x86/intel/vsec.c
> @@ -15,9 +15,11 @@
>  
>  #include <linux/auxiliary_bus.h>
>  #include <linux/bits.h>
> +#include <linux/bitops.h>
>  #include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/idr.h>
> +#include <linux/log2.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -32,8 +34,17 @@ static DEFINE_IDA(intel_vsec_ida);
>  static DEFINE_IDA(intel_vsec_sdsi_ida);
>  static DEFINE_XARRAY_ALLOC(auxdev_array);
>  
> +enum vsec_device_state {
> +	STATE_NOT_FOUND,
> +	STATE_REGISTERED,
> +	STATE_SKIP,
> +};
> +
>  struct vsec_priv {
>  	struct intel_vsec_platform_info *info;
> +	struct device *suppliers[VSEC_FEATURE_COUNT];
> +	enum vsec_device_state state[VSEC_FEATURE_COUNT];
> +	unsigned long found_caps;
>  };
>  
>  static const char *intel_vsec_name(enum intel_vsec_id id)
> @@ -95,6 +106,74 @@ static void intel_vsec_dev_release(struct device *dev)
>  	kfree(intel_vsec_dev);
>  }
>  
> +static const struct vsec_feature_dependency *
> +get_consumer_dependencies(struct vsec_priv *priv, int cap_id)
> +{
> +	const struct vsec_feature_dependency *deps = priv->info->deps;
> +	int consumer_id = priv->info->num_deps;
> +
> +	if (!deps)
> +		return NULL;
> +
> +	while (consumer_id--)
> +		if (deps[consumer_id].feature == BIT(cap_id))
> +			return &deps[consumer_id];
> +
> +	return NULL;
> +}
> +
> +/*
> + * Although pci_device_id table is available in the pdev, this prototype is
> + * necessary because the code using it can be called by an exported API that
> + * might pass a different pdev.
> + */
> +static const struct pci_device_id intel_vsec_pci_ids[];
> +
> +static int intel_vsec_link_devices(struct pci_dev *pdev, struct device *dev,
> +				   int consumer_id)
> +{
> +	const struct vsec_feature_dependency *deps;
> +	enum vsec_device_state *state;
> +	struct device **suppliers;
> +	struct vsec_priv *priv;
> +	int supplier_id;
> +
> +	if (!consumer_id)
> +		return 0;
> +
> +	if (!pci_match_id(intel_vsec_pci_ids, pdev))
> +		return 0;
> +
> +	priv = pci_get_drvdata(pdev);
> +	state = priv->state;
> +	suppliers = priv->suppliers;
> +
> +	priv->suppliers[consumer_id] = dev;
> +
> +	deps = get_consumer_dependencies(priv, consumer_id);
> +	if (!deps)
> +		return 0;
> +
> +	for_each_set_bit(supplier_id, &deps->supplier_bitmap, VSEC_FEATURE_COUNT) {
> +		struct device_link *link;
> +
> +		if (state[supplier_id] != STATE_REGISTERED)
> +			continue;
> +
> +		if (!suppliers[supplier_id]) {
> +			dev_err(dev, "Bad supplier list\n");
> +			return -EINVAL;
> +		}
> +
> +		link = device_link_add(dev, suppliers[supplier_id],
> +				       DL_FLAG_AUTOPROBE_CONSUMER);
> +		if (!link)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
>  		       struct intel_vsec_device *intel_vsec_dev,
>  		       const char *name)
> @@ -132,19 +211,37 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
>  		return ret;
>  	}
>  
> +	/*
> +	 * Assign a name now to ensure that the device link doesn't contain
> +	 * a null string for the consumer name. This is a problem when a supplier
> +	 * supplies more than one consumer and can lead to a duplicate name error
> +	 * when the link is created in sysfs.
> +	 */
> +	ret = dev_set_name(&auxdev->dev, "%s.%s.%d", KBUILD_MODNAME, auxdev->name,
> +			   auxdev->id);
> +	if (ret)
> +		goto cleanup_aux;
> +
> +	ret = intel_vsec_link_devices(pdev, &auxdev->dev, intel_vsec_dev->cap_id);
> +	if (ret)
> +		goto cleanup_aux;
> +
>  	ret = auxiliary_device_add(auxdev);
> -	if (ret < 0) {
> -		auxiliary_device_uninit(auxdev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto cleanup_aux;
>  
>  	return devm_add_action_or_reset(parent, intel_vsec_remove_aux,
>  				       auxdev);
> +
> +cleanup_aux:
> +	auxiliary_device_uninit(auxdev);
> +	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(intel_vsec_add_aux, "INTEL_VSEC");
>  
>  static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *header,
> -			      struct intel_vsec_platform_info *info)
> +			      struct intel_vsec_platform_info *info,
> +			      unsigned long cap_id)
>  {
>  	struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
>  	struct resource __free(kfree) *res = NULL;
> @@ -211,6 +308,7 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
>  	intel_vsec_dev->quirks = info->quirks;
>  	intel_vsec_dev->base_addr = info->base_addr;
>  	intel_vsec_dev->priv_data = info->priv_data;
> +	intel_vsec_dev->cap_id = cap_id;
>  
>  	if (header->id == VSEC_ID_SDSI)
>  		intel_vsec_dev->ida = &intel_vsec_sdsi_ida;
> @@ -225,6 +323,101 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
>  				  intel_vsec_name(header->id));
>  }
>  
> +static bool suppliers_ready(struct vsec_priv *priv,
> +			    const struct vsec_feature_dependency *consumer_deps,
> +			    int cap_id)
> +{
> +	enum vsec_device_state *state = priv->state;
> +	int supplier_id;
> +
> +	if (consumer_deps->feature != BIT(cap_id))
> +		return false; /* Should not happen */

I don't recall if I asked this already, but does the comment imply this is 
an error on the caller side warranting using WARN_ON_ONCE()?

-- 
 i.

> +
> +	/*
> +	 * Verify that all required suppliers have been found. Return false
> +	 * immediately if any are still missing.
> +	 */
> +	for_each_set_bit(supplier_id, &consumer_deps->supplier_bitmap, VSEC_FEATURE_COUNT) {
> +		if (state[supplier_id] == STATE_SKIP)
> +			continue;
> +
> +		if (state[supplier_id] == STATE_NOT_FOUND)
> +			return false;
> +	}
> +
> +	/*
> +	 * All suppliers have been found and the consumer is ready to be
> +	 * registered.
> +	 */
> +	return true;
> +}
> +
> +static int get_cap_id(u32 header_id, unsigned long *cap_id)
> +{
> +	switch (header_id) {
> +	case VSEC_ID_TELEMETRY:
> +		*cap_id = ilog2(VSEC_CAP_TELEMETRY);
> +		break;
> +	case VSEC_ID_WATCHER:
> +		*cap_id = ilog2(VSEC_CAP_WATCHER);
> +		break;
> +	case VSEC_ID_CRASHLOG:
> +		*cap_id = ilog2(VSEC_CAP_CRASHLOG);
> +		break;
> +	case VSEC_ID_SDSI:
> +		*cap_id = ilog2(VSEC_CAP_SDSI);
> +		break;
> +	case VSEC_ID_TPMI:
> +		*cap_id = ilog2(VSEC_CAP_TPMI);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_vsec_register_device(struct pci_dev *pdev,
> +				      struct intel_vsec_header *header,
> +				      struct intel_vsec_platform_info *info)
> +{
> +	const struct vsec_feature_dependency *consumer_deps;
> +	struct vsec_priv *priv;
> +	unsigned long cap_id;
> +	int ret;
> +
> +	ret = get_cap_id(header->id, &cap_id);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Only track dependencies for devices probed by the VSEC driver.
> +	 * For others using the exported APIs, add the device directly.
> +	 */
> +	if (!pci_match_id(intel_vsec_pci_ids, pdev))
> +		return intel_vsec_add_dev(pdev, header, info, cap_id);
> +
> +	priv = pci_get_drvdata(pdev);
> +	if (priv->state[cap_id] == STATE_REGISTERED ||
> +	    priv->state[cap_id] == STATE_SKIP)
> +		return -EEXIST;
> +
> +	priv->found_caps |= BIT(cap_id);
> +
> +	consumer_deps = get_consumer_dependencies(priv, cap_id);
> +	if (!consumer_deps || suppliers_ready(priv, consumer_deps, cap_id)) {
> +		ret = intel_vsec_add_dev(pdev, header, info, cap_id);
> +		if (ret)
> +			priv->state[cap_id] = STATE_SKIP;
> +		else
> +			priv->state[cap_id] = STATE_REGISTERED;
> +
> +		return ret;
> +	}
> +
> +	return -EAGAIN;
> +}
> +
>  static bool intel_vsec_walk_header(struct pci_dev *pdev,
>  				   struct intel_vsec_platform_info *info)
>  {
> @@ -233,7 +426,7 @@ static bool intel_vsec_walk_header(struct pci_dev *pdev,
>  	int ret;
>  
>  	for ( ; *header; header++) {
> -		ret = intel_vsec_add_dev(pdev, *header, info);
> +		ret = intel_vsec_register_device(pdev, *header, info);
>  		if (!ret)
>  			have_devices = true;
>  	}
> @@ -281,7 +474,7 @@ static bool intel_vsec_walk_dvsec(struct pci_dev *pdev,
>  		pci_read_config_dword(pdev, pos + PCI_DVSEC_HEADER2, &hdr);
>  		header.id = PCI_DVSEC_HEADER2_ID(hdr);
>  
> -		ret = intel_vsec_add_dev(pdev, &header, info);
> +		ret = intel_vsec_register_device(pdev, &header, info);
>  		if (ret)
>  			continue;
>  
> @@ -326,7 +519,7 @@ static bool intel_vsec_walk_vsec(struct pci_dev *pdev,
>  		header.tbir = INTEL_DVSEC_TABLE_BAR(table);
>  		header.offset = INTEL_DVSEC_TABLE_OFFSET(table);
>  
> -		ret = intel_vsec_add_dev(pdev, &header, info);
> +		ret = intel_vsec_register_device(pdev, &header, info);
>  		if (ret)
>  			continue;
>  
> @@ -375,7 +568,7 @@ static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id
>  	struct intel_vsec_platform_info *info;
>  	struct vsec_priv *priv;
>  	bool have_devices = false;
> -	int ret;
> +	int num_caps, ret;
>  
>  	ret = pcim_enable_device(pdev);
>  	if (ret)
> @@ -393,7 +586,13 @@ static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id
>  	priv->info = info;
>  	pci_set_drvdata(pdev, priv);
>  
> -	intel_vsec_feature_walk(pdev, &have_devices, info);
> +	num_caps = hweight_long(info->caps);
> +	while (num_caps--) {
> +		intel_vsec_feature_walk(pdev, &have_devices, info);
> +
> +		if (priv->found_caps == info->caps)
> +			break;
> +	}
>  
>  	if (!have_devices)
>  		return -ENODEV;
> diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
> index bc95821f1bfb..71067afaca99 100644
> --- a/include/linux/intel_vsec.h
> +++ b/include/linux/intel_vsec.h
> @@ -5,11 +5,18 @@
>  #include <linux/auxiliary_bus.h>
>  #include <linux/bits.h>
>  
> -#define VSEC_CAP_TELEMETRY	BIT(0)
> -#define VSEC_CAP_WATCHER	BIT(1)
> -#define VSEC_CAP_CRASHLOG	BIT(2)
> -#define VSEC_CAP_SDSI		BIT(3)
> -#define VSEC_CAP_TPMI		BIT(4)
> +/*
> + * VSEC_CAP_UNUSED is reserved. It exists to prevent zero initialized
> + * intel_vsec devices from being automatically set to a known
> + * capability with ID 0
> + */
> +#define VSEC_CAP_UNUSED		BIT(0)
> +#define VSEC_CAP_TELEMETRY	BIT(1)
> +#define VSEC_CAP_WATCHER	BIT(2)
> +#define VSEC_CAP_CRASHLOG	BIT(3)
> +#define VSEC_CAP_SDSI		BIT(4)
> +#define VSEC_CAP_TPMI		BIT(5)
> +#define VSEC_FEATURE_COUNT	6
>  
>  /* Intel DVSEC offsets */
>  #define INTEL_DVSEC_ENTRIES		0xA
> @@ -81,22 +88,31 @@ struct pmt_callbacks {
>  	int (*read_telem)(struct pci_dev *pdev, u32 guid, u64 *data, loff_t off, u32 count);
>  };
>  
> +struct vsec_feature_dependency {
> +	unsigned long feature;
> +	unsigned long supplier_bitmap;
> +};
> +
>  /**
>   * struct intel_vsec_platform_info - Platform specific data
>   * @parent:    parent device in the auxbus chain
>   * @headers:   list of headers to define the PMT client devices to create
> + * @deps:      array of feature dependencies
>   * @priv_data: private data, usable by parent devices, currently a callback
>   * @caps:      bitmask of PMT capabilities for the given headers
>   * @quirks:    bitmask of VSEC device quirks
>   * @base_addr: allow a base address to be specified (rather than derived)
> + * @num_deps:  Count feature dependencies
>   */
>  struct intel_vsec_platform_info {
>  	struct device *parent;
>  	struct intel_vsec_header **headers;
> +	const struct vsec_feature_dependency *deps;
>  	void *priv_data;
>  	unsigned long caps;
>  	unsigned long quirks;
>  	u64 base_addr;
> +	int num_deps;
>  };
>  
>  /**
> @@ -110,6 +126,7 @@ struct intel_vsec_platform_info {
>   * @priv_data:     any private data needed
>   * @quirks:        specified quirks
>   * @base_addr:     base address of entries (if specified)
> + * @cap_id:        the enumerated id of the vsec feature
>   */
>  struct intel_vsec_device {
>  	struct auxiliary_device auxdev;
> @@ -122,6 +139,7 @@ struct intel_vsec_device {
>  	size_t priv_data_size;
>  	unsigned long quirks;
>  	u64 base_addr;
> +	unsigned long cap_id;
>  };
>  
>  int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
> 

Powered by blists - more mailing lists