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

[Shiju Jose]

Thanks for reviewing.

>-----Original Message-----
>From: Borislav Petkov <bp@xxxxxxxxx>
>Sent: 13 September 2024 17:41
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-edac@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; linux-
>acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
>mchehab@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan
>Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
>alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
>david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx;
>Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx;
>Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
>naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
>somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
>duenwen@xxxxxxxxxx; mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx;
>wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
>wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; jgroves@xxxxxxxxxx;
>vsalve@xxxxxxxxxx; tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B)
><prime.zeng@xxxxxxxxxxxxx>; Roberto Sassu <roberto.sassu@xxxxxxxxxx>;
>kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>;
>Linuxarm <linuxarm@xxxxxxxxxx>
>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@xxxxxxxxxx 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. 
>
>--
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette

Thanks,
Shiju