RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from loading after ghes_edac is registered

[Justin He]

Hi Toshi,

> -----Original Message-----
> From: Kani, Toshi <toshi.kani@xxxxxxx>
> Sent: Friday, August 19, 2022 9:30 AM
> To: Justin He <Justin.He@xxxxxxx>; Ard Biesheuvel <ardb@xxxxxxxxxx>; Len
> Brown <lenb@xxxxxxxxxx>; James Morse <James.Morse@xxxxxxx>; Tony Luck
> <tony.luck@xxxxxxxxx>; Borislav Petkov <bp@xxxxxxxxx>; Mauro Carvalho
> Chehab <mchehab@xxxxxxxxxx>; Robert Richter <rric@xxxxxxxxxx>; Robert
> Moore <robert.moore@xxxxxxxxx>; Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>;
> Yazen Ghannam <yazen.ghannam@xxxxxxx>
> Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-edac@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx; Rafael J . Wysocki
> <rafael@xxxxxxxxxx>; Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>; Jarkko
> Sakkinen <jarkko@xxxxxxxxxx>; linux-efi@xxxxxxxxxxxxxxx; nd <nd@xxxxxxx>
> Subject: RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from
> loading after ghes_edac is registered
> 
> On Wednesday, August 17, 2022 8:35 AM, Jia He wrote:
> > Previous, there is only one edac can be registering in the EDAC core.
> > After ghes_edac is modularized, the registering of ghes devices may be
> > deferred when ghes_edac is loaded. Prevent the chipset-specific edac
> > drivers from loading after ghes_edac is registered, which allow the
> > edac drivers to get selected in e.g. HPE platform.
> ...
> > @@ -1382,6 +1395,7 @@ static int ghes_probe(struct platform_device
> > *ghes_dev)
> >  	platform_set_drvdata(ghes_dev, ghes);
> >
> >  	ghes->dev = &ghes_dev->dev;
> > +	set_ghes_devs_registered(false);
> 
> This does not look right to me.
> 
> The condition of using ghes_edac is (ghes-present) && (ghes-preferred),
> where:
>  - ghes-present is latched on ghes_probe()
>  - ghes-preferred is true if the platform-check passes.
> 
> ghes_get_device() introduced in the previous patch works as the
> ghes-preferred check.
> 
> We cannot use ghes_edac registration as the ghes-present check in this
> scheme since it is deferred to module_init().

What is the logic for ghes-present check? In this patch, I assumed it is equal to 
"ghes_edac devices have been registered". Seems it is not correct.
But should we consider the case as follows:
What if sbridge_init () is invoked before ghes_edac_init()? i.e. Should we get
sb_edac driver selected when ghes_edac is not loaded yet (e.g. on HPE)?

> 
> I'd suggest the following changes:
> - Update ghes_get_device() to check a flag latched on ghes_probe().

Still need your elaborating about the details of the flag. i.e. When is this flag
latched? When is it unlatched?

> - Remove 'force' argument from ghes_get_device().  ghes_edac_init() is too
> late to set this flag.  Also, other edac drivers should not be able to control this
> flag.  I think this force_load kernel option needs to belong to ghes with this
> scheme.
Agree, I will remove force_load in ghes_get_device(), and put all check/update of
force_load in ghes

> - ghes_get_device() exposes 'ghes_devs' to chipset-specific edac drivers, which
> should be internal to ghes.  ghes_get_device() may be renamed to something
> like ghes_edac_preferred() which returns true / false.
> 

Okay, agree

--
Cheers,
Justin (Jia He)