Re: [PATCH V4 04/17] platform/x86/intel/vsec: Add intel_vsec_register

["David E. Box" <david.e.box@linux.intel.com>]

On Mon, 2023-10-23 at 18:21 +0300, Ilpo Järvinen wrote:
> On Wed, 18 Oct 2023, David E. Box wrote:
> 
> Use () in the shortlog after the function name.
> 
> > From: Gayatri Kammela <gayatri.kammela@linux.intel.com>
> > 
> > Add and export intel_vsec_register() to allow the registration of Intel
> > extended capabilities from other drivers. Add check to look for memory
> > conflicts before registering a new capability. Add a parent field to
> > intel_vsec_platform_info to allow specifying the parent device for
> > device managed cleanup.
> 
> While reviewing this patch, I couldn't understand why the parent is never 
> assigned with anything?

That's because ...

> 
> > Signed-off-by: Gayatri Kammela <gayatri.kammela@linux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> > V4 - Move res cleanup to previous patch
> > 
> > V3 - Replace kfree on request_mem_region fail with use of cleanup.h helper.
> > 
> > V2 - New patch splitting previous PATCH 1
> > 
> >  drivers/platform/x86/intel/vsec.c | 19 +++++++++++++++++--
> >  drivers/platform/x86/intel/vsec.h |  4 ++++
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/vsec.c
> > b/drivers/platform/x86/intel/vsec.c
> > index 28191313d515..638dfde6a9e2 100644
> > --- a/drivers/platform/x86/intel/vsec.c
> > +++ b/drivers/platform/x86/intel/vsec.c
> > @@ -188,6 +188,12 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > struct intel_vsec_header *he
> >                              header->offset + i * (header->entry_size *
> > sizeof(u32));
> >                 tmp->end = tmp->start + (header->entry_size * sizeof(u32)) -
> > 1;
> >                 tmp->flags = IORESOURCE_MEM;
> > +
> > +               /* Check resource is not in use */
> > +               if (!request_mem_region(tmp->start, resource_size(tmp), ""))
> > +                       return -EBUSY;
> > +
> > +               release_mem_region(tmp->start, resource_size(tmp));
> >         }
> >  
> >         intel_vsec_dev->pcidev = pdev;
> > @@ -205,9 +211,8 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > struct intel_vsec_header *he
> >          * intel_vsec_add_aux()
> >          */
> >         no_free_ptr(res);
> > -       ret = intel_vsec_add_aux(pdev, NULL, no_free_ptr(intel_vsec_dev),
> > +       ret = intel_vsec_add_aux(pdev, info->parent,
> > no_free_ptr(intel_vsec_dev),
> >                                  intel_vsec_name(header->id));

... for devices probed by this driver, intel_vsec_add_aux() will assign the
probe device as the parent if the argument is NULL. Originally this function
didn't have an argument for parent. It was added when this function was exported
for use by the tpmi driver which needed to specify its own parent. Users of
intel_vsec_register() need to do the same. I suppose for clarity this driver
could set it as well and we can remove the NULL check.

David

> > -
> 
> Extra newline change.
>