Re: [RFC PATCH 3/6] pcie/cxl_timeout: Add CXL.mem timeout range programming

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

On 2/15/24 3:35 PM, Bjorn Helgaas wrote:
> On Thu, Feb 15, 2024 at 01:40:45PM -0600, Ben Cheatham wrote:
>> Add programming of CXL.mem transaction timeout range (CXL 3.0 8.2.4.23.1
>> bits 4 & 11:8) through sysfs. To program the device, read the ranges
>> supported from "available_timeout_ranges" in the PCIe service device
>> directory, then write the desired value to "timeout_range". The current
>> value can be found by reading from "timeout_range".
>>
>> Example for CXL-enabled PCIe port 0000:0c:00.0, with CXL timeout
>> service as 020:
>>  # cd /sys/bus/pci_express/devices/0000:0c:00.0:pcie020
> 
> I would really, really like to avoid adding sysfs dependences on
> portdrv.  Is there any chance these files could go in the normal
> /sys/bus/pci/devices/ hierarchy instead?
> 

This was just the first place I tried putting it that seemed ok. I'll take a look
at moving them over to pci/devices instead.

>> +/* CXL 3.0 8.2.4.23.2 CXL Timeout and Isolation Control Register, bits 3:0 */
>> +#define CXL_TIMEOUT_TIMEOUT_RANGE_DEFAULT 0x0
> 
>> +#define CXL_TIMEOUT_TIMEOUT_RANGE_D2 0xE
> 
> Looks like the single other example in this file of hex constants
> using A-F uses lower-case.
> 
> Does "TIMEOUT_TIMEOUT" add information over just "TIMEOUT"?
> 

TIMEOUT_TIMEOUT is supposed to specify transaction timeout, but I'll agree it is
confusing. I could try changing to CXL_TRANSACTION_TIMEOUT_RANGE_* instead, but
it would break the convention of CXL timeout & isolation defines starting with
CXL_TIMEOUT.

>> +#define NUM_CXL_TIMEOUT_RANGES 9
> 
> I don't think we actually need this constant, do we?
> 

Not really, just gets rid of a magic number (as well as makes sure anyone who
changes that array remembers to update the array size).

>> +static bool cxl_validate_timeout_range(struct cxl_timeout *cxlt, u8 range)
> 
> "validate" is not a very name for a function returning "bool" because
> you can't tell what true/false means from the name.  "valid" would be
> fine.
> 

Will change.

>> +	pci_dbg(cxlt->dev->port,
>> +		 "Timeout & isolation timeout set to range 0x%x\n", range);
> 
> I don't know CXL, but "timeout ... timeout" reads sort of strange.  Is
> it actually a timeout for a timeout?  Maybe it was supposed to be
> "transaction timeout"?
> 

Yeah it's the same as above. I'll change it to "Timeout & isolation transaction timeout ..." instead.

>> +const struct cxl_timeout_range {
>> +	const char *str;
>> +	u8 range_val;
>> +} cxl_timeout_ranges[NUM_CXL_TIMEOUT_RANGES] = {
> 
> Static?
> 

Yep, I'll change it.

> Bjorn
>