| Message ID | 20231212-vv-dax_abi-v4-2-1351758f0c92@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add DAX ABI for memmap_on_memory | expand |
Vishal Verma wrote: > Introduce a guard(device) macro to lock a 'struct device', and unlock it > automatically when going out of scope using Scope Based Resource > Management semantics. A lot of the sysfs attribute writes in > drivers/dax/bus.c benefit from a cleanup using these, so change these > where applicable. > > Cc: Joao Martins <joao.m.martins@oracle.com> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Tue, 12 Dec 2023 12:08:31 -0700 Vishal Verma <vishal.l.verma@intel.com> wrote: > Introduce a guard(device) macro to lock a 'struct device', and unlock it > automatically when going out of scope using Scope Based Resource > Management semantics. A lot of the sysfs attribute writes in > drivers/dax/bus.c benefit from a cleanup using these, so change these > where applicable. > > Cc: Joao Martins <joao.m.martins@oracle.com> > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> Hi Vishal, I'm a big fan of this cleanup.h stuff so very happen to see this getting used here. There are added opportunities for cleanup that result. Note that almost every time I see people using this stuff they don't look again at the code post the change so miss the wider cleanup that it enables. So you are in good company ;) Jonathan > --- > include/linux/device.h | 2 + > drivers/dax/bus.c | 109 +++++++++++++++++++------------------------------ > 2 files changed, 44 insertions(+), 67 deletions(-) > > diff --git a/include/linux/device.h b/include/linux/device.h > index d7a72a8749ea..a83efd9ae949 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -1131,6 +1131,8 @@ void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode); > void device_set_of_node_from_dev(struct device *dev, const struct device *dev2); > void device_set_node(struct device *dev, struct fwnode_handle *fwnode); > > +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) Nice. I'd expect this to be widely adopted, so maybe to make things less painful for backporting changes that depend on it, make this a separate trivial patch rather than having this in here. > + > static inline int dev_num_vf(struct device *dev) > { > if (dev->bus && dev->bus->num_vf) > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index 1ff1ab5fa105..ce1356ac6dc2 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -296,9 +296,8 @@ static ssize_t available_size_show(struct device *dev, > struct dax_region *dax_region = dev_get_drvdata(dev); > unsigned long long size; > > - device_lock(dev); > + guard(device)(dev); > size = dax_region_avail_size(dax_region); > - device_unlock(dev); > > return sprintf(buf, "%llu\n", size); return sprintf(buf, @%llu\n@, dax_region_avail_size(dax_region)); and drop the local variable that adds little perhaps? > } > @@ -314,10 +313,9 @@ static ssize_t seed_show(struct device *dev, > if (is_static(dax_region)) > return -EINVAL; > > - device_lock(dev); > + guard(device)(dev); > seed = dax_region->seed; > rc = sprintf(buf, "%s\n", seed ? dev_name(seed) : ""); return sprintf(); > - device_unlock(dev); > > return rc; > } > @@ -333,10 +331,9 @@ static ssize_t create_show(struct device *dev, > if (is_static(dax_region)) > return -EINVAL; > > - device_lock(dev); > + guard(device)(dev); > youngest = dax_region->youngest; > rc = sprintf(buf, "%s\n", youngest ? dev_name(youngest) : ""); return sprintf(); > - device_unlock(dev); > > return rc; > } > @@ -345,7 +342,14 @@ static ssize_t create_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t len) > { > struct dax_region *dax_region = dev_get_drvdata(dev); > + struct dev_dax_data data = { > + .dax_region = dax_region, > + .size = 0, > + .id = -1, > + .memmap_on_memory = false, > + }; > unsigned long long avail; > + struct dev_dax *dev_dax; > ssize_t rc; > int val; > > @@ -358,38 +362,26 @@ static ssize_t create_store(struct device *dev, struct device_attribute *attr, > if (val != 1) > return -EINVAL; > > - device_lock(dev); > + guard(device)(dev); > avail = dax_region_avail_size(dax_region); > if (avail == 0) > - rc = -ENOSPC; > - else { > - struct dev_dax_data data = { > - .dax_region = dax_region, > - .size = 0, > - .id = -1, > - .memmap_on_memory = false, > - }; > - struct dev_dax *dev_dax = devm_create_dev_dax(&data); > + return -ENOSPC; > > - if (IS_ERR(dev_dax)) > - rc = PTR_ERR(dev_dax); > - else { > - /* > - * In support of crafting multiple new devices > - * simultaneously multiple seeds can be created, > - * but only the first one that has not been > - * successfully bound is tracked as the region > - * seed. > - */ > - if (!dax_region->seed) > - dax_region->seed = &dev_dax->dev; > - dax_region->youngest = &dev_dax->dev; > - rc = len; > - } > - } > - device_unlock(dev); > + dev_dax = devm_create_dev_dax(&data); > + if (IS_ERR(dev_dax)) > + return PTR_ERR(dev_dax); > > - return rc; > + /* > + * In support of crafting multiple new devices rewrap this comment for the new indent. > + * simultaneously multiple seeds can be created, > + * but only the first one that has not been > + * successfully bound is tracked as the region > + * seed. > + */ > + if (!dax_region->seed) > + dax_region->seed = &dev_dax->dev; > + dax_region->youngest = &dev_dax->dev; Trivial but blank line here would be a nice to have > + return len; > } ... > @@ -1138,18 +1123,14 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, > return rc; > > rc = -ENXIO; Not needed with suggested changes that follow. > - device_lock(dax_region->dev); > - if (!dax_region->dev->driver) { > - device_unlock(dax_region->dev); > + guard(device)(dax_region->dev); > + if (!dax_region->dev->driver) > return rc; return -ENXIO; > - } > - device_lock(dev); > > + guard(device)(dev); > to_alloc = range_len(&r); > if (alloc_is_aligned(dev_dax, to_alloc)) > rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); Flip logic here and I'd drop the ternary stuff as well - same in other similar cases in this patch (though that is just personal taste) if (!alloc_is_aligned(dev_dax, to_allco)) return -ENXIO. rc = alloc_dev_dax_range(dev_dax, r.start, to_allco) if (rc) return rc; return len; > - device_unlock(dev); > - device_unlock(dax_region->dev); > > return rc == 0 ? len : rc; > } >
diff --git a/include/linux/device.h b/include/linux/device.h index d7a72a8749ea..a83efd9ae949 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -1131,6 +1131,8 @@ void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode); void device_set_of_node_from_dev(struct device *dev, const struct device *dev2); void device_set_node(struct device *dev, struct fwnode_handle *fwnode); +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T)) + static inline int dev_num_vf(struct device *dev) { if (dev->bus && dev->bus->num_vf) diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index 1ff1ab5fa105..ce1356ac6dc2 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -296,9 +296,8 @@ static ssize_t available_size_show(struct device *dev, struct dax_region *dax_region = dev_get_drvdata(dev); unsigned long long size; - device_lock(dev); + guard(device)(dev); size = dax_region_avail_size(dax_region); - device_unlock(dev); return sprintf(buf, "%llu\n", size); } @@ -314,10 +313,9 @@ static ssize_t seed_show(struct device *dev, if (is_static(dax_region)) return -EINVAL; - device_lock(dev); + guard(device)(dev); seed = dax_region->seed; rc = sprintf(buf, "%s\n", seed ? dev_name(seed) : ""); - device_unlock(dev); return rc; } @@ -333,10 +331,9 @@ static ssize_t create_show(struct device *dev, if (is_static(dax_region)) return -EINVAL; - device_lock(dev); + guard(device)(dev); youngest = dax_region->youngest; rc = sprintf(buf, "%s\n", youngest ? dev_name(youngest) : ""); - device_unlock(dev); return rc; } @@ -345,7 +342,14 @@ static ssize_t create_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { struct dax_region *dax_region = dev_get_drvdata(dev); + struct dev_dax_data data = { + .dax_region = dax_region, + .size = 0, + .id = -1, + .memmap_on_memory = false, + }; unsigned long long avail; + struct dev_dax *dev_dax; ssize_t rc; int val; @@ -358,38 +362,26 @@ static ssize_t create_store(struct device *dev, struct device_attribute *attr, if (val != 1) return -EINVAL; - device_lock(dev); + guard(device)(dev); avail = dax_region_avail_size(dax_region); if (avail == 0) - rc = -ENOSPC; - else { - struct dev_dax_data data = { - .dax_region = dax_region, - .size = 0, - .id = -1, - .memmap_on_memory = false, - }; - struct dev_dax *dev_dax = devm_create_dev_dax(&data); + return -ENOSPC; - if (IS_ERR(dev_dax)) - rc = PTR_ERR(dev_dax); - else { - /* - * In support of crafting multiple new devices - * simultaneously multiple seeds can be created, - * but only the first one that has not been - * successfully bound is tracked as the region - * seed. - */ - if (!dax_region->seed) - dax_region->seed = &dev_dax->dev; - dax_region->youngest = &dev_dax->dev; - rc = len; - } - } - device_unlock(dev); + dev_dax = devm_create_dev_dax(&data); + if (IS_ERR(dev_dax)) + return PTR_ERR(dev_dax); - return rc; + /* + * In support of crafting multiple new devices + * simultaneously multiple seeds can be created, + * but only the first one that has not been + * successfully bound is tracked as the region + * seed. + */ + if (!dax_region->seed) + dax_region->seed = &dev_dax->dev; + dax_region->youngest = &dev_dax->dev; + return len; } static DEVICE_ATTR_RW(create); @@ -481,12 +473,9 @@ static int __free_dev_dax_id(struct dev_dax *dev_dax) static int free_dev_dax_id(struct dev_dax *dev_dax) { struct device *dev = &dev_dax->dev; - int rc; - device_lock(dev); - rc = __free_dev_dax_id(dev_dax); - device_unlock(dev); - return rc; + guard(device)(dev); + return __free_dev_dax_id(dev_dax); } static int alloc_dev_dax_id(struct dev_dax *dev_dax) @@ -908,9 +897,8 @@ static ssize_t size_show(struct device *dev, struct dev_dax *dev_dax = to_dev_dax(dev); unsigned long long size; - device_lock(dev); + guard(device)(dev); size = dev_dax_size(dev_dax); - device_unlock(dev); return sprintf(buf, "%llu\n", size); } @@ -1080,15 +1068,12 @@ static ssize_t size_store(struct device *dev, struct device_attribute *attr, return -EINVAL; } - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) return -ENXIO; - } - device_lock(dev); + + guard(device)(dev); rc = dev_dax_resize(dax_region, dev_dax, val); - device_unlock(dev); - device_unlock(dax_region->dev); return rc == 0 ? len : rc; } @@ -1138,18 +1123,14 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, return rc; rc = -ENXIO; - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) return rc; - } - device_lock(dev); + guard(device)(dev); to_alloc = range_len(&r); if (alloc_is_aligned(dev_dax, to_alloc)) rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); - device_unlock(dev); - device_unlock(dax_region->dev); return rc == 0 ? len : rc; } @@ -1196,26 +1177,20 @@ static ssize_t align_store(struct device *dev, struct device_attribute *attr, if (!dax_align_valid(val)) return -EINVAL; - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) return -ENXIO; - } - device_lock(dev); - if (dev->driver) { - rc = -EBUSY; - goto out_unlock; - } + guard(device)(dev); + if (dev->driver) + return -EBUSY; align_save = dev_dax->align; dev_dax->align = val; rc = dev_dax_validate_align(dev_dax); if (rc) dev_dax->align = align_save; -out_unlock: - device_unlock(dev); - device_unlock(dax_region->dev); + return rc == 0 ? len : rc; } static DEVICE_ATTR_RW(align);
Introduce a guard(device) macro to lock a 'struct device', and unlock it automatically when going out of scope using Scope Based Resource Management semantics. A lot of the sysfs attribute writes in drivers/dax/bus.c benefit from a cleanup using these, so change these where applicable. Cc: Joao Martins <joao.m.martins@oracle.com> Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- include/linux/device.h | 2 + drivers/dax/bus.c | 109 +++++++++++++++++++------------------------------ 2 files changed, 44 insertions(+), 67 deletions(-)