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.

I also think the bug originates in:

dd2617ebd2a6 cxl/port: Use __free() to drop put_device() for cxl_port

...where the wrong port is cleaned up, but I want to revert the
scoped_guard() conversion first to make that cleanup easier.

In general for CXL I want to say that no function should be converted to
use cleanup helpers unless all gotos are removed at once, and if the
conversion needs to reach for scoped_guard() reconsider even attempting
the conversion. I.e. scoped_guard() is a leading indicator for needing
code refactoring.