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)
