netdev - Re: [PATCH v12 04/23] cxl: move register/capability check to driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <39f58b48-91f6-4ac6-81a1-83ab0b689d95@amd.com>
Date: Mon, 7 Apr 2025 14:40:01 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v12 04/23] cxl: move register/capability check to driver


On 4/4/25 16:47, Jonathan Cameron wrote:
>
>>   {
>>   	int cap, cap_count;
>>   	u32 cap_array;
>> @@ -85,6 +88,8 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>>   			decoder_cnt = cxl_hdm_decoder_count(hdr);
>>   			length = 0x20 * decoder_cnt + 0x10;
>>   			rmap = &map->hdm_decoder;
>> +			if (caps)
>> +				set_bit(CXL_DEV_CAP_HDM, caps);
> Maybe it's worth a local helper?
> 			cxl_set_cap_bit() that checks for NULL cap
>
> #define cxl_set_cap_bit(bit, caps) if (caps) { set_bit((bit), (caps)); }
>
> Or just always provide caps.  Do we have use cases where we really don't
> care about what is found?


The helper seems cleaner than current code.


There are just two cases, Type3 pci driver and sfc, with both using the 
caps for visibility of potential problems with the capabilities expected.


I guess even always providing caps would imply some sanity check to be 
added, so I think the helper is better.


>> +		dev_err(&pdev->dev,
>> +			"Found capabilities (%pb) not containing mandatory expected: (%pb)\n",
> The only obvious reason I can see for an expected bitmap is this print and this isn't
> that helpful as requires anyone seeing it to dig into what the bitmap means.  Maybe
>
> 	if (!test_bit(CXL_DEV_CAP_HDM, found))
> 		return dev_err_probe(&pdev->dev, -ENXIO "HDM decoder capability not found\n");
> 	etc.
>
> That will only print the first once not found though and avoiding that adds complexity we
> probably don't want here.


Uhmm. I think you are probably right. And I think I could somehow merge 
the expected capabilities bitmap initialization with the potential error 
reported.

I'll try to do so.


>
>> +			found, expected);
>> +		return -ENXIO;
>> +	}
>> +
>>   	rc = cxl_pci_type3_init_mailbox(cxlds);
>>   	if (rc)
>>   		return rc;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 1383fd724cf6..b9cd98950a38 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -25,6 +25,26 @@ enum cxl_devtype {
>>   
>>   struct device;
>>   
>> +
>> +/* Capabilities as defined for:
> CXL code so
> /*
>   * Capabilities...


Yes.


>
>> + *
>> + *	Component Registers (Table 8-22 CXL 3.2 specification)
>> + *	Device Registers (8.2.8.2.1 CXL 3.2 specification)
>> + *
>> + * and currently being used for kernel CXL support.
>> + */
>> +
>> +enum cxl_dev_cap {
>> +	/* capabilities from Component Registers */
>> +	CXL_DEV_CAP_RAS,
>> +	CXL_DEV_CAP_HDM,
>> +	/* capabilities from Device Registers */
>> +	CXL_DEV_CAP_DEV_STATUS,
>> +	CXL_DEV_CAP_MAILBOX_PRIMARY,
>> +	CXL_DEV_CAP_MEMDEV,
>> +	CXL_MAX_CAPS,
> No comma as this is at terminating entry. Seems unlikely to make
> sense to ever have anything after it so let us make that harder /
> more obvious in future patches, but not having the comma.
>

Right. I'll fix it.


Thanks!