On 2024/11/15 2:41, Bowman, Terry wrote:
Hi Lukas,
I added comments below.
On 11/14/2024 10:44 AM, Lukas Wunner wrote:
On Wed, Nov 13, 2024 at 03:54:19PM -0600, Terry Bowman wrote:
@@ -1115,8 +1131,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
{
- cxl_handle_error(dev, info);
- pci_aer_handle_error(dev, info);
+ if (is_internal_error(info) && handles_cxl_errors(dev))
+ cxl_handle_error(dev, info);
+ else
+ pci_aer_handle_error(dev, info);
+
pci_dev_put(dev);
}
If you just do this at the top of cxl_handle_error()...
if (!is_internal_error(info))
return;
...you avoid the need to move is_internal_error() around and the
patch becomes simpler and easier to review.
If is_internal_error()==0, then pci_aer_handle_error() should be called to process the PCIe error. Your suggestion would require returning a value from cxl_handle_error(). And then more "if" logic would be required for the cxl_handle_error() return value. Should both is_internal_error() and handles_cxl_errors()be moved into cxl_handle_error()? Would give this:
static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
{
- cxl_handle_error(dev, info);
- pci_aer_handle_error(dev, info);
+ if (!cxl_handle_error(dev, info))
+ pci_aer_handle_error(dev, info);
+
pci_dev_put(dev);
}
I think is_internal_error() can be moved into handles_cxl_errors().
handles_cxl_errors() helps to check if the error is a CXL error, avoid
this detail which CXL error is an internal error in
handle_error_source(). Like this:
static void handle_error_source(struct pci_dev *dev, struct
aer_err_info *info)
{
- cxl_handle_error(dev, info);
- pci_aer_handle_error(dev, info);
+ if (handles_cxl_errors(dev, info))
+ cxl_handle_error(dev, info);
+ else
+ pci_aer_handle_error(dev, info);
+
pci_dev_put(dev);
}
Ming
The AER service driver supports handling downstream port protocol errors in
restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
functionality for CXL PCIe ports operating in virtual hierarchy (VH)
mode.[1]
This is somewhat minor but by convention, patches in the PCI subsystem
adhere to spec language and capitalization, e.g. "Downstream Port"
instead of "downstream port". Makes it easier to connect the commit
message or code comments to the spec. So maybe you want to consider
that if/when respinning.
Thanks,
Lukas
Thanks for pointing that out. I'll update as you suggest.
Regards,
Terry
