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: <7kombko2ybkjvaolmwkgedo4qy2ifgu44gjypopgh4sjmgfnmy@pzlflvvpacej>
Date: Mon, 30 Jun 2025 13:51:29 -0700
From: David Box <david.e.box@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...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, Jun 30, 2025 at 03:02:33PM +0300, Ilpo Järvinen wrote:
> 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?

Yes. Will change is to features_found and just return it directly.

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

Will change it to intel_vsec_discover_features(). Thanks.

David

> 
> > +}
> > +
> >  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.