| Message ID | 20250217144828.30651-6-ming.li@zohomail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use guard() instead of rwsem locking | expand |
On 2/17/25 7:48 AM, Li Ming wrote: > In cxl_dpa_alloc(), some checking and operations need to be protected by > a rwsem called cxl_dpa_rwsem, so there is a goto pattern in > cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized > by using guard() to hold the rwsem. > > Creating a new function called __cxl_dpa_alloc() to include all checking > and operations needed to be procted by cxl_dpa_rwsem. Using s/procted/protected/ > guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new > function. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/hdm.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 874a791f8292..1edbf7873471 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -442,29 +442,25 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, > return 0; > } > > -int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > +static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > { > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > resource_size_t free_ram_start, free_pmem_start; > - struct cxl_port *port = cxled_to_port(cxled); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > struct device *dev = &cxled->cxld.dev; > resource_size_t start, avail, skip; > struct resource *p, *last; > - int rc; > > - down_write(&cxl_dpa_rwsem); > + guard(rwsem_write)(&cxl_dpa_rwsem); > if (cxled->cxld.region) { > dev_dbg(dev, "decoder attached to %s\n", > dev_name(&cxled->cxld.region->dev)); > - rc = -EBUSY; > - goto out; > + return -EBUSY; > } > > if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) { > dev_dbg(dev, "decoder enabled\n"); > - rc = -EBUSY; > - goto out; > + return -EBUSY; > } > > for (p = cxlds->ram_res.child, last = NULL; p; p = p->sibling) > @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > skip = skip_end - skip_start + 1; > } else { > dev_dbg(dev, "mode not set\n"); > - rc = -EINVAL; > - goto out; > + return -EINVAL; > } > > if (size > avail) { > dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size, > cxl_decoder_mode_name(cxled->mode), &avail); > - rc = -ENOSPC; > - goto out; > + return -ENOSPC; > } > > - rc = __cxl_dpa_reserve(cxled, start, size, skip); > -out: > - up_write(&cxl_dpa_rwsem); > + return __cxl_dpa_reserve(cxled, start, size, skip); > +} > + > +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > +{ > + struct cxl_port *port = cxled_to_port(cxled); > + int rc; > > + rc = __cxl_dpa_alloc(cxled, size); > if (rc) > return rc; >
On Mon, Feb 17, 2025 at 10:48:26PM +0800, Li Ming wrote: > In cxl_dpa_alloc(), some checking and operations need to be protected by > a rwsem called cxl_dpa_rwsem, so there is a goto pattern in > cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized > by using guard() to hold the rwsem. > > Creating a new function called __cxl_dpa_alloc() to include all checking > and operations needed to be procted by cxl_dpa_rwsem. Using > guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new > function. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Alison Schofield <alison.schofield@intel.com> snip
Li Ming wrote: > In cxl_dpa_alloc(), some checking and operations need to be protected by > a rwsem called cxl_dpa_rwsem, so there is a goto pattern in > cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized > by using guard() to hold the rwsem. > > Creating a new function called __cxl_dpa_alloc() to include all checking > and operations needed to be procted by cxl_dpa_rwsem. Using > guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new > function. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Li Ming <ming.li@zohomail.com> > --- > drivers/cxl/core/hdm.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 874a791f8292..1edbf7873471 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c [..] > @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > skip = skip_end - skip_start + 1; > } else { > dev_dbg(dev, "mode not set\n"); > - rc = -EINVAL; > - goto out; > + return -EINVAL; > } > > if (size > avail) { > dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size, > cxl_decoder_mode_name(cxled->mode), &avail); > - rc = -ENOSPC; > - goto out; > + return -ENOSPC; > } > > - rc = __cxl_dpa_reserve(cxled, start, size, skip); > -out: > - up_write(&cxl_dpa_rwsem); > + return __cxl_dpa_reserve(cxled, start, size, skip); > +} > + > +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) > +{ > + struct cxl_port *port = cxled_to_port(cxled); Am I missing something? This @port variable is unused? > + int rc; > > + rc = __cxl_dpa_alloc(cxled, size); > if (rc) > return rc; > So I think you can drop this new cxl_dpa_alloc + __cxl_dpa_alloc scheme? After that you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On 2/20/2025 9:01 AM, Dan Williams wrote: > Li Ming wrote: >> In cxl_dpa_alloc(), some checking and operations need to be protected by >> a rwsem called cxl_dpa_rwsem, so there is a goto pattern in >> cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized >> by using guard() to hold the rwsem. >> >> Creating a new function called __cxl_dpa_alloc() to include all checking >> and operations needed to be procted by cxl_dpa_rwsem. Using >> guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new >> function. >> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Signed-off-by: Li Ming <ming.li@zohomail.com> >> --- >> drivers/cxl/core/hdm.c | 29 ++++++++++++++--------------- >> 1 file changed, 14 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 874a791f8292..1edbf7873471 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c > [..] >> @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) >> skip = skip_end - skip_start + 1; >> } else { >> dev_dbg(dev, "mode not set\n"); >> - rc = -EINVAL; >> - goto out; >> + return -EINVAL; >> } >> >> if (size > avail) { >> dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size, >> cxl_decoder_mode_name(cxled->mode), &avail); >> - rc = -ENOSPC; >> - goto out; >> + return -ENOSPC; >> } >> >> - rc = __cxl_dpa_reserve(cxled, start, size, skip); >> -out: >> - up_write(&cxl_dpa_rwsem); >> + return __cxl_dpa_reserve(cxled, start, size, skip); >> +} >> + >> +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) >> +{ >> + struct cxl_port *port = cxled_to_port(cxled); > Am I missing something? This @port variable is unused? @port will be used after below __cxl_dpa_alloc(). > >> + int rc; >> >> + rc = __cxl_dpa_alloc(cxled, size); >> if (rc) >> return rc; >> > So I think you can drop this new cxl_dpa_alloc + __cxl_dpa_alloc scheme? After __cxl_dpa_alloc(), a 'devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled)' will be invoked, cxl_dpa_rwsem is possible to be held in cxl_dpa_release() in devm_add_action_or_reset() failure case. So I create __cxl_dpa_alloc() to hold cxl_dpa_rwsem for the operations needed cxl_dpa_resem protection, make sure that the cxl_dpa_rwsem is released before devm_add_action_or_reset() invoking. > > After that you can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Li Ming wrote: [..] > After __cxl_dpa_alloc(), a 'devm_add_action_or_reset(&port->dev, > cxl_dpa_release, cxled)' will be invoked, cxl_dpa_rwsem is possible to > be held in cxl_dpa_release() in devm_add_action_or_reset() failure > case. > > So I create __cxl_dpa_alloc() to hold cxl_dpa_rwsem for the operations > needed cxl_dpa_resem protection, make sure that the cxl_dpa_rwsem is > released before devm_add_action_or_reset() invoking. Ah, got it, missing context, makes sense.
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 874a791f8292..1edbf7873471 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -442,29 +442,25 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled, return 0; } -int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) +static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) { struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); resource_size_t free_ram_start, free_pmem_start; - struct cxl_port *port = cxled_to_port(cxled); struct cxl_dev_state *cxlds = cxlmd->cxlds; struct device *dev = &cxled->cxld.dev; resource_size_t start, avail, skip; struct resource *p, *last; - int rc; - down_write(&cxl_dpa_rwsem); + guard(rwsem_write)(&cxl_dpa_rwsem); if (cxled->cxld.region) { dev_dbg(dev, "decoder attached to %s\n", dev_name(&cxled->cxld.region->dev)); - rc = -EBUSY; - goto out; + return -EBUSY; } if (cxled->cxld.flags & CXL_DECODER_F_ENABLE) { dev_dbg(dev, "decoder enabled\n"); - rc = -EBUSY; - goto out; + return -EBUSY; } for (p = cxlds->ram_res.child, last = NULL; p; p = p->sibling) @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) skip = skip_end - skip_start + 1; } else { dev_dbg(dev, "mode not set\n"); - rc = -EINVAL; - goto out; + return -EINVAL; } if (size > avail) { dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size, cxl_decoder_mode_name(cxled->mode), &avail); - rc = -ENOSPC; - goto out; + return -ENOSPC; } - rc = __cxl_dpa_reserve(cxled, start, size, skip); -out: - up_write(&cxl_dpa_rwsem); + return __cxl_dpa_reserve(cxled, start, size, skip); +} + +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size) +{ + struct cxl_port *port = cxled_to_port(cxled); + int rc; + rc = __cxl_dpa_alloc(cxled, size); if (rc) return rc;