linux-kernel - Re: [PATCH v2 1/1] cxl/port: Avoid missing port component registers setup

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 for Android: free password hash cracker in your pocket

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

Message-ID: <34d5a43d-f3a7-4163-9ebc-afa2a9b666fb@zohomail.com>
Date: Tue, 30 Sep 2025 08:46:29 +0800
From: Li Ming <ming.li@...omail.com>
To: Dave Jiang <dave.jiang@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
 dave@...olabs.net, jonathan.cameron@...wei.com, alison.schofield@...el.com,
 vishal.l.verma@...el.com, ira.weiny@...el.com, dan.j.williams@...el.com,
 shiju.jose@...wei.com
Subject: Re: [PATCH v2 1/1] cxl/port: Avoid missing port component registers
 setup

On 9/30/2025 12:53 AM, Dave Jiang wrote:
>
> On 9/28/25 3:14 AM, Li Ming wrote:
>> port->nr_dports is used to represent how many dports added to the cxl
>> port, it will increase in add_dport() when a new dport is being added to
>> the cxl port, but it will not be reduced when a dport is removed from
>> the cxl port.
>>
>> Currently, when the first dport is added to a cxl port, it will trigger
>> component registers setup on the cxl port, the implementation is using
>> port->nr_dports to confirm if the dport is the first dport.
>>
>> A corner case here is that adding dport could fail after port->nr_dports
>> updating and before checking port->nr_dports for component registers
>> setup. If the failure happens during the first dport attaching, it will
>> cause that CXL subsystem has not chance to execute component registers
>> setup for the cxl port. the failure flow like below:
>>
>> port->nr_dports = 0
>> dport 1 adding to the port:
>> 	add_dport()	# port->nr_dports: 1
>> 	failed on devm_add_action_or_reset() or sysfs_create_link()
>> 	return error	# port->nr_dports: 1
>> dport 2 adding to the port:
>> 	add_dport()	# port->nr_dports: 2
>> 	no failure
>> 	skip component registers setup because of port->nr_dports is 2
>>
>> The solution here is that moving component registers setup closer to
>> add_dport(), so if add_dport() is executed correctly for the first
>> dport, component registers setup on the port will be executed
>> immediately after that.
>>
>> Signed-off-by: Li Ming <ming.li@...omail.com>
> That makes sense. Please add a fixes tag. The commit in cxl/next should be stable.

Thanks for the review, will do that.

Ming

>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
>
>> ---
>> v2:
>> - remove dport from port->dports in case of component registers setup
>>   failed.
>>
>> base-commit: 46037455cbb748c5e85071c95f2244e81986eb58 cxl/next
>> ---
>>  drivers/cxl/core/port.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index d5f71eb1ade8..8128fd2b5b31 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1182,6 +1182,20 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	if (rc)
>>  		return ERR_PTR(rc);
>>  
>> +	/*
>> +	 * Setup port register if this is the first dport showed up. Having
>> +	 * a dport also means that there is at least 1 active link.
>> +	 */
>> +	if (port->nr_dports == 1 &&
>> +	    port->component_reg_phys != CXL_RESOURCE_NONE) {
>> +		rc = cxl_port_setup_regs(port, port->component_reg_phys);
>> +		if (rc) {
>> +			xa_erase(&port->dports, (unsigned long)dport->dport_dev);
>> +			return ERR_PTR(rc);
>> +		}
>> +		port->component_reg_phys = CXL_RESOURCE_NONE;
>> +	}
>> +
>>  	get_device(dport_dev);
>>  	rc = devm_add_action_or_reset(host, cxl_dport_remove, dport);
>>  	if (rc)
>> @@ -1200,18 +1214,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  
>>  	cxl_debugfs_create_dport_dir(dport);
>>  
>> -	/*
>> -	 * Setup port register if this is the first dport showed up. Having
>> -	 * a dport also means that there is at least 1 active link.
>> -	 */
>> -	if (port->nr_dports == 1 &&
>> -	    port->component_reg_phys != CXL_RESOURCE_NONE) {
>> -		rc = cxl_port_setup_regs(port, port->component_reg_phys);
>> -		if (rc)
>> -			return ERR_PTR(rc);
>> -		port->component_reg_phys = CXL_RESOURCE_NONE;
>> -	}
>> -
>>  	return dport;
>>  }
>>