| Message ID | 20240813070552.3353530-2-ming4.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] cxl/port: Use __free() to drop put_device() for cxl_port | expand |
Li Ming wrote: > A device_lock() and device_unlock() pair can be replaced by a > scope-based resource management function scoped_guard() which can make > the code more readable and safer. Some device_lock() and device_unlock() > pairs in CXL subsystem can be simply replaced by a scoped_guard(). > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Li Ming <ming4.li@intel.com> > --- > drivers/cxl/core/mbox.c | 26 ++++++------ > drivers/cxl/core/port.c | 87 ++++++++++++++++++--------------------- > drivers/cxl/core/region.c | 25 ++++++----- > drivers/cxl/mem.c | 22 +++++----- > drivers/cxl/pmem.c | 16 ++++--- > 5 files changed, 82 insertions(+), 94 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index e5cdeafdf76e..a8ab5f2697be 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) > int rc; > > /* synchronize with cxl_mem_probe() and decoder write operations */ > - device_lock(&cxlmd->dev); > - endpoint = cxlmd->endpoint; > - down_read(&cxl_region_rwsem); > - /* > - * Require an endpoint to be safe otherwise the driver can not > - * be sure that the device is unmapped. > - */ > - if (endpoint && cxl_num_decoders_committed(endpoint) == 0) > - rc = __cxl_mem_sanitize(mds, cmd); > - else > - rc = -EBUSY; > - up_read(&cxl_region_rwsem); > - device_unlock(&cxlmd->dev); > + scoped_guard(device, &cxlmd->dev) { > + endpoint = cxlmd->endpoint; > + down_read(&cxl_region_rwsem); > + /* > + * Require an endpoint to be safe otherwise the driver can not > + * be sure that the device is unmapped. > + */ > + if (endpoint && cxl_num_decoders_committed(endpoint) == 0) > + rc = __cxl_mem_sanitize(mds, cmd); > + else > + rc = -EBUSY; > + up_read(&cxl_region_rwsem); > + } > > return rc; > } > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 6119cb3ad25c..53e2593daa95 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new) > struct cxl_port *port = new->dport->port; > int rc; > > - device_lock(&port->dev); > - if (port->dead) { > - device_unlock(&port->dev); > - return -ENXIO; > + scoped_guard(device, &port->dev) { > + if (port->dead) > + return -ENXIO; > + rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, > + GFP_KERNEL); > } > - rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, > - GFP_KERNEL); > - device_unlock(&port->dev); > > return rc; > } > @@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data) > struct cxl_port *endpoint = cxlmd->endpoint; > struct device *host = endpoint_host(endpoint); > > - device_lock(host); > - if (host->driver && !endpoint->dead) { > - devm_release_action(host, cxl_unlink_parent_dport, endpoint); > - devm_release_action(host, cxl_unlink_uport, endpoint); > - devm_release_action(host, unregister_port, endpoint); > + scoped_guard(device, host) { > + if (host->driver && !endpoint->dead) { > + devm_release_action(host, cxl_unlink_parent_dport, endpoint); > + devm_release_action(host, cxl_unlink_uport, endpoint); > + devm_release_action(host, unregister_port, endpoint); > + } > + cxlmd->endpoint = NULL; > } > - cxlmd->endpoint = NULL; > - device_unlock(host); > put_device(&endpoint->dev); > put_device(host); > } > @@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, > return -EAGAIN; > } > > - device_lock(&parent_port->dev); > - if (!parent_port->dev.driver) { > - dev_warn(&cxlmd->dev, > - "port %s:%s disabled, failed to enumerate CXL.mem\n", > - dev_name(&parent_port->dev), dev_name(uport_dev)); > - port = ERR_PTR(-ENXIO); > - goto out; > - } > + scoped_guard(device, &parent_port->dev) { > + if (!parent_port->dev.driver) { > + dev_warn(&cxlmd->dev, > + "port %s:%s disabled, failed to enumerate CXL.mem\n", > + dev_name(&parent_port->dev), dev_name(uport_dev)); > + return -ENXIO; > + } > > - port = find_cxl_port_at(parent_port, dport_dev, &dport); > - if (!port) { > - component_reg_phys = find_component_registers(uport_dev); > - port = devm_cxl_add_port(&parent_port->dev, uport_dev, > - component_reg_phys, parent_dport); > - /* retry find to pick up the new dport information */ > - if (!IS_ERR(port)) > + port = find_cxl_port_at(parent_port, dport_dev, &dport); > + if (!port) { > + component_reg_phys = find_component_registers(uport_dev); > + port = devm_cxl_add_port(&parent_port->dev, uport_dev, > + component_reg_phys, parent_dport); > + if (IS_ERR(port)) > + return PTR_ERR(port); > + > + /* retry find to pick up the new dport information */ > port = find_cxl_port_at(parent_port, dport_dev, &dport); Can this call to find_cxl_port_at() fail? > + } > } > -out: > - device_unlock(&parent_port->dev); > > - if (IS_ERR(port)) ^^^^^^^^^^^^ Does this need to be checked above? It seems like the logic is changing for the call to cxl_add_ep() but it might be ok? Ira > - rc = PTR_ERR(port); > - else { > - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", > - dev_name(&port->dev), dev_name(port->uport_dev)); > - rc = cxl_add_ep(dport, &cxlmd->dev); > - if (rc == -EBUSY) { > - /* > - * "can't" happen, but this error code means > - * something to the caller, so translate it. > - */ > - rc = -ENXIO; > - } > + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", > + dev_name(&port->dev), dev_name(port->uport_dev)); > + rc = cxl_add_ep(dport, &cxlmd->dev); > + if (rc == -EBUSY) { > + /* > + * "can't" happen, but this error code means > + * something to the caller, so translate it. > + */ > + rc = -ENXIO; > } > > return rc; > @@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map) > > port = to_cxl_port(cxld->dev.parent); > > - device_lock(&port->dev); > - rc = cxl_decoder_add_locked(cxld, target_map); > - device_unlock(&port->dev); > + scoped_guard(device, &port->dev) > + rc = cxl_decoder_add_locked(cxld, target_map); > > return rc; > } > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 21ad5f242875..a3ce7cbe5c6c 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3094,11 +3094,11 @@ static void cxlr_release_nvdimm(void *_cxlr) > struct cxl_region *cxlr = _cxlr; > struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb; > > - device_lock(&cxl_nvb->dev); > - if (cxlr->cxlr_pmem) > - devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, > - cxlr->cxlr_pmem); > - device_unlock(&cxl_nvb->dev); > + scoped_guard(device, &cxl_nvb->dev) { > + if (cxlr->cxlr_pmem) > + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, > + cxlr->cxlr_pmem); > + } > cxlr->cxl_nvb = NULL; > put_device(&cxl_nvb->dev); > } > @@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) > dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent), > dev_name(dev)); > > - device_lock(&cxl_nvb->dev); > - if (cxl_nvb->dev.driver) > - rc = devm_add_action_or_reset(&cxl_nvb->dev, > - cxlr_pmem_unregister, cxlr_pmem); > - else > - rc = -ENXIO; > - device_unlock(&cxl_nvb->dev); > - > + scoped_guard(device, &cxl_nvb->dev) { > + if (cxl_nvb->dev.driver) > + rc = devm_add_action_or_reset(&cxl_nvb->dev, > + cxlr_pmem_unregister, cxlr_pmem); > + else > + rc = -ENXIO; > + } > if (rc) > goto err_bridge; > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index ab9b8ab8df44..ae94018a01bd 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev) > > cxl_setup_parent_dport(dev, dport); > > - device_lock(endpoint_parent); > - if (!endpoint_parent->driver) { > - dev_err(dev, "CXL port topology %s not enabled\n", > - dev_name(endpoint_parent)); > - rc = -ENXIO; > - goto unlock; > - } > + scoped_guard(device, endpoint_parent) { > + if (!endpoint_parent->driver) { > + dev_err(dev, "CXL port topology %s not enabled\n", > + dev_name(endpoint_parent)); > + return -ENXIO; > + } > > - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > -unlock: > - device_unlock(endpoint_parent); > - if (rc) > - return rc; > + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); > + if (rc) > + return rc; > + } > > /* > * The kernel may be operating out of CXL memory on this device, > diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c > index 4ef93da22335..647c7e25ef3a 100644 > --- a/drivers/cxl/pmem.c > +++ b/drivers/cxl/pmem.c > @@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data) > if (!is_cxl_nvdimm(dev)) > return 0; > > - device_lock(dev); > - if (!dev->driver) > - goto out; > - > - cxl_nvd = to_cxl_nvdimm(dev); > - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) > - release = true; > -out: > - device_unlock(dev); > + scoped_guard(device, dev) { > + if (dev->driver) { > + cxl_nvd = to_cxl_nvdimm(dev); > + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) > + release = true; > + } > + } > if (release) > device_release_driver(dev); > return 0; > -- > 2.40.1 >
On 8/22/2024 5:49 AM, Ira Weiny wrote: > Li Ming wrote: >> A device_lock() and device_unlock() pair can be replaced by a >> scope-based resource management function scoped_guard() which can make >> the code more readable and safer. Some device_lock() and device_unlock() >> pairs in CXL subsystem can be simply replaced by a scoped_guard(). >> >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Li Ming <ming4.li@intel.com> >> --- >> drivers/cxl/core/mbox.c | 26 ++++++------ >> drivers/cxl/core/port.c | 87 ++++++++++++++++++--------------------- >> drivers/cxl/core/region.c | 25 ++++++----- >> drivers/cxl/mem.c | 22 +++++----- >> drivers/cxl/pmem.c | 16 ++++--- >> 5 files changed, 82 insertions(+), 94 deletions(-) >> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c >> index e5cdeafdf76e..a8ab5f2697be 100644 >> --- a/drivers/cxl/core/mbox.c >> +++ b/drivers/cxl/core/mbox.c >> @@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) >> int rc; >> >> /* synchronize with cxl_mem_probe() and decoder write operations */ >> - device_lock(&cxlmd->dev); >> - endpoint = cxlmd->endpoint; >> - down_read(&cxl_region_rwsem); >> - /* >> - * Require an endpoint to be safe otherwise the driver can not >> - * be sure that the device is unmapped. >> - */ >> - if (endpoint && cxl_num_decoders_committed(endpoint) == 0) >> - rc = __cxl_mem_sanitize(mds, cmd); >> - else >> - rc = -EBUSY; >> - up_read(&cxl_region_rwsem); >> - device_unlock(&cxlmd->dev); >> + scoped_guard(device, &cxlmd->dev) { >> + endpoint = cxlmd->endpoint; >> + down_read(&cxl_region_rwsem); >> + /* >> + * Require an endpoint to be safe otherwise the driver can not >> + * be sure that the device is unmapped. >> + */ >> + if (endpoint && cxl_num_decoders_committed(endpoint) == 0) >> + rc = __cxl_mem_sanitize(mds, cmd); >> + else >> + rc = -EBUSY; >> + up_read(&cxl_region_rwsem); >> + } >> >> return rc; >> } >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >> index 6119cb3ad25c..53e2593daa95 100644 >> --- a/drivers/cxl/core/port.c >> +++ b/drivers/cxl/core/port.c >> @@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new) >> struct cxl_port *port = new->dport->port; >> int rc; >> >> - device_lock(&port->dev); >> - if (port->dead) { >> - device_unlock(&port->dev); >> - return -ENXIO; >> + scoped_guard(device, &port->dev) { >> + if (port->dead) >> + return -ENXIO; >> + rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, >> + GFP_KERNEL); >> } >> - rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, >> - GFP_KERNEL); >> - device_unlock(&port->dev); >> >> return rc; >> } >> @@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data) >> struct cxl_port *endpoint = cxlmd->endpoint; >> struct device *host = endpoint_host(endpoint); >> >> - device_lock(host); >> - if (host->driver && !endpoint->dead) { >> - devm_release_action(host, cxl_unlink_parent_dport, endpoint); >> - devm_release_action(host, cxl_unlink_uport, endpoint); >> - devm_release_action(host, unregister_port, endpoint); >> + scoped_guard(device, host) { >> + if (host->driver && !endpoint->dead) { >> + devm_release_action(host, cxl_unlink_parent_dport, endpoint); >> + devm_release_action(host, cxl_unlink_uport, endpoint); >> + devm_release_action(host, unregister_port, endpoint); >> + } >> + cxlmd->endpoint = NULL; >> } >> - cxlmd->endpoint = NULL; >> - device_unlock(host); >> put_device(&endpoint->dev); >> put_device(host); >> } >> @@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, >> return -EAGAIN; >> } >> >> - device_lock(&parent_port->dev); >> - if (!parent_port->dev.driver) { >> - dev_warn(&cxlmd->dev, >> - "port %s:%s disabled, failed to enumerate CXL.mem\n", >> - dev_name(&parent_port->dev), dev_name(uport_dev)); >> - port = ERR_PTR(-ENXIO); >> - goto out; >> - } >> + scoped_guard(device, &parent_port->dev) { >> + if (!parent_port->dev.driver) { >> + dev_warn(&cxlmd->dev, >> + "port %s:%s disabled, failed to enumerate CXL.mem\n", >> + dev_name(&parent_port->dev), dev_name(uport_dev)); >> + return -ENXIO; >> + } >> >> - port = find_cxl_port_at(parent_port, dport_dev, &dport); >> - if (!port) { >> - component_reg_phys = find_component_registers(uport_dev); >> - port = devm_cxl_add_port(&parent_port->dev, uport_dev, >> - component_reg_phys, parent_dport); >> - /* retry find to pick up the new dport information */ >> - if (!IS_ERR(port)) >> + port = find_cxl_port_at(parent_port, dport_dev, &dport); >> + if (!port) { >> + component_reg_phys = find_component_registers(uport_dev); >> + port = devm_cxl_add_port(&parent_port->dev, uport_dev, >> + component_reg_phys, parent_dport); >> + if (IS_ERR(port)) >> + return PTR_ERR(port); >> + >> + /* retry find to pick up the new dport information */ >> port = find_cxl_port_at(parent_port, dport_dev, &dport); > Can this call to find_cxl_port_at() fail? I have thought about it during making this changes. I was also confused if the find_cxl_port_at() could fail. my understanding is that this new cxl port is just added before calling find_cxl_port_at() with holding the lock of parent_port, the find_cxl_port_at() will always get the cxl_port in this case. other threads have no chance to release the new cxl_port in this case. >> + } >> } >> -out: >> - device_unlock(&parent_port->dev); >> >> - if (IS_ERR(port)) > ^^^^^^^^^^^^ > Does this need to be checked above? > > It seems like the logic is changing for the call to cxl_add_ep() but it > might be ok? > > Ira If my understanding is correct, you meat that above 'IS_ERR(port)' can help to check the result of the above 'port = find_cxl_port_at(parent_port, dport_dev, &dport)', but actually It cannot, because the return of find_cxl_port_at() is a correct cxl port or a NULL. It can help if it is a 'IS_ERR_OR_NULL()'. > >> - rc = PTR_ERR(port); >> - else { >> - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", >> - dev_name(&port->dev), dev_name(port->uport_dev)); >> - rc = cxl_add_ep(dport, &cxlmd->dev); >> - if (rc == -EBUSY) { >> - /* >> - * "can't" happen, but this error code means >> - * something to the caller, so translate it. >> - */ >> - rc = -ENXIO; >> - } >> + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", >> + dev_name(&port->dev), dev_name(port->uport_dev)); >> + rc = cxl_add_ep(dport, &cxlmd->dev); >> + if (rc == -EBUSY) { >> + /* >> + * "can't" happen, but this error code means >> + * something to the caller, so translate it. >> + */ >> + rc = -ENXIO; >> } >> >> return rc; >> @@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map) >> >> port = to_cxl_port(cxld->dev.parent); >> >> - device_lock(&port->dev); >> - rc = cxl_decoder_add_locked(cxld, target_map); >> - device_unlock(&port->dev); >> + scoped_guard(device, &port->dev) >> + rc = cxl_decoder_add_locked(cxld, target_map); >> >> return rc; >> } >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 21ad5f242875..a3ce7cbe5c6c 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3094,11 +3094,11 @@ static void cxlr_release_nvdimm(void *_cxlr) >> struct cxl_region *cxlr = _cxlr; >> struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb; >> >> - device_lock(&cxl_nvb->dev); >> - if (cxlr->cxlr_pmem) >> - devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, >> - cxlr->cxlr_pmem); >> - device_unlock(&cxl_nvb->dev); >> + scoped_guard(device, &cxl_nvb->dev) { >> + if (cxlr->cxlr_pmem) >> + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, >> + cxlr->cxlr_pmem); >> + } >> cxlr->cxl_nvb = NULL; >> put_device(&cxl_nvb->dev); >> } >> @@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) >> dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent), >> dev_name(dev)); >> >> - device_lock(&cxl_nvb->dev); >> - if (cxl_nvb->dev.driver) >> - rc = devm_add_action_or_reset(&cxl_nvb->dev, >> - cxlr_pmem_unregister, cxlr_pmem); >> - else >> - rc = -ENXIO; >> - device_unlock(&cxl_nvb->dev); >> - >> + scoped_guard(device, &cxl_nvb->dev) { >> + if (cxl_nvb->dev.driver) >> + rc = devm_add_action_or_reset(&cxl_nvb->dev, >> + cxlr_pmem_unregister, cxlr_pmem); >> + else >> + rc = -ENXIO; >> + } >> if (rc) >> goto err_bridge; >> >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >> index ab9b8ab8df44..ae94018a01bd 100644 >> --- a/drivers/cxl/mem.c >> +++ b/drivers/cxl/mem.c >> @@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev) >> >> cxl_setup_parent_dport(dev, dport); >> >> - device_lock(endpoint_parent); >> - if (!endpoint_parent->driver) { >> - dev_err(dev, "CXL port topology %s not enabled\n", >> - dev_name(endpoint_parent)); >> - rc = -ENXIO; >> - goto unlock; >> - } >> + scoped_guard(device, endpoint_parent) { >> + if (!endpoint_parent->driver) { >> + dev_err(dev, "CXL port topology %s not enabled\n", >> + dev_name(endpoint_parent)); >> + return -ENXIO; >> + } >> >> - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); >> -unlock: >> - device_unlock(endpoint_parent); >> - if (rc) >> - return rc; >> + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); >> + if (rc) >> + return rc; >> + } >> >> /* >> * The kernel may be operating out of CXL memory on this device, >> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c >> index 4ef93da22335..647c7e25ef3a 100644 >> --- a/drivers/cxl/pmem.c >> +++ b/drivers/cxl/pmem.c >> @@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data) >> if (!is_cxl_nvdimm(dev)) >> return 0; >> >> - device_lock(dev); >> - if (!dev->driver) >> - goto out; >> - >> - cxl_nvd = to_cxl_nvdimm(dev); >> - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) >> - release = true; >> -out: >> - device_unlock(dev); >> + scoped_guard(device, dev) { >> + if (dev->driver) { >> + cxl_nvd = to_cxl_nvdimm(dev); >> + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) >> + release = true; >> + } >> + } >> if (release) >> device_release_driver(dev); >> return 0; >> -- >> 2.40.1 >> >
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index e5cdeafdf76e..a8ab5f2697be 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) int rc; /* synchronize with cxl_mem_probe() and decoder write operations */ - device_lock(&cxlmd->dev); - endpoint = cxlmd->endpoint; - down_read(&cxl_region_rwsem); - /* - * Require an endpoint to be safe otherwise the driver can not - * be sure that the device is unmapped. - */ - if (endpoint && cxl_num_decoders_committed(endpoint) == 0) - rc = __cxl_mem_sanitize(mds, cmd); - else - rc = -EBUSY; - up_read(&cxl_region_rwsem); - device_unlock(&cxlmd->dev); + scoped_guard(device, &cxlmd->dev) { + endpoint = cxlmd->endpoint; + down_read(&cxl_region_rwsem); + /* + * Require an endpoint to be safe otherwise the driver can not + * be sure that the device is unmapped. + */ + if (endpoint && cxl_num_decoders_committed(endpoint) == 0) + rc = __cxl_mem_sanitize(mds, cmd); + else + rc = -EBUSY; + up_read(&cxl_region_rwsem); + } return rc; } diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 6119cb3ad25c..53e2593daa95 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new) struct cxl_port *port = new->dport->port; int rc; - device_lock(&port->dev); - if (port->dead) { - device_unlock(&port->dev); - return -ENXIO; + scoped_guard(device, &port->dev) { + if (port->dead) + return -ENXIO; + rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, + GFP_KERNEL); } - rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new, - GFP_KERNEL); - device_unlock(&port->dev); return rc; } @@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data) struct cxl_port *endpoint = cxlmd->endpoint; struct device *host = endpoint_host(endpoint); - device_lock(host); - if (host->driver && !endpoint->dead) { - devm_release_action(host, cxl_unlink_parent_dport, endpoint); - devm_release_action(host, cxl_unlink_uport, endpoint); - devm_release_action(host, unregister_port, endpoint); + scoped_guard(device, host) { + if (host->driver && !endpoint->dead) { + devm_release_action(host, cxl_unlink_parent_dport, endpoint); + devm_release_action(host, cxl_unlink_uport, endpoint); + devm_release_action(host, unregister_port, endpoint); + } + cxlmd->endpoint = NULL; } - cxlmd->endpoint = NULL; - device_unlock(host); put_device(&endpoint->dev); put_device(host); } @@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd, return -EAGAIN; } - device_lock(&parent_port->dev); - if (!parent_port->dev.driver) { - dev_warn(&cxlmd->dev, - "port %s:%s disabled, failed to enumerate CXL.mem\n", - dev_name(&parent_port->dev), dev_name(uport_dev)); - port = ERR_PTR(-ENXIO); - goto out; - } + scoped_guard(device, &parent_port->dev) { + if (!parent_port->dev.driver) { + dev_warn(&cxlmd->dev, + "port %s:%s disabled, failed to enumerate CXL.mem\n", + dev_name(&parent_port->dev), dev_name(uport_dev)); + return -ENXIO; + } - port = find_cxl_port_at(parent_port, dport_dev, &dport); - if (!port) { - component_reg_phys = find_component_registers(uport_dev); - port = devm_cxl_add_port(&parent_port->dev, uport_dev, - component_reg_phys, parent_dport); - /* retry find to pick up the new dport information */ - if (!IS_ERR(port)) + port = find_cxl_port_at(parent_port, dport_dev, &dport); + if (!port) { + component_reg_phys = find_component_registers(uport_dev); + port = devm_cxl_add_port(&parent_port->dev, uport_dev, + component_reg_phys, parent_dport); + if (IS_ERR(port)) + return PTR_ERR(port); + + /* retry find to pick up the new dport information */ port = find_cxl_port_at(parent_port, dport_dev, &dport); + } } -out: - device_unlock(&parent_port->dev); - if (IS_ERR(port)) - rc = PTR_ERR(port); - else { - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", - dev_name(&port->dev), dev_name(port->uport_dev)); - rc = cxl_add_ep(dport, &cxlmd->dev); - if (rc == -EBUSY) { - /* - * "can't" happen, but this error code means - * something to the caller, so translate it. - */ - rc = -ENXIO; - } + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n", + dev_name(&port->dev), dev_name(port->uport_dev)); + rc = cxl_add_ep(dport, &cxlmd->dev); + if (rc == -EBUSY) { + /* + * "can't" happen, but this error code means + * something to the caller, so translate it. + */ + rc = -ENXIO; } return rc; @@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map) port = to_cxl_port(cxld->dev.parent); - device_lock(&port->dev); - rc = cxl_decoder_add_locked(cxld, target_map); - device_unlock(&port->dev); + scoped_guard(device, &port->dev) + rc = cxl_decoder_add_locked(cxld, target_map); return rc; } diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 21ad5f242875..a3ce7cbe5c6c 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3094,11 +3094,11 @@ static void cxlr_release_nvdimm(void *_cxlr) struct cxl_region *cxlr = _cxlr; struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb; - device_lock(&cxl_nvb->dev); - if (cxlr->cxlr_pmem) - devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, - cxlr->cxlr_pmem); - device_unlock(&cxl_nvb->dev); + scoped_guard(device, &cxl_nvb->dev) { + if (cxlr->cxlr_pmem) + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister, + cxlr->cxlr_pmem); + } cxlr->cxl_nvb = NULL; put_device(&cxl_nvb->dev); } @@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr) dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent), dev_name(dev)); - device_lock(&cxl_nvb->dev); - if (cxl_nvb->dev.driver) - rc = devm_add_action_or_reset(&cxl_nvb->dev, - cxlr_pmem_unregister, cxlr_pmem); - else - rc = -ENXIO; - device_unlock(&cxl_nvb->dev); - + scoped_guard(device, &cxl_nvb->dev) { + if (cxl_nvb->dev.driver) + rc = devm_add_action_or_reset(&cxl_nvb->dev, + cxlr_pmem_unregister, cxlr_pmem); + else + rc = -ENXIO; + } if (rc) goto err_bridge; diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index ab9b8ab8df44..ae94018a01bd 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev) cxl_setup_parent_dport(dev, dport); - device_lock(endpoint_parent); - if (!endpoint_parent->driver) { - dev_err(dev, "CXL port topology %s not enabled\n", - dev_name(endpoint_parent)); - rc = -ENXIO; - goto unlock; - } + scoped_guard(device, endpoint_parent) { + if (!endpoint_parent->driver) { + dev_err(dev, "CXL port topology %s not enabled\n", + dev_name(endpoint_parent)); + return -ENXIO; + } - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); -unlock: - device_unlock(endpoint_parent); - if (rc) - return rc; + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport); + if (rc) + return rc; + } /* * The kernel may be operating out of CXL memory on this device, diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c index 4ef93da22335..647c7e25ef3a 100644 --- a/drivers/cxl/pmem.c +++ b/drivers/cxl/pmem.c @@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data) if (!is_cxl_nvdimm(dev)) return 0; - device_lock(dev); - if (!dev->driver) - goto out; - - cxl_nvd = to_cxl_nvdimm(dev); - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) - release = true; -out: - device_unlock(dev); + scoped_guard(device, dev) { + if (dev->driver) { + cxl_nvd = to_cxl_nvdimm(dev); + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data) + release = true; + } + } if (release) device_release_driver(dev); return 0;
A device_lock() and device_unlock() pair can be replaced by a scope-based resource management function scoped_guard() which can make the code more readable and safer. Some device_lock() and device_unlock() pairs in CXL subsystem can be simply replaced by a scoped_guard(). Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Li Ming <ming4.li@intel.com> --- drivers/cxl/core/mbox.c | 26 ++++++------ drivers/cxl/core/port.c | 87 ++++++++++++++++++--------------------- drivers/cxl/core/region.c | 25 ++++++----- drivers/cxl/mem.c | 22 +++++----- drivers/cxl/pmem.c | 16 ++++--- 5 files changed, 82 insertions(+), 94 deletions(-)