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

Jonathan Cameron
2024-08-27T11:48:05Z

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.

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