| Message ID | 172921382410.2133624.570103049126168259.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl/port: Cleanup add_port_attach_ep() "cleanup" confusion | expand |
Dan Williams wrote: > Dan points out via smatch [1] that the conversion of > add_port_attach_ep() to use cleanup helpers does appears to not > correctly handle a pointer reassignment case. > > drivers/cxl/core/port.c:1591 add_port_attach_ep() > warn: re-assigning __cleanup__ ptr 'port' > > The reassignment still results in the correct cleanup and reference > counting, but it is not immediately obvious from the code. > > Revert the __free() conversion in preparation for refactoring the > subtlety out of this function. > > Cc: Dan Carpenter <dan.carpenter@linaro.org> > Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1] > Cc: Li Ming <ming4.li@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 54dd2cd450ca..8e6596e147b3 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1544,6 +1544,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, struct device *dport_dev) { 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; @@ -1559,18 +1560,12 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, return -ENXIO; } - struct cxl_port *parent_port __free(put_cxl_port) = - find_cxl_port(dparent, &parent_dport); + parent_port = find_cxl_port(dparent, &parent_dport); if (!parent_port) { /* iterate to create this parent_port */ return -EAGAIN; } - /* - * Definition with __free() here to keep the sequence of - * dereferencing the device of the port before the parent_port releasing. - */ - struct cxl_port *port __free(put_cxl_port) = NULL; device_lock(&parent_port->dev); if (!parent_port->dev.driver) { dev_warn(&cxlmd->dev, @@ -1605,8 +1600,10 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, */ rc = -ENXIO; } + put_device(&port->dev); } + put_device(&parent_port->dev); return rc; }
Dan points out via smatch [1] that the conversion of add_port_attach_ep() to use cleanup helpers does appears to not correctly handle a pointer reassignment case. drivers/cxl/core/port.c:1591 add_port_attach_ep() warn: re-assigning __cleanup__ ptr 'port' The reassignment still results in the correct cleanup and reference counting, but it is not immediately obvious from the code. Revert the __free() conversion in preparation for refactoring the subtlety out of this function. Cc: Dan Carpenter <dan.carpenter@linaro.org> Link: http://lore.kernel.org/2a19289b-0bcf-42c4-82a9-268a922535f2@stanley.mountain [1] Cc: Li Ming <ming4.li@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/port.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)