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