Re: [PATCH v2 5/7] cxl/core: Use guard() to drop goto pattern of cxl_dpa_alloc()

[Li Ming]

On 2/20/2025 9:01 AM, Dan Williams wrote:
> Li Ming wrote:
>> In cxl_dpa_alloc(), some checking and operations need to be protected by
>> a rwsem called cxl_dpa_rwsem, so there is a goto pattern in
>> cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized
>> by using guard() to hold the rwsem.
>>
>> Creating a new function called __cxl_dpa_alloc() to include all checking
>> and operations needed to be procted by cxl_dpa_rwsem. Using
>> guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new
>> function.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Signed-off-by: Li Ming <ming.li@xxxxxxxxxxxx>
>> ---
>>  drivers/cxl/core/hdm.c | 29 ++++++++++++++---------------
>>  1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 874a791f8292..1edbf7873471 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
> [..]
>> @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>>  		skip = skip_end - skip_start + 1;
>>  	} else {
>>  		dev_dbg(dev, "mode not set\n");
>> -		rc = -EINVAL;
>> -		goto out;
>> +		return -EINVAL;
>>  	}
>>  
>>  	if (size > avail) {
>>  		dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size,
>>  			cxl_decoder_mode_name(cxled->mode), &avail);
>> -		rc = -ENOSPC;
>> -		goto out;
>> +		return -ENOSPC;
>>  	}
>>  
>> -	rc = __cxl_dpa_reserve(cxled, start, size, skip);
>> -out:
>> -	up_write(&cxl_dpa_rwsem);
>> +	return __cxl_dpa_reserve(cxled, start, size, skip);
>> +}
>> +
>> +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>> +{
>> +	struct cxl_port *port = cxled_to_port(cxled);
> Am I missing something? This @port variable is unused?
@port will be used after below __cxl_dpa_alloc().
>
>> +	int rc;
>>  
>> +	rc = __cxl_dpa_alloc(cxled, size);
>>  	if (rc)
>>  		return rc;
>>  
> So I think you can drop this new cxl_dpa_alloc + __cxl_dpa_alloc scheme?

After __cxl_dpa_alloc(), a 'devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled)' will be invoked, cxl_dpa_rwsem is possible to be held in cxl_dpa_release() in devm_add_action_or_reset() failure case.

So I create __cxl_dpa_alloc() to hold cxl_dpa_rwsem for the operations needed cxl_dpa_resem protection, make sure that the cxl_dpa_rwsem is released before devm_add_action_or_reset() invoking.

>
> After that you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>