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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1b2fc196-fc27-f782-e7d6-86b72d950fe7@linux.intel.com>
Date: Mon, 30 Jun 2025 15:02:33 +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 03/15] platform/x86/intel/vsec: Create wrapper to walk
 PCI config space

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@...ux.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.