Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Jun 2023 14:12:01 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Terry Bowman <terry.bowman@amd.com>, <alison.schofield@intel.com>,
        <vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
        <bwidawsk@kernel.org>, <dan.j.williams@intel.com>,
        <dave.jiang@intel.com>, <Jonathan.Cameron@huawei.com>,
        <linux-cxl@vger.kernel.org>
CC:     <terry.bowman@amd.com>, <rrichter@amd.com>,
        <linux-kernel@vger.kernel.org>, <bhelgaas@google.com>
Subject: RE: [PATCH v5 21/26] cxl/pci: Update CXL error logging to use RAS
 register address
Message-ID: <64878a219f3d6_142af8294c1@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230607221651.2454764-1-terry.bowman@amd.com>
 <20230607221651.2454764-22-terry.bowman@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230607221651.2454764-22-terry.bowman@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?sfJ0NweIvXBEY/T8LLWyZQdYbli7NkBwz4l+Toi2PM5ZYscvtiMmFrysz0jm?=
 =?us-ascii?Q?CtolJ1evyW+BAsSCJ0Pf6Dt1RVVwf1qsZ15v4UEz3Wgow/t7I0iXq8Ckf+DW?=
 =?us-ascii?Q?2pBCUnL7qa1i0aeIC2mBWzp5mGSwS4WYoiv6LZwbH7j3I1A50DcD9ulIOfIY?=
 =?us-ascii?Q?4yilIlJgYh5FQWRtwvaeP02fXvbKtxmbHNIwwMuM7SHhR0HVCH30JIZNdpVr?=
 =?us-ascii?Q?uxZQmhBHvtkT3W1Gz2kDoZxZRY1kvQGF3q418Z1hnCncHWHSpjxXUc7sFSYd?=
 =?us-ascii?Q?4g+J9lwzm8ZzsSiWEvg3hHDBSVblQoLLM982nFqYs2clEKYoOozYoW3Co2bJ?=
 =?us-ascii?Q?058AjVBEC2Ch/vp+U/Q9d9RfGZuXxUVqafAO0N8QwpL4gCs1vhs8fZwR6tqK?=
 =?us-ascii?Q?EjK5dFLubWtEa1cUEtM67W9JqUaUIgGgQdnwKoXVGCYhQGLXJv7EmmaUamxi?=
 =?us-ascii?Q?EqufCGxZj/Eyo2wI2iqqcrf76S2BJfYZuldg0KK7pGkeL6+6cMFNVrMgp5fI?=
 =?us-ascii?Q?m2sOr7vGB9TIZj1EAHxw4n3p15RJOpdZRbcKvlf1pqQ13ksSN2Pwwp8FnVzh?=
 =?us-ascii?Q?ymef6aH0CYpPCxYZUH2eyviFPqGYGlLXE5Vc3phMHf3KR3XBFKc8qc17jEUN?=
 =?us-ascii?Q?F2sVEPYCYtbnZbBXg0iDTZzbsddnRs4IRr8dbNX7KAOkurwThXYjC4caXx+d?=
 =?us-ascii?Q?EL0jrM1r4KDtvm/uuusn/NoP3SJ0DvyRD9cYry9M3DvhWaNJZRxpATbHU591?=
 =?us-ascii?Q?V7wK5cTRXNLkZrtfGc+ebx1Nebz4brf97+UVCGQdmO16PGy+1CbC8JXAsBhi?=
 =?us-ascii?Q?m/6s0SuEtvc3GkIGLoKiCdl2xNE7LiEhA25Banmxvo7KV1PuBRe9hVq/tPFm?=
 =?us-ascii?Q?JA22GpOKrU+IEJsFp6W8nUd9cVsXXpCuGUmFdaeOoo/8TYHCMX0hpWxEzZaP?=
 =?us-ascii?Q?WMRNM//ZwxocYOoNRVz6hKncCXxbUqpKhaEdJKn6lJIlcWX3mum5AuRXim7N?=
 =?us-ascii?Q?3Pw9CUiPqiFvpbUVEJo3IX3MQVQnT0z6YIST8KNWKH3Jumg3vm4hp7Ab2d2o?=
 =?us-ascii?Q?GxQFkUTjN8IkhMRAwOGtzEWp8Gv8vY0bbIlFyjEtu6XRRy0Up+y3SUs6qM5+?=
 =?us-ascii?Q?ASgKoYGXrC/zp8gjlVA2MIW90cBXD0/fTX61TWr4XNIlHO9iYhxY4svK+EgJ?=
 =?us-ascii?Q?Bh2/onRuXm7Eu5VCrMXfhWu9jJP3cf+1Ho81KjNHtFQHYoG2DtONdX3sb2Q/?=
 =?us-ascii?Q?eBaAyn5phshr6vmOxGOWrjua+R+HaGMd2tjOSDLRQUoJ/Abry5jTSZnbUsmV?=
 =?us-ascii?Q?4f0MJRacztrDJlwKjiAshy+Y/QkUKBZ1oInV2dUK3KSW22YS1IQeINOoIFmx?=
 =?us-ascii?Q?IwtERQAx4/W2WiHqUOINuI4cl/Khj5TX8JsEHfsvE6G51OP5NQMxCnJ2ZWir?=
 =?us-ascii?Q?feMhqEZyBATSPodZahnzXDRm39vPYSFiYrOmIeWxDCzhYlaOOWwhjaxGisvj?=
 =?us-ascii?Q?2alpKy5QXJiUI2fCJEKffuocEDAhBro1jWSODYfjHqNu1PLtzmiWVP9MHUuS?=
 =?us-ascii?Q?ZZc11wsYF2o6nx+6I4NkWzSxkOMH2gAXNNbzyzMcE6Pr46TJIb6urAT+m5zC?=
 =?us-ascii?Q?VQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d405bb0-5673-4d3d-6b02-08db6b89abc3
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jun 2023 21:12:04.2611
 (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: wAydKopyogQEMVtsJSfNmcMTW7aeKI3Bz1kli/b7siDPqZi8cwkRsEO22xHQjuUNa7sxbRzlB9Irfxx2XR1hcj5uKWxGpS9c5xlC75xbuw4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5327
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Terry Bowman wrote:
> The CXL error handler currently only logs endpoint RAS status. The CXL
> topology includes several components providing RAS details to be logged
> during error handling.[1] Update the current handler's RAS logging to use a
> RAS register address. This will allow for adding support to log other CXL
> component's RAS details in the future.
> 
> [1] CXL3.0 Table 8-22 CXL_Capability_ID Assignment
> 
> Co-developed-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/pci.c | 42 ++++++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 67f4ab6daa34..def6ee5ab4f5 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -665,32 +665,36 @@ void read_cdat_data(struct cxl_port *port)
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
>  
> -void cxl_cor_error_detected(struct pci_dev *pdev)
> +static void __cxl_log_correctable_ras(struct cxl_dev_state *cxlds,
> +				      void __iomem *ras_base)
>  {
> -	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>  	void __iomem *addr;
>  	u32 status;
>  
> -	if (!cxlds->regs.ras)
> +	if (!ras_base)
>  		return;
>  
> -	addr = cxlds->regs.ras + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
> +	addr = ras_base + CXL_RAS_CORRECTABLE_STATUS_OFFSET;
>  	status = readl(addr);
>  	if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
>  		writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
>  		trace_cxl_aer_correctable_error(cxlds->cxlmd, status);
>  	}
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, CXL);
> +
> +static void cxl_log_correctable_ras_endpoint(struct cxl_dev_state *cxlds)
> +{
> +	return __cxl_log_correctable_ras(cxlds, cxlds->regs.ras);
> +}

As I review patch 24 it leads me back here to grumble about some naming
choices. We now have:

cxl_cor_error_detected()
__cxl_log_correctable_ras()
cxl_report_and_clear()

Which of those clear the status? What is the difference between "report"
and a "log"? I don't much care what name gets chosen but these three
functions at least need to give the impression they were written with a
common vision. The term "handle" would not surprise me in the names of
functions that emit messages and clear register status bits.