Re: [PATCH V2 03/15] platform/x86/intel/vsec: Create wrapper to walk PCI config space

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

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

> Combine three PCI config space walkers — intel_vsec_walk_dvsec(),
> intel_vsec_walk_vsec(), and intel_vsec_walk_header() — into a new wrapper
> function, intel_vsec_feature_walk().  This refactoring simplifies the probe
> logic and lays the groundwork for future patches that will loop over these
> calls. No functional changes.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 
> Changes in v2:
>   - No changes
> 
>  drivers/platform/x86/intel/vsec.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
> index 59fb6568a855..f01651f498ca 100644
> --- a/drivers/platform/x86/intel/vsec.c
> +++ b/drivers/platform/x86/intel/vsec.c
> @@ -349,6 +349,27 @@ int intel_vsec_register(struct pci_dev *pdev,
>  }
>  EXPORT_SYMBOL_NS_GPL(intel_vsec_register, "INTEL_VSEC");
>  
> +static void intel_vsec_feature_walk(struct pci_dev *pdev, bool *have_devices,
> +				    struct intel_vsec_platform_info *info)
> +{
> +	/*
> +	 * Both DVSEC and VSEC capabilities can exist on the same device,
> +	 * so both intel_vsec_walk_dvsec() and intel_vsec_walk_vsec() must be
> +	 * called independently. Additionally, intel_vsec_walk_header() is
> +	 * needed for devices that do not have VSEC/DVSEC but provide the
> +	 * information via device_data.
> +	 */
> +	if (intel_vsec_walk_dvsec(pdev, info))
> +		*have_devices = true;
> +
> +	if (intel_vsec_walk_vsec(pdev, info))
> +		*have_devices = true;
> +
> +	if (info && (info->quirks & VSEC_QUIRK_NO_DVSEC) &&
> +	    intel_vsec_walk_header(pdev, info))
> +		*have_devices = true;

Should have_devices be named something more specific in this function or 
perhaps be simply the return value for this function?

IMO, the name of the function could be better too, having "walk" in the 
name feels unnecessary internal detail compared to what this function 
tries to do on a more abstract level.

> +}
> +
>  static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct intel_vsec_platform_info *info;
> @@ -372,15 +393,7 @@ static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id
>  	priv->info = info;
>  	pci_set_drvdata(pdev, priv);
>  
> -	if (intel_vsec_walk_dvsec(pdev, info))
> -		have_devices = true;
> -
> -	if (intel_vsec_walk_vsec(pdev, info))
> -		have_devices = true;
> -
> -	if (info && (info->quirks & VSEC_QUIRK_NO_DVSEC) &&
> -	    intel_vsec_walk_header(pdev, info))
> -		have_devices = true;
> +	intel_vsec_feature_walk(pdev, &have_devices, info);
>  
>  	if (!have_devices)
>  		return -ENODEV;
> 

-- 
 i.