>-----Original Message-----
>From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>Sent: 16 September 2024 11:50
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: Borislav Petkov <bp@xxxxxxxxx>; 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; 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 Mon, 16 Sep 2024 10:21:58 +0100
>Shiju Jose <shiju.jose@xxxxxxxxxx> wrote:
>
>> 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.
>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.
Will do.
>
>J
>> >
>> >--
>> >Regards/Gruss,
>> > Boris.
>> >
>> >https://people.kernel.org/tglx/notes-about-netiquette
>>
Thanks,
Shiju
