Received-SPF: pass (google.com: domain of linux-kernel+bounces-21401-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 9 Jan 2024 12:49:24 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Dan Williams
	<dan.j.williams@intel.com>
CC: Ira Weiny <ira.weiny@intel.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@amd.com>, Shiju Jose <shiju.jose@huawei.com>,
	Yazen Ghannam <yazen.ghannam@amd.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>, Alison Schofield
	<alison.schofield@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, "Ard
 Biesheuvel" <ardb@kernel.org>, <linux-efi@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>, "Rafael J.
 Wysocki" <rafael@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v5 0/9] efi/cxl-cper: Report CPER CXL component events
 through trace events
Message-ID: <659db15452090_24a8294f4@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231220-cxl-cper-v5-0-1bb8a4ca2c7a@intel.com>
 <20240108165855.00002f5a@Huawei.com>
 <eefc5888-4610-8e39-61ed-2d84e9ebf255@amd.com>
 <659caa8da651c_127da22947b@dwillia2-xfh.jf.intel.com.notmuch>
 <659cb0295ac1_8d749294b@iweiny-mobl.notmuch>
 <659cb684deb2d_127da22945a@dwillia2-xfh.jf.intel.com.notmuch>
 <20240109160435.00004a4a@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240109160435.00004a4a@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?bAXvCkCG9fn4VXSyhTUhv66MmSwKP2aYEwneSLnHFtxyn5GHJ338M7gn09pQ?=
 =?us-ascii?Q?ALg+bt9VZQQQ7IyNMdUHmOIzAYELihIbpI75q4oaqzRnahWTV9ayXBfU6MyN?=
 =?us-ascii?Q?hY/6AO6wZ1hDZqRna+jjpzzbyn8mFBRpG0qFofXyRPYx/12RrE8MMGfq4ZYM?=
 =?us-ascii?Q?58xtjqe3S3fzaGPGB4L0QX2EVfdyVs9bQ86DCOjDWT8zNM/ypef5OHFHL0ez?=
 =?us-ascii?Q?/7LVAnGopJM0iUtyAKmDZBUxZ86C0iSNiPPhbkbFOkXNRHcGRqmxa3O8DeJJ?=
 =?us-ascii?Q?9j4+SwRn+MLifxbHm+Ku4lWZp00S2diTbPkub6K84fY2x+7VicoyoiICJk+J?=
 =?us-ascii?Q?4MbZbZ7EiTaB0ma4CLa2ntY1ikp45F4bCvM+I1arBP7kIQKjphi/jA593+r9?=
 =?us-ascii?Q?YIk2M7WcOSLZlh4Rhc/G4IqxsCxogjYBrh20nGDfBRN5JqxSZq8fprichBQO?=
 =?us-ascii?Q?c5MWCLIcILuCQIzV8sniHSI7iAISq/a6l5CGYBFG9+ILZzVPn1Z5mc0+N3x9?=
 =?us-ascii?Q?KKwWRdQltRFQHXXZpW8hFLTVpo3W6AoDfyBs87wWAMmB3kBnhsOSXWwAhXPN?=
 =?us-ascii?Q?7YjfGoBWpmJonfNHGyOD5z5a8XlKDDk9/iSJYqh6dVvwniDqHrrsP2ZFlACg?=
 =?us-ascii?Q?lmjrhgA16kOSd41TvidMOewwCrKPytKI9OLQLJzmXtfBuGPv+/Y+pQT0/1/v?=
 =?us-ascii?Q?S4lnst1afAxI7kPsOJUWeS+pDdOPnOVnakUp5lcj39EclQKOQzSrX3djwpYl?=
 =?us-ascii?Q?VR0GU31lFiI1jmi6WowX4ki25O+L1YMJ93W8Vmt8JbFrtwPrwc5KKWU1UQsy?=
 =?us-ascii?Q?Ngz9x8QiEKqViqjLAb7U8ndI1RYv+fIBrcPQvP7tIlyUas5j7uEpj5sUzaFl?=
 =?us-ascii?Q?W2xlBWvc2uNh5vSLldy7hku8YXgtk9SVf7UJKYqXNnZESNQa7JZcZ7fYWT08?=
 =?us-ascii?Q?55SQ2d75llgRnAqx0xhzVMdIZI1y7T0GQLUxW5P22Sj3Us/Iwj/R3uvmBPCO?=
 =?us-ascii?Q?i5GUxF0xXssulJGUxR5rXw3f1sfJU5CYVnTIdISWLAPqKHqg5fyMg0LP+e7d?=
 =?us-ascii?Q?1DXlXH1cOJDTvQcPV0WlcdBTpp8tcybLIRIRa1VLzNxESjlOSvqWzY+4l/CM?=
 =?us-ascii?Q?F03mXqhAXIDj9gEIYfIqd7jlGrtXowjwJPK9SVYcXVdMgPZuwCHFLEhE+GqP?=
 =?us-ascii?Q?lCpLxGkC0cXe3GEZTbL/GrvZKfPMStTMhjDLNg0xDQ0wqu8pgJXUc+aXNASt?=
 =?us-ascii?Q?PXpRl2iCtnAfKY2PT2iAeKKQjChsdUYLEnqFfzYvvrG73z6zwcU0Ec4SKws7?=
 =?us-ascii?Q?eDttHW8WD36jaKoFLqsO/NCYwugIHRzPoiTkZ+iIQTuj3z0O+TRwuG+k6ZJD?=
 =?us-ascii?Q?+71jQY7dx3z2j5v7qMP+gyjARm3Y+R+fgvXvUIZDlpzANK+eqdQ85ekQ7TZi?=
 =?us-ascii?Q?an7CrerbeFin4C4iuFBkgGgshZwq4TPN9u4VwSNuIn0dFN38VNF35+ULlDme?=
 =?us-ascii?Q?q4wDSZtefIQkTUzz+q54S/T1wIb+4MAqU/dr88lKbMGVwE1Kcr7mfWsIAuPM?=
 =?us-ascii?Q?DFS9ScMG34Chi4hURrJEufEV5qazL+sW2VvkJBs+Ljp50cNfJ6OY/WVrS2Wi?=
 =?us-ascii?Q?Rw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 46875dc2-763e-453c-d15a-08dc1154784f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 20:49:27.4699
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oPlnTtcAMXThN5DyMqGXrIjC401S4ftaUTf7BURbTJe3uMdbV4avetOjqU2QENzD6yZT/XaANrsjP5usJ0uWMnu3M+vX2cNC4g+RCoJB8R8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6978
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Mon, 8 Jan 2024 18:59:16 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Ira Weiny wrote:
> > > Dan Williams wrote:  
> > > > Smita Koralahalli wrote:  
> > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote:  
> > > > > > On Wed, 20 Dec 2023 16:17:27 -0800
> > > > > > Ira Weiny <ira.weiny@intel.com> wrote:
> > > > > >   
> > > > > >> Series status/background
> > > > > >> ========================
> > > > > >>
> > > > > >> Smita has been a great help with this series.  Thank you again!
> > > > > >>
> > > > > >> Smita's testing found that the GHES code ended up printing the events
> > > > > >> twice.  This version avoids the duplicate print by calling the callback
> > > > > >> from the GHES code instead of the EFI code as suggested by Dan.  
> > > > > > 
> > > > > > I'm not sure this is working as intended.
> > > > > > 
> > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus()
> > > > > > and now the EFI code handling that pretty printed things is missing we get
> > > > > > the horrible kernel logging for an unknown block instead.
> > > > > > 
> > > > > > So I think we need some minimal code in cper.c to match the guids then not
> > > > > > log them (on basis we are arguing there is no need for new cper records).
> > > > > > Otherwise we are in for some messy kernel logs
> > > > > > 
> > > > > > Something like:
> > > > > > 
> > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1
> > > > > > {1}[Hardware Error]: event severity: recoverable
> > > > > > {1}[Hardware Error]:  Error 0, type: recoverable
> > > > > > {1}[Hardware Error]:   section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6
> > > > > > {1}[Hardware Error]:   section length: 0x90
> > > > > > {1}[Hardware Error]:   00000000: 00000090 00000007 00000000 0d938086  ................
> > > > > > {1}[Hardware Error]:   00000010: 00100000 00000000 00040000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000020: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000030: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000040: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000050: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000060: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000070: 00000000 00000000 00000000 00000000  ................
> > > > > > {1}[Hardware Error]:   00000080: 00000000 00000000 00000000 00000000  ................
> > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags=''
> > > > > > 
> > > > > > (I'm filling the record with 0s currently)  
> > > > > 
> > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there 
> > > > > in dmesg from EFI as the handling is done in trace events from GHES.
> > > > > 
> > > > > If, we need to handle from EFI, then it would be a good reason to move 
> > > > > the GUIDs out from GHES and place it in a common location for EFI/cper 
> > > > > to share similar to protocol errors.  
> > > > 
> > > > Ah, yes, my expectation was more aligned with Jonathan's observation to
> > > > do the processing in GHES code *and* skip the processing in the CPER
> > > > code, something like:
> > > >   
> > > 
> > > Agreed this was intended I did not realize the above.
> > >   
> > > > 
> > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > > > index 35c37f667781..0a4eed470750 100644
> > > > --- a/drivers/firmware/efi/cper.c
> > > > +++ b/drivers/firmware/efi/cper.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/bcd.h>
> > > >  #include <acpi/ghes.h>
> > > >  #include <ras/ras_event.h>
> > > > +#include <linux/cxl-event.h>
> > > >  #include "cper_cxl.h"
> > > >  
> > > >  /*
> > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
> > > >  			cper_print_prot_err(newpfx, prot_err);
> > > >  		else
> > > >  			goto err_section_too_small;
> > > > +	} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
> > > > +		printk("%ssection_type: CXL General Media Error\n", newpfx);  
> > > 
> > > Do we want the printk's here?  I did not realize that a generic event
> > > would be printed.  So intention was nothing would be done on this path.  
> > 
> > I think we do otherwise the kernel will say
> > 
> >     {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1
> >     {1}[Hardware Error]: event severity: recoverable
> >     {1}[Hardware Error]:  Error 0, type: recoverable
> >     ...
> > 
> > ...leaving the user hanging vs:
> >  
> >     {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1
> >     {1}[Hardware Error]: event severity: recoverable
> >     {1}[Hardware Error]:  Error 0, type: recoverable
> >     {1}[Hardware Error]:   section type: General Media Error
> > 
> > ...as an indicator to go follow up with rasdaemon or whatever else is
> > doing the detailed monitoring of CXL events.
> 
> Agreed. Maybe push it out to a static const table though.
> As the argument was that we shouldn't be spitting out big logs in this
> modern world, let's make it easy for people to add more entries.
> 
> struct skip_me {
> 	guid_t guid;
> 	const char *name;
> };
> static const struct skip_me skip_me = {
> 	{ &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" },
> etc.
> };
> 
> for (i = 0; i < ARRAY_SIZE(skip_me); i++) {
> 	if (guid_equal(sec_type, skip_me[i].guid)) {
> 		printk("%asection_type: %s\n", newpfx, skip_me[i].name);
> 		break;	
> }
> 
> or something like that in the final else.

I like it.

Any concerns with that being an -rc fixup, and move ahead with the base
enabling for v6.8? I don't see that follow-on as a reason to push the
whole thing to v6.9.