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
>
