From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BF75D3BF2E9; Thu, 15 Jan 2026 15:40:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768491664; cv=none; b=rUJMbe73pG5+3esbFvvjEgmlwdf7yTzj8dGxgMrqjioevKdiSl5tsbBgjF2uvOx5kYC8nJ42z7K6ObYHlNYUwjm8uN96bSx0hBJnJjmanqeYNOteU93zLu2psKOLOzHsZPKZigWJTNzEZ8gVmzErmxv185aL4mldLBpZRxB3vR4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768491664; c=relaxed/simple; bh=AILqHjR8blSEVx9HB049ys6+Lg7yY58xWteu4c9+Nwk=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=V8FWTwoNVplVYUyWZqMc2P6ir8AKEayV3tW++JLVTMZRTlqeL96N1F8TJm8hTciM3XJnoNkyUt17kQskvBmo7fDmmpFCy5zMY5mAlcw63NE6MWtIqA7JVIAdErAZozxdlN3BWWuOJaYK+cZn8E1SVhNmTBu2/bEuARaWRFnEyAQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.224.150]) by frasgout.his.huawei.com (SkyGuard) with ESMTPS id 4dsRyP4WkvzJ46BF; Thu, 15 Jan 2026 23:40:29 +0800 (CST) Received: from dubpeml100005.china.huawei.com (unknown [7.214.146.113]) by mail.maildlp.com (Postfix) with ESMTPS id 1D81640563; Thu, 15 Jan 2026 23:40:46 +0800 (CST) Received: from localhost (10.203.177.15) by dubpeml100005.china.huawei.com (7.214.146.113) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.36; Thu, 15 Jan 2026 15:40:45 +0000 Date: Thu, 15 Jan 2026 15:40:43 +0000 From: Jonathan Cameron To: Terry Bowman CC: , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v14 28/34] PCI/AER: Move AER driver's CXL VH handling to pcie/aer_cxl_vh.c Message-ID: <20260115154043.0000428a@huawei.com> In-Reply-To: <20260114182055.46029-29-terry.bowman@amd.com> References: <20260114182055.46029-1-terry.bowman@amd.com> <20260114182055.46029-29-terry.bowman@amd.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml500010.china.huawei.com (7.191.174.240) To dubpeml100005.china.huawei.com (7.214.146.113) On Wed, 14 Jan 2026 12:20:49 -0600 Terry Bowman wrote: > CXL virtual hierarchy (VH) RAS handling for CXL Port devices will be added > soon. This requires a notification mechanism for the AER driver to share > the AER interrupt with the CXL driver. The notification will be used as an > indication for the CXL drivers to handle and log the CXL RAS errors. > > Note, 'CXL protocol error' terminology will refer to CXL VH and not > CXL RCH errors unless specifically noted going forward. > > Introduce a new file in the AER driver to handle the CXL protocol errors > named pci/pcie/aer_cxl_vh.c. > > Add a kfifo work queue to be used by the AER and CXL drivers. The AER > driver will be the sole kfifo producer adding work and the cxl_core will be > the sole kfifo consumer removing work. Add the boilerplate kfifo support. > Encapsulate the kfifo, RW semaphore, and work pointer in a single structure. > > Add CXL work queue handler registration functions in the AER driver. Export > the functions allowing CXL driver to access. Implement registration > functions for the CXL driver to assign or clear the work handler function. > Synchronize accesses using the RW semaphore. > > Introduce 'struct cxl_proto_err_work_data' to serve as the kfifo work data. > This will contain a reference to the PCI error source device and the error > severity. This will be used when the work is dequeued by the cxl_core driver. > > Signed-off-by: Terry Bowman > Reviewed-by: Jonathan Cameron I'd not trust me :). Some more comments inline. > Reviewed-by: Dave Jiang > > diff --git a/drivers/pci/pcie/aer_cxl_vh.c b/drivers/pci/pcie/aer_cxl_vh.c > new file mode 100644 > index 000000000000..2189d3c6cef1 > --- /dev/null > +++ b/drivers/pci/pcie/aer_cxl_vh.c > @@ -0,0 +1,78 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright(c) 2025 AMD Corporation. All rights reserved. */ > + > +#include I'd pick an ordering for headers. This doesn't seem to follow any common style. > +#include > +#include > +#include > +#include "../pci.h" > +#include "portdrv.h" > + > +#define CXL_ERROR_SOURCES_MAX 128 > + > +struct cxl_proto_err_kfifo { > + struct work_struct *work; > + struct rw_semaphore rw_sema; > + DECLARE_KFIFO(fifo, struct cxl_proto_err_work_data, > + CXL_ERROR_SOURCES_MAX); > +}; I've not checked later patches yet, but given the type is never really used, can we make this an anonymous structure? It's just there for grouping a bunch of related things so not surprising the structure type isn't much used. > + > +static struct cxl_proto_err_kfifo cxl_proto_err_kfifo = { > + .rw_sema = __RWSEM_INITIALIZER(cxl_proto_err_kfifo.rw_sema) > +}; > + > +bool is_aer_internal_error(struct aer_err_info *info) > +{ > + if (info->severity == AER_CORRECTABLE) > + return info->status & PCI_ERR_COR_INTERNAL; > + > + return info->status & PCI_ERR_UNC_INTN; > +} As per earlier feedback, this is generic AER stuff. I'd leave it in the original file and remove the ifdef magic around it (thus avoiding the stub etc) > + > +bool is_cxl_error(struct pci_dev *pdev, struct aer_err_info *info) > +{ > + if (!info || !info->is_cxl) > + return false; > + > + if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT) To me this doesn't fit with the function name. Lots of things are cxl errors from non endpoint sources. So I'd choose a more specific name. > + return false; > + > + return is_aer_internal_error(info); > +}