Re: [PATCH v2 1/3] cxl/port: Use __free() to drop put_device() for cxl_port

["Li, Ming4" <ming4.li@intel.com>]

On 8/27/2024 7:48 PM, Jonathan Cameron wrote:
> On Mon, 26 Aug 2024 08:30:56 +0000
> Li Ming <ming4.li@intel.com> wrote:
>
>> Using scope-based resource management __free() marco with a new helper
>> called put_cxl_port() to drop open coded the put_device() used to
>> dereference the 'struct device' in cxl_port.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
> I'm a bit doubtful about this in general because of the increase
> in scope and reordering of the releases, but there
> is one case below that I particularly dislike.
>
> This is fiddly code so you've done a good job btw.
>
> Jonathan
>
>> ---
>> v2:
>> - Use guard() instead of scoped_guard() in some cases.
>> - Ira: Check the return value of find_cxl_port_at().
>> Link to v1: https://lore.kernel.org/linux-cxl/8ac82c61-7871-4914-b376-32431868622c@intel.com/T/#m07695675435bf702311dfc40f64289b9623afa16
>> ---
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 1d5007e3795a..b50dda6610e3 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1539,8 +1537,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			      struct device *uport_dev,
>>  			      struct device *dport_dev)
>>  {
>> +	struct cxl_port *port __free(put_cxl_port) = NULL;
> I don't much like the ordering here.  This will get freed
> later than it probably should.
>
> Can you move it down to just before the device_lock() is taken?
> That way at least it will get released in the same order
> wrt to the parent_port->dev + keep it's constructor as near
> as possible.

Oh, sure, thank you for pointing out, will do it in next version.


>
>
>>  	struct device *dparent = grandparent(dport_dev);
>> -	struct cxl_port *port, *parent_port = NULL;
>>  	struct cxl_dport *dport, *parent_dport;
>>  	resource_size_t component_reg_phys;
>>  	int rc;
>> @@ -1556,7 +1554,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  		return -ENXIO;
>>  	}
>>  
>> -	parent_port = find_cxl_port(dparent, &parent_dport);
>> +	struct cxl_port *parent_port __free(put_cxl_port) =
>> +		find_cxl_port(dparent, &parent_dport);
>>  	if (!parent_port) {
>>  		/* iterate to create this parent_port */
>>  		return -EAGAIN;
>> @@ -1596,10 +1595,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>>  			 */
>>  			rc = -ENXIO;
>>  		}
>> -		put_device(&port->dev);
>>  	}
>>  
>> -	put_device(&parent_port->dev);
>>  	return rc;
>>  }