Re: [PATCH v12 01/17] EDAC: Add support for EDAC device features control

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 16 Sep 2024 10:21:58 +0100
Shiju Jose <shiju.jose@huawei.com> wrote:

> Thanks for reviewing.
> 
> >-----Original Message-----
> >From: Borislav Petkov <bp@alien8.de>
> >Sent: 13 September 2024 17:41
> >To: Shiju Jose <shiju.jose@huawei.com>
> >Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org; linux-
> >acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> >tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
> >mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
> >Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com;
> >alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
> >david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
> >Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com;
> >Jon.Grimm@amd.com; dave.hansen@linux.intel.com;
> >naoya.horiguchi@nec.com; james.morse@arm.com; jthoughton@google.com;
> >somasundaram.a@hpe.com; erdemaktas@google.com; pgonda@google.com;
> >duenwen@google.com; mike.malvestuto@intel.com; gthelen@google.com;
> >wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
> >wbs@os.amperecomputing.com; nifan.cxl@gmail.com; jgroves@micron.com;
> >vsalve@micron.com; tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B)
> ><prime.zeng@hisilicon.com>; Roberto Sassu <roberto.sassu@huawei.com>;
> >kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
> >Linuxarm <linuxarm@huawei.com>
> >Subject: Re: [PATCH v12 01/17] EDAC: Add support for EDAC device features
> >control
> >
> >On Wed, Sep 11, 2024 at 10:04:30AM +0100, shiju.jose@huawei.com wrote:  
> >> +/**
> >> + * edac_dev_feature_init - Init a RAS feature
> >> + * @parent: client device.
> >> + * @dev_data: pointer to the edac_dev_data structure, which contains
> >> + * client device specific info.
> >> + * @feat: pointer to struct edac_dev_feature.
> >> + * @attr_groups: pointer to attribute group's container.
> >> + *
> >> + * Returns number of scrub features attribute groups on success,  
> >
> >Not "scrub" - this is an interface initializing a generic feature.  
> Will correct.
> >  
> >> + * error otherwise.
> >> + */
> >> +static int edac_dev_feat_init(struct device *parent,
> >> +			      struct edac_dev_data *dev_data,
> >> +			      const struct edac_dev_feature *ras_feat,
> >> +			      const struct attribute_group **attr_groups) {
> >> +	int num;
> >> +
> >> +	switch (ras_feat->ft_type) {
> >> +	case RAS_FEAT_SCRUB:
> >> +		dev_data->scrub_ops = ras_feat->scrub_ops;
> >> +		dev_data->private = ras_feat->ctx;
> >> +		return 1;
> >> +	case RAS_FEAT_ECS:
> >> +		num = ras_feat->ecs_info.num_media_frus;
> >> +		dev_data->ecs_ops = ras_feat->ecs_ops;
> >> +		dev_data->private = ras_feat->ctx;
> >> +		return num;
> >> +	case RAS_FEAT_PPR:
> >> +		dev_data->ppr_ops = ras_feat->ppr_ops;
> >> +		dev_data->private = ras_feat->ctx;
> >> +		return 1;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +}  
> >
> >And why does this function even exist and has kernel-doc comments when all it
> >does is assign a couple of values? And it gets called exactly once?
> >
> >Just merge its body into the call site. There you can reuse the switch-case there
> >too. No need for too much noodling around.  
> edac_dev_feat_init () function is updated with feature specific function call() etc in subsequent
> EDAC feature specific patches. Thus added a separate function.   
> >  
> >> diff --git a/include/linux/edac.h b/include/linux/edac.h index
> >> b4ee8961e623..b337254cf5b8 100644
> >> --- a/include/linux/edac.h
> >> +++ b/include/linux/edac.h
> >> @@ -661,4 +661,59 @@ static inline struct dimm_info
> >> *edac_get_dimm(struct mem_ctl_info *mci,
> >>
> >>  	return mci->dimms[index];
> >>  }
> >> +
> >> +/* EDAC device features */
> >> +
> >> +#define EDAC_FEAT_NAME_LEN	128
> >> +
> >> +/* RAS feature type */
> >> +enum edac_dev_feat {
> >> +	RAS_FEAT_SCRUB,
> >> +	RAS_FEAT_ECS,
> >> +	RAS_FEAT_PPR,
> >> +	RAS_FEAT_MAX  
> >
> >I still don't know what ECS or PPR is.  
> I will add comment/documentation here with a short explanation of features
> if that make sense?
> Each feature is described in the subsequent EDAC feature specific patches. 
Can you bring the enum entries in with those patches?
That way there is no reference to them before we have the information
on what they are.

J
> >
> >--
> >Regards/Gruss,
> >    Boris.
> >
> >https://people.kernel.org/tglx/notes-about-netiquette  
> 
> Thanks,
> Shiju
>