Received-SPF: pass (google.com: domain of linux-kernel+bounces-182135-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>
To: Dan Williams <dan.j.williams@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>, Ira Weiny <ira.weiny@intel.com>,
 linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org
Subject:
 Re: [PATCH v2] cxl/events: Use a common struct for DRAM and General Media
 events
Date: Fri, 17 May 2024 15:16:09 +0200
Message-ID: <13630976.tdPhlSkOF2@fdefranc-mobl3>
In-Reply-To: <6646a835ac144_2c26294da@dwillia2-xfh.jf.intel.com.notmuch>
References:
 <20240516102116.3512377-1-fabio.m.de.francesco@linux.intel.com>
 <6646a835ac144_2c26294da@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

On Friday, May 17, 2024 2:43:33=E2=80=AFAM GMT+2 Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > cxl_event_common was a poor naming choice and caused confusion with the
> > existing Common Event Record.
>=20
> I would say "unfortunate" rather than "poor".
>

Agreed.
=20
> > Use cxl_event_media as a common structure to record information about D=
RAM
> > and General Media events because it simplifies handling the two events.
> >=20
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Fixes: 6aec00139d3a ("cxl/core: Add region info to cxl_general_media and
> > cxl_dram events")
> What is the fix? There is no user visible behavior changes that results f=
rom
> this cleanup, right?

Right.
=20
> > Signed-off-by: Fabio M. De Francesco
> > <fabio.m.de.francesco@linux.intel.com>
> > ---
> >=20
> > Changes for v2:
> > 	- Extend the commit message (Alison);
> > 	- Add a "Fixes" tag (Alison, thanks).
> > =09
> >  drivers/cxl/core/mbox.c      |  6 ++--
> >  drivers/cxl/core/trace.h     |  4 +--
> >  include/linux/cxl-event.h    | 70 +++++++++++++++---------------------
> >  tools/testing/cxl/test/mem.c |  4 +--
> >  4 files changed, 36 insertions(+), 48 deletions(-)
>=20
> Oh, nice, net reduction in code I was not expecting.
>=20
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2626f3fff201..ad4d7b0f7f4d 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -875,16 +875,16 @@ void cxl_event_trace_record(const struct cxl_memd=
ev
> > *cxlmd,>=20
> >  		guard(rwsem_read)(&cxl_region_rwsem);
> >  		guard(rwsem_read)(&cxl_dpa_rwsem);
> >=20
> > -		dpa =3D le64_to_cpu(evt->common.phys_addr) &=20
CXL_DPA_MASK;
> > +		dpa =3D le64_to_cpu(evt->media_common.phys_addr) &=20
CXL_DPA_MASK;
>=20
> I was hoping at the end of this to get rid of the word "common" to avoid =
any
> more "common event record confusion".

Right, still an unfortunate naming choice.=20

> > [...]
>=20
> Oh nice you went for the full 'struct_group_tagged' proposal. However, gi=
ven
> that Jonathan is already asking questions about "__packed", lets do the
> simpler change, something like this, what do you think?=20

I think that these simpler changes are better.=20

> This also preserves
> type-safety.
>=20
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 60b25020281f..f48b59943977 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -33,14 +33,16 @@ struct cxl_event_generic {
>   */
>  #define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10
>  struct cxl_event_gen_media {
> -       struct cxl_event_record_hdr hdr;
> -       __le64 phys_addr;
> -       u8 descriptor;
> -       u8 type;
> -       u8 transaction_type;
> -       u8 validity_flags[2];
> -       u8 channel;
> -       u8 rank;
> +       struct cxl_event_media_hdr {
> +               struct cxl_event_record_hdr hdr;
> +               __le64 phys_addr;
> +               u8 descriptor;
> +               u8 type;
> +               u8 transaction_type;
> +               u8 validity_flags[2];
> +               u8 channel;
> +               u8 rank;
> +       } hdr;
>         u8 device[3];
>         u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
>         u8 reserved[46];
> @@ -52,14 +54,7 @@ struct cxl_event_gen_media {
>   */
>  #define CXL_EVENT_DER_CORRECTION_MASK_SIZE     0x20
>  struct cxl_event_dram {
> -       struct cxl_event_record_hdr hdr;
> -       __le64 phys_addr;
> -       u8 descriptor;
> -       u8 type;
> -       u8 transaction_type;
> -       u8 validity_flags[2];
> -       u8 channel;
> -       u8 rank;
> +       struct cxl_event_media_hdr hdr;
>         u8 nibble_mask[3];
>         u8 bank_group;
>         u8 bank;
> @@ -109,7 +104,7 @@ union cxl_event {
>         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;
>=20
>  /*

I'll make v3 according to the suggestion showed above.

Thanks,

=46abio