Re: [PATCH v5 4/7] cxl: Add inject-error command

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 12/16/2025 10:36 PM, Alison Schofield wrote:
> On Mon, Dec 15, 2025 at 03:36:27PM -0600, Ben Cheatham wrote:
>> Add the 'cxl-inject-error' command. This command will provide CXL
>> protocol error injection for CXL VH root ports and CXL RCH downstream
>> ports, as well as poison injection for CXL memory devices.
>>
>> Add util_cxl_dport_filter() to find downstream ports by either dport id
>> or device name.
> 
> Does above comment match code? Does util_cxl_dport_filter() match by
> 'id' or 'name' ?

It should just be 'name'; I forgot to update the commit message from v4.

> 
> 
> snip
> 
>> +#define EINJ_TYPES_BUF_SIZE 512
> 
> above appears unused

It is, I replaced it with SYSFS_ATTR_SIZE IIRC. I'll remove it.

> 
> 
> snip
> 
>> +static int poison_action(struct cxl_ctx *ctx, const char *filter,
>> +			 const char *addr_str)
>> +{
>> +	struct cxl_memdev *memdev;
>> +	size_t addr;
>> +	int rc;
>> +
>> +	memdev = find_cxl_memdev(ctx, filter);
>> +	if (!memdev)
>> +		return -ENODEV;
>> +
>> +	if (!cxl_memdev_has_poison_injection(memdev)) {
>> +		log_err(&iel, "%s does not support error injection\n",
>> +			cxl_memdev_get_devname(memdev));
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!addr_str) {
>> +		log_err(&iel, "no address provided\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	addr = strtoull(addr_str, NULL, 0);
>> +	if (addr == ULLONG_MAX && errno == ERANGE) {
>> +		log_err(&iel, "invalid address %s", addr_str);
>> +		return -EINVAL;
>> +	}
> 
> errno best set to 0 before strtoull
> there is a type mismatch btw addr of size_t and strtoull

Got it, I'll fix those.

> 
> snip
> 
>> +static int inject_action(int argc, const char **argv, struct cxl_ctx *ctx,
>> +			 const struct option *options, const char *usage)
>> +{
>> +	struct cxl_protocol_error *perr;
>> +	const char * const u[] = {
>> +		usage,
>> +		NULL
>> +	};
>> +	int rc = -EINVAL;
>> +
>> +	log_init(&iel, "cxl inject-error", "CXL_INJECT_LOG");
>> +	argc = parse_options(argc, argv, options, u, 0);
>> +
>> +	if (debug) {
>> +		cxl_set_log_priority(ctx, LOG_DEBUG);
>> +		iel.log_priority = LOG_DEBUG;
>> +	} else {
>> +		iel.log_priority = LOG_INFO;
>> +	}
>> +
>> +	if (argc != 1) {
>> +		usage_with_options(u, options);
>> +		return rc;
>> +	}
> 
> The above catches bad syntax like this where I omit type:
> # cxl inject-error mem10 -t -a 0x0
> 
> We also need to catch this where I omit the option altogether:
> # cxl inject-error mem10  -a 0x0
> Segmentation fault (core dumped)

Good point. I'll change it to catch omitted required options as well.

> 
>> +
>> +	if (strcmp(inj_param.type, "poison") == 0) {
>> +		rc = poison_action(ctx, argv[0], inj_param.address);
>> +		return rc;
>> +	}
> 
> snip
>