Received-SPF: pass (google.com: domain of linux-kernel+bounces-184027-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>,
 Jonathan Cameron <jonathan.cameron@huawei.com>,
 Dave Jiang <dave.jiang@intel.com>,
 Alison Schofield <alison.schofield@intel.com>,
 Vishal Verma <vishal.l.verma@intel.com>, linux-cxl@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>
Subject:
 Re: [PATCH v3] cxl/events: Use a common struct for DRAM and General Media
 events
Date: Mon, 20 May 2024 19:55:17 +0200
Message-ID: <4446774.UPlyArG6xL@fdefranc-mobl3>
In-Reply-To: <20240518113317.3683718-1-fabio.m.de.francesco@linux.intel.com>
References: <20240518113317.3683718-1-fabio.m.de.francesco@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

On Saturday, May 18, 2024 1:26:21=E2=80=AFPM GMT+2 Fabio M. De Francesco wr=
ote:
> cxl_event_common was an unfortunate naming choice and caused confusion wi=
th
> the existing Common Event Record. Furthermore, its fields didn't map all
> the common information between DRAM and General Media Events.
>=20
> Remove cxl_event_common and introduce cxl_event_media_hdr to record common
> information between DRAM and General Media events.
>=20
> cxl_event_media_hdr, which is embedded in both cxl_event_gen_media and
> cxl_event_dram, leverages the commonalities between the two events to
> simplify their respective handling.
>=20
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.co=
m>
> ---
>=20
> [...]
> -
>  union cxl_event {
>  	struct cxl_event_generic generic;
>  	struct cxl_event_gen_media gen_media;
>  	struct cxl_event_dram dram;
>  	struct cxl_event_mem_module mem_module;
> -	struct cxl_event_common common;
> +	struct cxl_event_media_hdr media_hdr;
>  } __packed;

Today I was thinking about a comment from Ira. He didn't like the addition =
of=20
an event that is not in the specs.[0] (Notice that the other issues have be=
en=20
already addressed).=20

I dislike the addition of an artificial event for the very same reasons Ira=
=20
expressed.

This additional event could be easily removed by something simple like the=
=20
following diff.

=46abio

[0] https://lore.kernel.org/linux-cxl/66467b8b47170_8c79294b3@iweiny-mobl.n=
otmuch/

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a08f050cc1ca..05de8836adea 100644
=2D-- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -875,7 +875,13 @@ void cxl_event_trace_record(const struct cxl_memdev=20
*cxlmd,
                guard(rwsem_read)(&cxl_region_rwsem);
                guard(rwsem_read)(&cxl_dpa_rwsem);
=20
=2D               dpa =3D le64_to_cpu(evt->media_hdr.phys_addr) & CXL_DPA_M=
ASK;
+               if (event_type =3D=3D CXL_CPER_EVENT_GEN_MEDIA)
+                       dpa =3D le64_to_cpu(evt->gen_media.media_hdr.phys_a=
ddr)
+                             & CXL_DPA_MASK;
+               else if (event_type =3D=3D CXL_CPER_EVENT_GEN_MEDIA)
+                       dpa =3D le64_to_cpu(evt->dram.media_hdr.phys_addr)
+                             & CXL_DPA_MASK;
+
                cxlr =3D cxl_dpa_to_region(cxlmd, dpa);
                if (cxlr)
                        hpa =3D cxl_trace_hpa(cxlr, cxlmd, dpa);
diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
index 6562663a036d..f0a5be131e6a 100644
=2D-- a/include/linux/cxl-event.h
+++ b/include/linux/cxl-event.h
@@ -97,7 +97,6 @@ union cxl_event {
        struct cxl_event_gen_media gen_media;
        struct cxl_event_dram dram;
        struct cxl_event_mem_module mem_module;
=2D       struct cxl_event_media_hdr media_hdr;
 } __packed;
=20
 /*