Received-SPF: pass (google.com: domain of linux-kernel+bounces-20312-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 8 Jan 2024 18:59:16 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>, Jonathan Cameron
	<Jonathan.Cameron@huawei.com>
CC: Dan Williams <dan.j.williams@intel.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: <659cb684deb2d_127da22945a@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <659cb0295ac1_8d749294b@iweiny-mobl.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?T0gKZR8/VGsvvfO/Fz0mb/0N/8H9OKsoP/J6hN+85A/tO9x0iZW4se3uXmtA?=
 =?us-ascii?Q?dxyTfp1InmQDbCyFs6ECmvB+AUGd96hv8O6DJLTWlTNBZCQoXiBLnpxOHFiG?=
 =?us-ascii?Q?mdhPOSKSKyBtiU8TU64R8yXJcSO1RKCDArArIXONMurv7L18S3S8fz3sUa9f?=
 =?us-ascii?Q?yXyED+o8ysbGsaeg0MUVdekNlHhuL1Ek+jPunqDobB4Gqtw8c8bDXeD8WhTv?=
 =?us-ascii?Q?IIERD0bb7Flte9ijWsqykOBYxqBSYc9kDz+X7xrsDZ4buFgzjWvylsaTgFGn?=
 =?us-ascii?Q?yBiIiOzMjmcLVzsigW7aEHijtJ/aLyNjdk7EGTfEHXLWY4pXNs4wGXzDo/vQ?=
 =?us-ascii?Q?5R+Y/Nc7Akr2ejNXgl2vbHcRKTcujfTrpCWZ0uYlA25LkDZpFNAVaIYh9uLf?=
 =?us-ascii?Q?Z+/0M5ZUvfq6n6mCqNENW9KzAxiFVFGyyi+kY4oau/PhPUPNFtspNijGmmYK?=
 =?us-ascii?Q?mhkvAQiwvK+y3A1QqSswHQe56BYfNPGLHLKQWRLY/VwaVzVfeyLfKOz43Aii?=
 =?us-ascii?Q?lLEWf9d2rBsiD8r8pf5BOj+dUmNmWx5OSNA7u1OwVXuCdX5wxR4aKMfinlos?=
 =?us-ascii?Q?AXxcqfJG6xBmUZyLXuG1WMymTtnr3ZO/aAkceOTEJ9seGQoH2NQnQ7UeuXrR?=
 =?us-ascii?Q?5avr75/nYcU3HBcRc/uaG0oNsBaCNQkLPhCbpimwSv8MOySVaySXf/v2r7o1?=
 =?us-ascii?Q?n9oAJOLN/vm7AZ0fkW2idMaXCB3ak/yrTUoi4/obrBlkb5vdvkKPUJTVhfMv?=
 =?us-ascii?Q?LTBOCyy9vbAhiFcHdo0v8YlOPG0KRv58VuQHGADZlBAvaMaeybpnVsbXrAvR?=
 =?us-ascii?Q?BSzlWnKv/G46s8LYZgQdhlcD//wlV+axipzZbOFCJO5lviMBJMOJoxNIGwGq?=
 =?us-ascii?Q?CkapUQ6NCy8iwkjgZ+rwcVdMAM3NlXbVeiT+2R6tgID3LbvbKF0N5QOHaw1n?=
 =?us-ascii?Q?WMimyn4EUs0S636urXB/PzLE+R7u6lASgdBds3GuYpCbAWnFEs1MZVqYjpWH?=
 =?us-ascii?Q?W7PmPVh/GawsREMaDtnP6LZlYIlCmZMUbNst/TUz6MrI6L8ZobB+lQtkWDZ/?=
 =?us-ascii?Q?sZi3xOZB763LXb1ElgLTQH77ChWoG3oTRaRMmSss7fGmt0m//oAJhVua/4yG?=
 =?us-ascii?Q?Yr2tZ5pFMS7FHaXWfnl+GDtAO58e5ZnNHJ70vz/l2ryyg2YbJffPt0s0MP6I?=
 =?us-ascii?Q?rXqeAtOxuGPPIODvcewrKXG4fhalES/4UPZc+OkSMuUaTbTH2HvN3yI+/4UA?=
 =?us-ascii?Q?4QzIWXE+Zq0Sxuw+mN8wMWLyHkgABPKd2P5oQSjvPOadFUkOFjpvD0QUkK8z?=
 =?us-ascii?Q?kfRVT6kMZgpJvWlw/xpmC7H8fzm73OVHD+YgpRpig3LSDTHChcJdNKltv2uw?=
 =?us-ascii?Q?ZEjXnfKMP8VYKseT+NnCFljekKFoz+r8dj1XOHO1tw4jHj2/Ru3+aKOgWMQS?=
 =?us-ascii?Q?py58bcgkStdtWYI+jnkFwX8hNMVVcnJ29MFwTO5BapZSJ0oV6LXwegvYg5+L?=
 =?us-ascii?Q?c9OmR8y4iRrp0ww11AFSfuUn7JL+YURxLhsZXQb67xaZBYQ3ZhCzE3jOAM31?=
 =?us-ascii?Q?/GUNXuq3fPyASKJu5lEBDnDRr/MylUlNeGeThvmBSfLBhJwm+YiMmj3etO/F?=
 =?us-ascii?Q?9A=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 69ef4e86-4a40-4d8b-b8d2-08dc10bef9c2
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 02:59:20.1556
 (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: HBiSNEj+PES4AQb+C1DfYzASvLTeprm9xSGYUQAyPhyMpRyd2ko1ZSgKrqUhUF6bMFVzjfCl06zJqupBiBEB+uj5/J4b4GHWyf4nhr80eHc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6563
X-OriginatorOrg: intel.com

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.