Re: [bug report] cxl/port: Use scoped_guard()/guard() to drop device_lock() for cxl_port

[Dan Williams <dan.j.williams@intel.com>]

Dan Carpenter wrote:
> Hello Li Ming,
> 
> Commit 7f569e917b78 ("cxl/port: Use scoped_guard()/guard() to drop
> device_lock() for cxl_port") from Aug 30, 2024 (linux-next), leads to
> the following (unpublished) Smatch static checker warning:
> 
> 	drivers/cxl/core/port.c:1591 add_port_attach_ep()
> 	warn: re-assigning __cleanup__ ptr 'port'
> 
> drivers/cxl/core/port.c
>     1542 static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>     1543                               struct device *uport_dev,
>     1544                               struct device *dport_dev)
>     1545 {
>     1546         struct device *dparent = grandparent(dport_dev);
>     1547         struct cxl_dport *dport, *parent_dport;
>     1548         resource_size_t component_reg_phys;
>     1549         int rc;
>     1550 
>     1551         if (!dparent) {
>     1552                 /*
>     1553                  * The iteration reached the topology root without finding the
>     1554                  * CXL-root 'cxl_port' on a previous iteration, fail for now to
>     1555                  * be re-probed after platform driver attaches.
>     1556                  */
>     1557                 dev_dbg(&cxlmd->dev, "%s is a root dport\n",
>     1558                         dev_name(dport_dev));
>     1559                 return -ENXIO;
>     1560         }
>     1561 
>     1562         struct cxl_port *parent_port __free(put_cxl_port) =
>     1563                 find_cxl_port(dparent, &parent_dport);
>     1564         if (!parent_port) {
>     1565                 /* iterate to create this parent_port */
>     1566                 return -EAGAIN;
>     1567         }
>     1568 
>     1569         /*
>     1570          * Definition with __free() here to keep the sequence of
>     1571          * dereferencing the device of the port before the parent_port releasing.
>     1572          */
>     1573         struct cxl_port *port __free(put_cxl_port) = NULL;
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^
> We free port when we exit the function, fine.
> 
>     1574         scoped_guard(device, &parent_port->dev) {
>     1575                 if (!parent_port->dev.driver) {
>     1576                         dev_warn(&cxlmd->dev,
>     1577                                  "port %s:%s disabled, failed to enumerate CXL.mem\n",
>     1578                                  dev_name(&parent_port->dev), dev_name(uport_dev));
>     1579                         return -ENXIO;
>     1580                 }
>     1581 
>     1582                 port = find_cxl_port_at(parent_port, dport_dev, &dport);
>     1583                 if (!port) {
>     1584                         component_reg_phys = find_component_registers(uport_dev);
>     1585                         port = devm_cxl_add_port(&parent_port->dev, uport_dev,
>     1586                                                  component_reg_phys, parent_dport);
> 
> This port from devm_cxl_add_port() needs to be undone.
> 
>     1587                         if (IS_ERR(port))
>     1588                                 return PTR_ERR(port);
>     1589 
>     1590                         /* retry find to pick up the new dport information */
> --> 1591                         port = find_cxl_port_at(parent_port, dport_dev, &dport);
> 
> But we re-assign port here so it will only free this port and not the one from
> devm_cxl_add_port().

Yup. I must say I really do not like what scoped_guard() does to code
flow. If conversion to cleanup helpers causes this much code indentation
then the value is low, especially when it hides tricky bugs.

Lets revert the scoped_guard() conversions for now.

I would much prefer factoring out code sections that need a lock to
their own helper that uses guard() rather than convert inline code to
scoped_guard().