On 2/15/24 3:49 PM, Bjorn Helgaas wrote:
> On Thu, Feb 15, 2024 at 01:40:46PM -0600, Ben Cheatham wrote:
>> Add and enable CXL.mem error isolation support (CXL 3.0 12.3.2)
>> to the CXL Timeout & Isolation service driver.
>
>> @@ -341,7 +366,8 @@ static int cxl_timeout_probe(struct pcie_device *dev)
>> struct pci_dev *port = dev->port;
>> struct pcie_cxlt_data *pdata;
>> struct cxl_timeout *cxlt;
>> - int rc = 0;
>> + bool timeout_enabled;
>> + int rc;
>>
>> /* Limit to CXL root ports */
>> if (!pci_find_dvsec_capability(port, PCI_DVSEC_VENDOR_ID_CXL,
>> @@ -360,6 +386,18 @@ static int cxl_timeout_probe(struct pcie_device *dev)
>> pci_dbg(dev->port, "Failed to enable CXL.mem timeout: %d\n",
>> rc);
>>
>> + timeout_enabled = !rc;
>
> This ends up being a little weird: mixing int and bool, negating rc
> here and then negating timeout_enabled later, several messages.
>
> Maybe could just keep rc1 and rc2, drop the pci_dbg messages and
> enhance the "enabled" message to be something like:
> "enabled %s%s with IRQ", rc1 ? "" : "timeout", rc2 ? "" : "isolation"
> ("&" left for your imagination).
>
> Or something like
> #define FLAG(x) ((x) ? '-' : '+')
> "CXL.mem timeout%c isolation%c enabled with IRQ %d", FLAG(rc1), FLAG(rc2)
>
I thought it was janky as well. I tried something really quick using ternaries
and it looked weirder :/. But I agree with you here, I'll take another stab at it.
>> + rc = cxl_enable_isolation(dev, cxlt);
>
>> + if (rc)
>> + pci_dbg(dev->port, "Failed to enable CXL.mem isolation: %d\n",
>> + rc);
>
> "(%pe)"
>
>> + if (rc && !timeout_enabled) {
>> + pci_info(dev->port,
>> + "Failed to enable CXL.mem timeout and isolation.\n");
>
> Most messages don't include a period at end. It just adds the chance
> for line wrapping unnecessarily.
>
I'll remove them.
