Received-SPF: pass (google.com: domain of linux-kernel+bounces-184289-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 20 May 2024 16:11:32 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: "Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>, "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>, Dan Williams <dan.j.williams@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC: "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
Message-ID: <664bd8a41ca0f_a028294e8@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240518113317.3683718-1-fabio.m.de.francesco@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240518113317.3683718-1-fabio.m.de.francesco@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8LWLNK+418O6QP/1rzv+Dl5OkYEh6DAMfq9XbU2LO0gcY8nJ+D6viEYET9GJ?=
 =?us-ascii?Q?SVKTnoPkojC6pFXziDqD4yCAWZW0ES9NEdTSwDZ6kOHnyiWH+OsDaSvtzMyN?=
 =?us-ascii?Q?kSgBRBouiLs9s29M38L4dMYHxKUdvDO2fzJ5PBIWE25/EYJcqMHa5Pz66t3F?=
 =?us-ascii?Q?0ZlelRRThGBOTlWID7kNfztfGm1THuze9tXLF/dATculktvvukfYZK3U+PCo?=
 =?us-ascii?Q?Xi5LRmcwqlwipMbo/YDRiH35lq8ZURTA2ZY9ZQKh+AHhswxBkHPF9QXzmpGi?=
 =?us-ascii?Q?vOW8PXs3LwiDSOnkgn44Icy0LlYVQ00IKUiyLTDTF4QNAMV+JuuQbG0Bs2hq?=
 =?us-ascii?Q?Irak37m7Zzxkjgt8B066HYs9AVsPPauj51CW+VMiSdzQwL9uMc8mnow+uiq6?=
 =?us-ascii?Q?+cXXbSEO1OLo6kJpTH0M3b7VD/z/9+2foFh6oK9+YjGUxo3Y2PeZMjMep4hs?=
 =?us-ascii?Q?nXIvoe5OscZ34JkLzvJBIrCXzqD8j5g8UmSqQN493zJck+7Z2tzXJaafL5o7?=
 =?us-ascii?Q?D0JigiYQsWQhw9DC/mo+lVWeVtG67raMGpsgLH9ZOK1aZjC8MZJUQ/k8/L5+?=
 =?us-ascii?Q?f2zVFoccHtMVJUl5cEvyHSW9vWhG7PkYt9obuu8Fc0kJbUchVAWKtW+Rw64V?=
 =?us-ascii?Q?jXvYSEU940Mgd/kyDUvMFN+F8TYmIc50F70Rm9IGQ6dTumSEGBOCbnng9Udn?=
 =?us-ascii?Q?rGs/J7tOFt5+xjF6Vi8xf6zx8YyDBISFystNA6w9SvKf/5PUCLchjz8+DVaU?=
 =?us-ascii?Q?SxVORaIf33ZzELus2Yoe6wqtx7Qo/6BgT4baxugCPs0ugG2g3DwV93BKG6uP?=
 =?us-ascii?Q?WBQ2zftaDrZewZhKN9F5UYHdMyushTze14pBEKXrSHMJsHBYgSo8iEDmSSbD?=
 =?us-ascii?Q?sTeZjMjt9DQ/M3CDCZ9oM4NFtIS7M69FJqro/gcHjFk/j8T++72cGq49G7iw?=
 =?us-ascii?Q?n2NkFxvKil5BdNniZD1ib2BCK3KCDq74zz0tL54g7hXNDKRKoBEpodi8a2Y7?=
 =?us-ascii?Q?yTDtoxfCeUdbcV91rWioAi2uoh3c8jKHgLcnyPoK39rBLm9pmOtEYaTbPoAs?=
 =?us-ascii?Q?LB++2nx8FBYMvy29F3EaPw43Onp+5lZ2ONY23MJ+VhWYiaPHOodHfXokHb3g?=
 =?us-ascii?Q?zCzTOdm2i5zQhZ66uoN48G9wBI+4jCgWotpxHpwIaDBxt6jbiGNo98g0cwVt?=
 =?us-ascii?Q?ObxIsP1YLro1E+FQJtM1ViRrIk4pQ6eIOe5Jgt4iF5BUvsL7/svWuLoa27ei?=
 =?us-ascii?Q?AykC9kFcFZCAbsHkSDIsaBkUOD5ILyrXNlVzwdMKvSxAlhclnPtwKEdeIYND?=
 =?us-ascii?Q?JNMJVcuA6rGfX8SgOQ1PDSWGoGfXgJRr2HQp3TXGss8wk2eVCaX85jJjDPNw?=
 =?us-ascii?Q?JEn7DoLuHi6eqoQ6ILy3n7TSRi8O+682pbw+yu3zcO/BgAAk1YriTJ8GTkPb?=
 =?us-ascii?Q?nY7XY7pBCiFKLlhPi9rDLqEnCNTeT0hMqhh3N29XB3VrmihTmCVH364hsmZP?=
 =?us-ascii?Q?ZsaYVoocHJFlGFwauPYbS68RnriF19zzTBmRFAJVtVeltCCKgrJOBcQajOLu?=
 =?us-ascii?Q?9cra40nAsps3rshf0f+tTK1i/S8c558suHCheG8sO4u1mt7mMQYKLvbNWOgQ?=
 =?us-ascii?Q?Mw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b2bfe72d-ab7e-4976-a8d6-08dc79223150
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2024 23:11:34.5170
 (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: GeF+jvZRgwomd6PreVhX0VwBGqIY2yRfulcsRZ1L3dTfxKR6oTf4XL0Lp6802BO3tHCBKW5FdRMShDUa7Ky6lqUYKKNvGCaxOp4jmIMjRAo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4771
X-OriginatorOrg: intel.com

Fabio M. De Francesco wrote:
> cxl_event_common was an unfortunate naming choice and caused confusion with
> the existing Common Event Record. Furthermore, its fields didn't map all
> the common information between DRAM and General Media Events.
> 
> Remove cxl_event_common and introduce cxl_event_media_hdr to record common
> information between DRAM and General Media events.
> 
> 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.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
> 
> - Changes for v3 -
> 
> 	- Rework the layout of cxl_event_dram and cxl_event_gen_media
> 	  to make a simpler change (Dan)
> 	- Remove a "Fixes" tag (Dan)
> 	- Don't use unnecessary struct_group[_tagged] (Jonathan, Ira)
> 	- Rewrite end extend the commit message
> 
> - Link to v2 -
> 
> 	https://lore.kernel.org/linux-cxl/20240516102116.3512377-1-fabio.m.de.francesco@linux.intel.com/
> 
> 
>  drivers/cxl/core/mbox.c      |  2 +-
>  drivers/cxl/core/trace.h     | 32 ++++++++++++++---------------
>  include/linux/cxl-event.h    | 40 +++++++++++++-----------------------
>  tools/testing/cxl/test/mem.c | 30 +++++++++++++--------------
>  4 files changed, 46 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..a08f050cc1ca 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -875,7 +875,7 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>  		guard(rwsem_read)(&cxl_region_rwsem);
>  		guard(rwsem_read)(&cxl_dpa_rwsem);
>  
> -		dpa = le64_to_cpu(evt->common.phys_addr) & CXL_DPA_MASK;
> +		dpa = le64_to_cpu(evt->media_hdr.phys_addr) & CXL_DPA_MASK;
>  		cxlr = cxl_dpa_to_region(cxlmd, dpa);
>  		if (cxlr)
>  			hpa = cxl_trace_hpa(cxlr, cxlmd, dpa);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 07a0394b1d99..5a76f94accf4 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -340,23 +340,23 @@ TRACE_EVENT(cxl_general_media,
[..]

Changes look good.

> @@ -440,19 +440,19 @@ TRACE_EVENT(cxl_dram,
[..]

Looks good.

[..]
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 6584443144de..63b71541d399 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -384,19 +384,19 @@ struct cxl_test_gen_media {
>  struct cxl_test_gen_media gen_media = {
>  	.id = CXL_EVENT_GEN_MEDIA_UUID,
>  	.rec = {
> -		.hdr = {
> +		.media_hdr.hdr = {
>  			.length = sizeof(struct cxl_test_gen_media),
>  			.flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
>  			/* .handle = Set dynamically */
>  			.related_handle = cpu_to_le16(0),
>  		},
> -		.phys_addr = cpu_to_le64(0x2000),
> -		.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> -		.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> -		.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
> +		.media_hdr.phys_addr = cpu_to_le64(0x2000),
> +		.media_hdr.descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
> +		.media_hdr.type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
> +		.media_hdr.transaction_type = CXL_GMER_TRANS_HOST_WRITE,
>  		/* .validity_flags = <set below> */
> -		.channel = 1,
> -		.rank = 30
> +		.media_hdr.channel = 1,
> +		.media_hdr.rank = 30

This looks awkward, I would have expected a conversion like:

@@ -384,19 +384,21 @@ struct cxl_test_gen_media {
 struct cxl_test_gen_media gen_media = {
        .id = CXL_EVENT_GEN_MEDIA_UUID,
        .rec = {
-               .hdr = {
-                       .length = sizeof(struct cxl_test_gen_media),
-                       .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
-                       /* .handle = Set dynamically */
-                       .related_handle = cpu_to_le16(0),
+               .media_hdr = {
+                       .hdr = {
+                               .length = sizeof(struct cxl_test_gen_media),
+                               .flags[0] = CXL_EVENT_RECORD_FLAG_PERMANENT,
+                               /* .handle = Set dynamically */
+                               .related_handle = cpu_to_le16(0),
+                       },
+                       .phys_addr = cpu_to_le64(0x2000),
+                       .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
+                       .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
+                       .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
+                       /* .validity_flags = <set below> */
+                       .channel = 1,
+                       .rank = 30
                },
-               .phys_addr = cpu_to_le64(0x2000),
-               .descriptor = CXL_GMER_EVT_DESC_UNCORECTABLE_EVENT,
-               .type = CXL_GMER_MEM_EVT_TYPE_DATA_PATH_ERROR,
-               .transaction_type = CXL_GMER_TRANS_HOST_WRITE,
-               /* .validity_flags = <set below> */
-               .channel = 1,
-               .rank = 30
        },
 };


>  	},
>  };
>  
> @@ -408,18 +408,18 @@ struct cxl_test_dram {
>  struct cxl_test_dram dram = {
>  	.id = CXL_EVENT_DRAM_UUID,
>  	.rec = {
> -		.hdr = {
> +		.media_hdr.hdr = {

Same comment as above, just scope the declarations with out extra
de-references.