RE: [PATCH v2 7/8] cxl/memfeature: Add CXL memory device soft PPR control feature

[Shiju Jose]

>-----Original Message-----
>From: Borislav Petkov <bp@xxxxxxxxx>
>Sent: 27 March 2025 17:09
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-cxl@xxxxxxxxxxxxxxx; 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; linux-edac@xxxxxxxxxxxxxxx;
>linux-acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
>mchehab@xxxxxxxxxx; 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;
>gthelen@xxxxxxxxxx; wschwartz@xxxxxxxxxxxxxxxxxxx;
>dferguson@xxxxxxxxxxxxxxxxxxx; wbs@xxxxxxxxxxxxxxxxxxxxxx;
>nifan.cxl@xxxxxxxxx; 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 v2 7/8] cxl/memfeature: Add CXL memory device soft PPR
>control feature
>
>On Thu, Mar 27, 2025 at 06:01:56PM +0100, Borislav Petkov wrote:
>> On Thu, Mar 20, 2025 at 06:04:44PM +0000, shiju.jose@xxxxxxxxxx wrote:
>> > diff --git a/drivers/edac/mem_repair.c b/drivers/edac/mem_repair.c
>> > index 3b1a845457b0..bf7e01a8b4dd 100755
>> > --- a/drivers/edac/mem_repair.c
>> > +++ b/drivers/edac/mem_repair.c
>> > @@ -45,6 +45,11 @@ struct edac_mem_repair_context {
>> >  	struct attribute_group group;
>> >  };
>> >
>> > +const char * const edac_repair_type[] = {
>> > +	[EDAC_PPR] = "ppr",
>> > +};
>> > +EXPORT_SYMBOL_GPL(edac_repair_type);
>>
>> Why is this thing exported instead of adding a getter function and
>> having all its users pass in proper defines as arguments?
>>
>> And "EDAC_PPR" is not a proper define - it doesn't tell me what it is.
>>
>> It should be more likely a
>>
>> EDAC_REPAIR_PPR,
>> EDAC_REPAIR_ROW_SPARING,
>> EDAC_REPAIR_BANK_SPARING,
>>
>> and so on.

Hi Borislav,

Will change.

>
>Looking at this more:
>
>+static int cxl_ppr_get_repair_type(struct device *dev, void *drv_data,
>+				   const char **repair_type)
>+{
>+	*repair_type = edac_repair_type[EDAC_PPR];
>+
>+	return 0;
>+}
>
>Can this be any more silly?
>
>An ops member which copies a string pointer into some argument. What for?
>
>If those strings are for userspace, why don't you simply return *numbers* and
>let userspace convert them into strings?
Yes. The EDAC memory repair interface defined as return 'numbers' for userspace until 
v18 of the EDAC series.  Changed to return 'string' as Mauro wanted.
Please see discussion here.
https://lore.kernel.org/all/20250114152617.14eb41b5@xxxxxxx/

>
Thanks,
Shiju