Re: [RFC PATCH 4/6] pcie/cxl_timeout: Add CXL.mem error isolation support

[Ben Cheatham <benjamin.cheatham@amd.com>]

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.