| Message ID | 20231018231624.1044633-5-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | intel_pmc: Add telemetry API to read counters | expand |
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? > 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)); > - Extra newline change.
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. >
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)); - return ret; } @@ -325,6 +330,16 @@ static bool intel_vsec_walk_vsec(struct pci_dev *pdev, return have_devices; } +void intel_vsec_register(struct pci_dev *pdev, + struct intel_vsec_platform_info *info) +{ + if (!pdev || !info) + return; + + intel_vsec_walk_header(pdev, info); +} +EXPORT_SYMBOL_NS_GPL(intel_vsec_register, INTEL_VSEC); + static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct intel_vsec_platform_info *info; diff --git a/drivers/platform/x86/intel/vsec.h b/drivers/platform/x86/intel/vsec.h index d3fefba3e623..a15fda2fcd28 100644 --- a/drivers/platform/x86/intel/vsec.h +++ b/drivers/platform/x86/intel/vsec.h @@ -69,6 +69,7 @@ enum intel_vsec_quirks { /* Platform specific data */ struct intel_vsec_platform_info { + struct device *parent; struct intel_vsec_header **headers; unsigned long caps; unsigned long quirks; @@ -98,4 +99,7 @@ static inline struct intel_vsec_device *auxdev_to_ivdev(struct auxiliary_device { return container_of(auxdev, struct intel_vsec_device, auxdev); } + +void intel_vsec_register(struct pci_dev *pdev, + struct intel_vsec_platform_info *info); #endif