Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from loading after ghes_edac is registered | Date | Fri, 19 Aug 2022 10:34:43 +0000 |
| |
Hi Toshi,
> -----Original Message----- > From: Kani, Toshi <toshi.kani@hpe.com> > Sent: Friday, August 19, 2022 9:30 AM > To: Justin He <Justin.He@arm.com>; Ard Biesheuvel <ardb@kernel.org>; Len > Brown <lenb@kernel.org>; James Morse <James.Morse@arm.com>; Tony Luck > <tony.luck@intel.com>; Borislav Petkov <bp@alien8.de>; Mauro Carvalho > Chehab <mchehab@kernel.org>; Robert Richter <rric@kernel.org>; Robert > Moore <robert.moore@intel.com>; Qiuxu Zhuo <qiuxu.zhuo@intel.com>; > Yazen Ghannam <yazen.ghannam@amd.com> > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-edac@vger.kernel.org; devel@acpica.org; Rafael J . Wysocki > <rafael@kernel.org>; Shuai Xue <xueshuai@linux.alibaba.com>; Jarkko > Sakkinen <jarkko@kernel.org>; linux-efi@vger.kernel.org; nd <nd@arm.com> > 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)
| |