>-----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