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

[Borislav Petkov]

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.

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?

Oh boy.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette