Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Elliott, Robert (Servers)" <elliott@hpe.com>
To:     "Kani, Toshi" <toshi.kani@hpe.com>, Justin He <Justin.He@arm.com>
CC:     "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
        "devel@acpica.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" <linux-efi@vger.kernel.org>,
        nd <nd@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>
Subject: RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from
 loading after ghes_edac is registered
Thread-Topic: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac from
 loading after ghes_edac is registered
Thread-Index: AQHYskbLB3dm9/vQRkSlm6bwr07US621cbmAgACYT4CAAHk+gIAADlig
Date:   Fri, 19 Aug 2022 18:57:11 +0000
Message-ID: <MW5PR84MB18429A4A41FD70DCD91CFADDAB6C9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
References: <20220817143458.335938-1-justin.he@arm.com>
 <20220817143458.335938-6-justin.he@arm.com>
 <DM4PR84MB18538A56870A280CDC4637A7826C9@DM4PR84MB1853.NAMPRD84.PROD.OUTLOOK.COM>
 <DBBPR08MB453891E87563F1BBE291248AF76C9@DBBPR08MB4538.eurprd08.prod.outlook.com>
 <DM4PR84MB1853B213F2F45E495D9D6446826C9@DM4PR84MB1853.NAMPRD84.PROD.OUTLOOK.COM>
In-Reply-To: <DM4PR84MB1853B213F2F45E495D9D6446826C9@DM4PR84MB1853.NAMPRD84.PROD.OUTLOOK.COM>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7exOesFobQmxM8QoGiIIfYXFPO+eiK82MwahZKoPCwx2Kz/2A1gXN1Ln7vGS?=
 =?us-ascii?Q?ZCdcFsukbc6imSApIDZodIZI/UxUzGZWGyWcO8OSYYSv/GZJreZ3G1VYy2uC?=
 =?us-ascii?Q?tNZy0Ljz10fOgEt2TQB6FQCSwBT4RKWl1eVyISxu5REXYQxtXVJM1bMnXW8Z?=
 =?us-ascii?Q?/BEczO4eBEW5a/0K9DjhNyrsfASp0WayCSOTh9U+bgAzx+GQJUv/9CB5BeHB?=
 =?us-ascii?Q?xoKhfr3MFVP+gBrx3vMvJ+Etl6Lv1prT+8QsDyjsunvM+uAwSsrNYGGRKuGh?=
 =?us-ascii?Q?Le4zQjRNvkryhEv/K1LXmfOXIeJNsn/JKeRMhxx7HtALw4/l5AEIQNnc9/iT?=
 =?us-ascii?Q?tAhb4kl4+O8CEIi/CT+Q36IXlDCPEHOhpZAyrvnTCqHNomOJ6gASeVp4czkV?=
 =?us-ascii?Q?zSRaQQkAH5soDzg6pi9x9iN2TXLUGS1IJDOpcLrrP5OP/5xRY6T/dpuwf6EF?=
 =?us-ascii?Q?XvzeNwPqquALttChX7lhOe7DdvRJDJ042dhff88yw3ouigDeRBSlD/botPc1?=
 =?us-ascii?Q?94pl5n/A64iGy/9LnlDfq3wAOHdG58AGOykWkNjhedA3rc76Vt/Lt5HUaM5b?=
 =?us-ascii?Q?jkf4L4fhpM8trtEV2hNgQSk1Ef/tvX0nQx5t/mRa1XPh/Fo1/w1F9vYkFj5P?=
 =?us-ascii?Q?p5iczc2ir850BIwDW5IsUeW3GBQo426CQoiEcLPGpR+1lYeaHOSip3kaT/g6?=
 =?us-ascii?Q?Mt9FQjjqUJr6t7oKvtu+nHIII7hCxgXhN0DUpm7JO32KHH7YvmODJlQBcjv/?=
 =?us-ascii?Q?bOAPtkN5Qr5kbvYN6ECEDFcAeMHU7fCJAiuY7MwAFR04E1tqILej2mFaHbnq?=
 =?us-ascii?Q?T7Pfrm9XZkA5b67mr6g9euROWJgBJJIzhww/AaFZGkb6UmQyF5IwOzYrdAs9?=
 =?us-ascii?Q?HHQOyfpB7Gc5mOrFgo52631C42rhiiueV2rt4k/grmDJgZnY7x7nzYT180p/?=
 =?us-ascii?Q?VQh4FjuIa2gq6NBrqN4lYC8a6eCW9Sc0V9QhXG1tx7Q0qFCamAdzGQptcE+d?=
 =?us-ascii?Q?QVCbGIkNkDD2MvouallWEZvWvHrsaiTfNiMujIoeX2S84daLkh8nqoPOvWR0?=
 =?us-ascii?Q?NdoT1VCb0c3TK/hOIhg1mI69zk9aQMjIFOEhpZJRMS/loAlhk8iehgsO9+Os?=
 =?us-ascii?Q?g5tb7kleMO72g4hoPxHLWKVBbH2nERacrVuuMnkpiheSLKSvxWhRu3Z7Tpwl?=
 =?us-ascii?Q?YZweEBHkd1p/xcbK5jqwYemwn/MktGU3AjZFc6M2X3ikYE8ZZfswypJORpKO?=
 =?us-ascii?Q?GOlvoYE52AYcPKBtBYNfVHWxG1eRHubteUYej4Gz5zKDKzx5k0Ao6r1inl8O?=
 =?us-ascii?Q?kbb2nrja2sL0fGOmJS1IR/ygr8iiusZFFPlXUsmzTwm3jMmz3uRiY8wWc3uH?=
 =?us-ascii?Q?4HaXiLDL0r6zX3v1bTr3ctuD/UhGsusftMkyN4OUW7XVkL6jNByDvadPWd5d?=
 =?us-ascii?Q?hy/A+Dq025l4oLLBm9iy/UBjeYRsIo/hdYUquTktbetsvM2XdFHz5XL2qJFl?=
 =?us-ascii?Q?xdPRoOta/38QvqLFpnHdRswSAxcL47DglbqjidnUJoBLkUpC21l1b7qFg2K5?=
 =?us-ascii?Q?wAmIBfnORdt9lJfM6BY=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-Network-Message-Id: 8705f8ee-4540-475f-1767-08da82149fa9
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Aug 2022 18:57:11.5806
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /hSnuS7VGWMgC4sdAjomT3Nri0dfFBA8+7MbsGXx2aKLrvVOTPZL7K/hZew9OOOi
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR84MB1853
X-OriginatorOrg: hpe.com
X-Proofpoint-ORIG-GUID: W6J4TelboVfCQrALU8y93pDk0PY6AlEy
X-Proofpoint-GUID: W6J4TelboVfCQrALU8y93pDk0PY6AlEy
X-HPE-SCL: -1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-19_10,2022-08-18_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501
 malwarescore=0 suspectscore=0 mlxlogscore=999 lowpriorityscore=0
 impostorscore=0 phishscore=0 mlxscore=0 clxscore=1011 adultscore=0
 bulkscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2207270000 definitions=main-2208190070
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Kani, Toshi <toshi.kani@hpe.com>
> Sent: Friday, August 19, 2022 12:49 PM
> To: Justin He <Justin.He@arm.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>; 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>
> Subject: RE: [PATCH v2 5/7] EDAC/ghes: Prevent chipset-specific edac
> from loading after ghes_edac is registered
>=20
> On Friday, August 19, 2022 4:35 AM, Justin He wrote:
> > > > @@ -1382,6 +1395,7 @@ static int ghes_probe(struct
> platform_device
> > > > *ghes_dev)
> > > >  	platform_set_drvdata(ghes_dev, ghes);
> > > >
> > > >  	ghes->dev =3D &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.
>=20
> Using (ghes_edac-registered) is a wrong check in this scheme
> since ghes_edac registration is deferred.  This check is fine in
> the current scheme since ghes_edac gets registered before
> any other chipset-specific edac drivers.
>=20
> > 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)?
>=20
> No.  The point is that ghes_edac driver needs to be selected,
> "regardless of the module ordering", on a system with GHES
> present & preferred.
>
> Note that this new scheme leads to the following condition
> change:
> - Current: (ghes-present) && (ghes-preferred) && (ghes_edac registered)
> - New: (ghes-present) && (ghes-preferred)
>=20
> The option I suggested previously keeps the current condition,
> but this new scheme does not for better modularity.
>=20
> What this means is that if ghes_edac is not enabled (but ghes
> is enabled) on a system with GHES present & preferred, no edac
> driver gets registered.  This change is fine from my (HPE) perspective
> and should be fine for other GHES systems.  GHES systems have
> chipset-specific edac driver in FW.  OS-based chipset-specific edac
> driver is not necessary and may lead to a conflict of chipset register
> ownership.

Currently, running with this on the kernel command line
	ghes.disable

causes the ACPI ghes driver to quit early in acpi_ghes_init():

  /*
   * This driver isn't really modular, however for the time being,
   * continuing to use module_param is the easiest way to remain
   * compatible with existing boot arg use cases.
   */
  bool ghes_disable;
  module_param_named(disable, ghes_disable, bool, 0);

which results in the skx_edac module assuming it should run:
  [   33.628140] calling  skx_init+0x0/0xe5a [skx_edac] @ 1444
  [   33.628531] EDAC MC0: Giving out device to module skx_edac controller =
Skylake Socket#0 IMC#0: DEV 0000:36:0a.0 (INTERRUPT)
  [   33.641432] EDAC MC1: Giving out device to module skx_edac controller =
Skylake Socket#0 IMC#1: DEV 0000:36:0c.0 (INTERRUPT)
  [   33.653256] EDAC MC2: Giving out device to module skx_edac controller =
Skylake Socket#1 IMC#0: DEV 0000:ae:0a.0 (INTERRUPT)
  [   33.665055] EDAC MC3: Giving out device to module skx_edac controller =
Skylake Socket#1 IMC#1: DEV 0000:ae:0c.0 (INTERRUPT)
  [   33.676801] initcall skx_init+0x0/0xe5a [skx_edac] returned 0 after 36=
343 usecs

We might need to differentiate between the system ROM really not
offering GHES vs. the ghes module not running.