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

[Bjorn Helgaas <helgaas@kernel.org>]

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?

> +/* 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"?

> +#define NUM_CXL_TIMEOUT_RANGES 9

I don't think we actually need this constant, do we?

> +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.

> +	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"?

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

Static?

Bjorn