Re: [PATCH 3/4] platform/x86/intel/pmt: decouple sysfs and namespace

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4312 bytes --]

On Fri, 16 May 2025, Michael J. Ruhl wrote:

> The PMT namespace includes the crashlog sysfs attribute
> information.  Other crashlog version/types may need

One space is enough after . (it might be your editor auto-adds the second 
space, if that's the case, look into the editors settings).

> different sysfs attributes.  Coupling the attributes with
> the namespace blocks this usage.
> 
> Decouple sysfs attributes from the name space and add them
> to the specific entry.

Too short lines here as well, please reflow the paragraphs.

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/class.c    | 12 ++++++------
>  drivers/platform/x86/intel/pmt/class.h    |  2 +-
>  drivers/platform/x86/intel/pmt/crashlog.c |  3 ++-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index 7233b654bbad..7404807c3943 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
> @@ -284,8 +284,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  
>  	entry->kobj = &dev->kobj;
>  
> -	if (ns->attr_grp) {
> -		ret = sysfs_create_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp) {
> +		ret = sysfs_create_group(entry->kobj, entry->attr_grp);
>  		if (ret)
>  			goto fail_sysfs_create_group;
>  	}
> @@ -326,8 +326,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  fail_add_endpoint:
>  	sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>  fail_ioremap:
> -	if (ns->attr_grp)
> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp)
> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>  fail_sysfs_create_group:
>  	device_unregister(dev);
>  fail_dev_create:
> @@ -369,8 +369,8 @@ void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
>  	if (entry->size)
>  		sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
>  
> -	if (ns->attr_grp)
> -		sysfs_remove_group(entry->kobj, ns->attr_grp);
> +	if (entry->attr_grp)
> +		sysfs_remove_group(entry->kobj, entry->attr_grp);
>  
>  	device_unregister(dev);
>  	xa_erase(ns->xa, entry->devid);
> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index b2006d57779d..6b3455a86471 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
> @@ -41,6 +41,7 @@ struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;
>  	struct intel_pmt_header	header;
>  	struct bin_attribute	pmt_bin_attr;
> +	const struct attribute_group *attr_grp;
>  	struct kobject		*kobj;
>  	void __iomem		*disc_table;
>  	void __iomem		*base;
> @@ -54,7 +55,6 @@ struct intel_pmt_entry {
>  struct intel_pmt_namespace {
>  	const char *name;
>  	struct xarray *xa;
> -	const struct attribute_group *attr_grp;
>  	int (*pmt_header_decode)(struct intel_pmt_entry *entry,
>  				 struct device *dev);
>  	int (*pmt_add_endpoint)(struct intel_vsec_device *ivdev,
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index dba7e7c1585d..c9bfe1c26311 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -307,6 +307,8 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>  	/* Size is measured in DWORDS, but accessor returns bytes */
>  	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
>  
> +	entry->attr_grp = &pmt_crashlog_group;
> +
>  	return 0;
>  }
>  
> @@ -314,7 +316,6 @@ static DEFINE_XARRAY_ALLOC(crashlog_array);
>  static struct intel_pmt_namespace pmt_crashlog_ns = {
>  	.name = "crashlog",
>  	.xa = &crashlog_array,
> -	.attr_grp = &pmt_crashlog_group,
>  	.pmt_header_decode = pmt_crashlog_header_decode,
>  	.pmt_add_endpoint = pmt_crashlog_add_endpoint,
>  };
> 

With minor changelog related issues fixed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


BTW, now that I remember, you should always include the correct entries 
from MAINTAINERS file as receipients. It's not enough to send only to
platform-driver-x86@vger.kernel.org.

-- 
 i.