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

Li, Ming4
2024-08-28T01:28:08Z

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;
>> }