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
