[v6,2/4] dax/bus: Use guard(device) in sysfs attribute helpers
Commit Message
Use the 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>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
drivers/dax/bus.c | 143 ++++++++++++++++++++++--------------------------------
1 file changed, 59 insertions(+), 84 deletions(-)
Comments
On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
> - unsigned long long size;
>
> - device_lock(dev);
> - size = dax_region_avail_size(dax_region);
> - device_unlock(dev);
> + guard(device)(dev);
>
> - return sprintf(buf, "%llu\n", size);
> + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> }
Is this an appropriate use of guard()? sprintf is not the fastest of
functions, so we will end up holding the device_lock for longer than
we used to.
> @@ -908,9 +890,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);
> }
If it is appropriate, then you can do without the 'size' variable here.
> @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr,
> if (rc)
> return rc;
>
> - rc = -ENXIO;
> - device_lock(dax_region->dev);
> - if (!dax_region->dev->driver) {
> - device_unlock(dax_region->dev);
> - return rc;
> - }
> - device_lock(dev);
> + guard(device)(dax_region->dev);
> + if (!dax_region->dev->driver)
> + return -ENXIO;
>
> + 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);
> + if (!alloc_is_aligned(dev_dax, to_alloc))
> + return -ENXIO;
>
> - return rc == 0 ? len : rc;
> + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> + if (rc)
> + return rc;
> +
> + return len;
> }
Have I mentioned how much I hate the "rc" naming convention? It tells
you nothing useful about the contents of the variable. If you called it
'err', I'd know it was an error, and then the end of this function would
make sense.
if (err)
return err;
return len;
On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote:
> On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > struct dax_region *dax_region = dev_get_drvdata(dev);
> > - unsigned long long size;
> >
> > - device_lock(dev);
> > - size = dax_region_avail_size(dax_region);
> > - device_unlock(dev);
> > + guard(device)(dev);
> >
> > - return sprintf(buf, "%llu\n", size);
> > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> > }
>
> Is this an appropriate use of guard()? sprintf is not the fastest of
> functions, so we will end up holding the device_lock for longer than
> we used to.
Hi Matthew,
Agreed that we end up holding the lock for a bit longer in many of
these. I'm inclined to say this is okay, since these are all user
configuration paths through sysfs, not affecting any sort of runtime
performance.
>
> > @@ -908,9 +890,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);
> > }
>
> If it is appropriate, then you can do without the 'size' variable here.
Yep will remove. I suppose a lot of these can also switch to sysfs_emit
as Greg pointed out in a previous posting. I can add that as a separate
cleanup patch.
>
> > @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr,
> > if (rc)
> > return rc;
> >
> > - rc = -ENXIO;
> > - device_lock(dax_region->dev);
> > - if (!dax_region->dev->driver) {
> > - device_unlock(dax_region->dev);
> > - return rc;
> > - }
> > - device_lock(dev);
> > + guard(device)(dax_region->dev);
> > + if (!dax_region->dev->driver)
> > + return -ENXIO;
> >
> > + 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);
> > + if (!alloc_is_aligned(dev_dax, to_alloc))
> > + return -ENXIO;
> >
> > - return rc == 0 ? len : rc;
> > + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> > + if (rc)
> > + return rc;
> > +
> > + return len;
> > }
>
> Have I mentioned how much I hate the "rc" naming convention? It tells
> you nothing useful about the contents of the variable. If you called it
> 'err', I'd know it was an error, and then the end of this function would
> make sense.
>
> if (err)
> return err;
> return len;
>
I'm a little hesitant to change this because the 'rc' convention is
used all over this file, and while I don't mind making this change for
the bits I touch in this patch, it would just result in a mix of 'rc'
and 'err' in this file.
On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> Use the 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.
Wait, why are you needing to call device_lock() at all here? Why is dax
special in needing this when no other subsystem requires it?
>
> Cc: Joao Martins <joao.m.martins@oracle.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
> drivers/dax/bus.c | 143 ++++++++++++++++++++++--------------------------------
> 1 file changed, 59 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 1ff1ab5fa105..6226de131d17 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
> - unsigned long long size;
>
> - device_lock(dev);
> - size = dax_region_avail_size(dax_region);
> - device_unlock(dev);
> + guard(device)(dev);
You have a valid device here, why are you locking it? How can it go
away? And if it can, shouldn't you have a local lock for it, and not
abuse the driver core lock?
>
> - return sprintf(buf, "%llu\n", size);
> + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
sysfs_emit() everywhere please.
But again, the issue is "why do you need a lock"?
thanks,
greg k-h
On Fri, Dec 15, 2023 at 06:33:58AM +0000, Verma, Vishal L wrote:
> On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote:
> > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > struct dax_region *dax_region = dev_get_drvdata(dev);
> > > - unsigned long long size;
> > >
> > > - device_lock(dev);
> > > - size = dax_region_avail_size(dax_region);
> > > - device_unlock(dev);
> > > + guard(device)(dev);
> > >
> > > - return sprintf(buf, "%llu\n", size);
> > > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> > > }
> >
> > Is this an appropriate use of guard()? sprintf is not the fastest of
> > functions, so we will end up holding the device_lock for longer than
> > we used to.
>
> Hi Matthew,
>
> Agreed that we end up holding the lock for a bit longer in many of
> these. I'm inclined to say this is okay, since these are all user
> configuration paths through sysfs, not affecting any sort of runtime
> performance.
Why does the lock have to be taken at all? You have a valid reference,
isn't that all you need?
thanks,
greg k-h
Greg Kroah-Hartman wrote:
> On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > Use the 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.
>
> Wait, why are you needing to call device_lock() at all here? Why is dax
> special in needing this when no other subsystem requires it?
>
> >
> > Cc: Joao Martins <joao.m.martins@oracle.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> > drivers/dax/bus.c | 143 ++++++++++++++++++++++--------------------------------
> > 1 file changed, 59 insertions(+), 84 deletions(-)
> >
> > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > index 1ff1ab5fa105..6226de131d17 100644
> > --- a/drivers/dax/bus.c
> > +++ b/drivers/dax/bus.c
> > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > struct dax_region *dax_region = dev_get_drvdata(dev);
> > - unsigned long long size;
> >
> > - device_lock(dev);
> > - size = dax_region_avail_size(dax_region);
> > - device_unlock(dev);
> > + guard(device)(dev);
>
> You have a valid device here, why are you locking it? How can it go
> away? And if it can, shouldn't you have a local lock for it, and not
> abuse the driver core lock?
Yes, this is a driver-core lock abuse written by someone who should have
known better. And yes, a local lock to protect the dax_region resource
tree should replace this. A new rwsem to synchronize all list walks
seems appropriate.
On Fri, 2023-12-15 at 09:15 -0800, Dan Williams wrote:
> Greg Kroah-Hartman wrote:
> > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > > Use the 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.
> >
> > Wait, why are you needing to call device_lock() at all here? Why is dax
> > special in needing this when no other subsystem requires it?
> >
> > >
> > > Cc: Joao Martins <joao.m.martins@oracle.com>
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > ---
> > > drivers/dax/bus.c | 143 ++++++++++++++++++++++--------------------------------
> > > 1 file changed, 59 insertions(+), 84 deletions(-)
> > >
> > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > > index 1ff1ab5fa105..6226de131d17 100644
> > > --- a/drivers/dax/bus.c
> > > +++ b/drivers/dax/bus.c
> > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > struct dax_region *dax_region = dev_get_drvdata(dev);
> > > - unsigned long long size;
> > >
> > > - device_lock(dev);
> > > - size = dax_region_avail_size(dax_region);
> > > - device_unlock(dev);
> > > + guard(device)(dev);
> >
> > You have a valid device here, why are you locking it? How can it go
> > away? And if it can, shouldn't you have a local lock for it, and not
> > abuse the driver core lock?
>
> Yes, this is a driver-core lock abuse written by someone who should have
> known better. And yes, a local lock to protect the dax_region resource
> tree should replace this. A new rwsem to synchronize all list walks
> seems appropriate.
I see why _a_ lock is needed both here and in size_show() - the size
calculations do a walk over discontiguous ranges, and we don't want the
device to get reconfigured in the middle of that. A different local
lock seems reasonable - however can that go as a separate cleanup that
stands on its own?
For this series, I'll add a cleanup to replace the sprintfs with
sysfs_emit().
On Fri, Dec 15, 2023 at 05:32:50PM +0000, Verma, Vishal L wrote:
> On Fri, 2023-12-15 at 09:15 -0800, Dan Williams wrote:
> > Greg Kroah-Hartman wrote:
> > > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > > > Use the 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.
> > >
> > > Wait, why are you needing to call device_lock() at all here? Why is dax
> > > special in needing this when no other subsystem requires it?
> > >
> > > >
> > > > Cc: Joao Martins <joao.m.martins@oracle.com>
> > > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > > ---
> > > > drivers/dax/bus.c | 143 ++++++++++++++++++++++--------------------------------
> > > > 1 file changed, 59 insertions(+), 84 deletions(-)
> > > >
> > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> > > > index 1ff1ab5fa105..6226de131d17 100644
> > > > --- a/drivers/dax/bus.c
> > > > +++ b/drivers/dax/bus.c
> > > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > > > struct device_attribute *attr, char *buf)
> > > > {
> > > > struct dax_region *dax_region = dev_get_drvdata(dev);
> > > > - unsigned long long size;
> > > >
> > > > - device_lock(dev);
> > > > - size = dax_region_avail_size(dax_region);
> > > > - device_unlock(dev);
> > > > + guard(device)(dev);
> > >
> > > You have a valid device here, why are you locking it? How can it go
> > > away? And if it can, shouldn't you have a local lock for it, and not
> > > abuse the driver core lock?
> >
> > Yes, this is a driver-core lock abuse written by someone who should have
> > known better. And yes, a local lock to protect the dax_region resource
> > tree should replace this. A new rwsem to synchronize all list walks
> > seems appropriate.
>
> I see why _a_ lock is needed both here and in size_show() - the size
> calculations do a walk over discontiguous ranges, and we don't want the
> device to get reconfigured in the middle of that. A different local
> lock seems reasonable - however can that go as a separate cleanup that
> stands on its own?
Sure, but do not add a conversion to use guard(device) here, as that
will be pointless as you will just use a real lock instead.
> For this series, I'll add a cleanup to replace the sprintfs with
> sysfs_emit().
Why not have that be the first patch in the series? Then add your local
lock and convert everything to use it instead of the device lock?
thanks,
greg k-h
On Thu, 14 Dec 2023 22:25:27 -0700
Vishal Verma <vishal.l.verma@intel.com> wrote:
> Use the 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>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Hi Vishal,
A few really minor suggestions inline if you happen to be doing a v7.
Either way
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> @@ -481,12 +466,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);
guard(device)(&dev_dax->dev); /* Only one user now */
> + return __free_dev_dax_id(dev_dax);
> }
>
> static int alloc_dev_dax_id(struct dev_dax *dev_dax)
> @@ -908,9 +890,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);
Might as well make this
guard(device)(dev);
return sprintf(buf, "%llu\n", dev_dax_size(to_dev_dax(dev));
> }
@@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct dax_region *dax_region = dev_get_drvdata(dev);
- unsigned long long size;
- device_lock(dev);
- size = dax_region_avail_size(dax_region);
- device_unlock(dev);
+ guard(device)(dev);
- return sprintf(buf, "%llu\n", size);
+ return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
}
static DEVICE_ATTR_RO(available_size);
@@ -309,17 +306,14 @@ static ssize_t seed_show(struct device *dev,
{
struct dax_region *dax_region = dev_get_drvdata(dev);
struct device *seed;
- ssize_t rc;
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;
+ return sprintf(buf, "%s\n", seed ? dev_name(seed) : "");
}
static DEVICE_ATTR_RO(seed);
@@ -328,24 +322,28 @@ static ssize_t create_show(struct device *dev,
{
struct dax_region *dax_region = dev_get_drvdata(dev);
struct device *youngest;
- ssize_t rc;
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;
+ return sprintf(buf, "%s\n", youngest ? dev_name(youngest) : "");
}
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 +356,25 @@ 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 +466,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 +890,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,17 +1061,16 @@ 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);
+ if (rc)
+ return rc;
- return rc == 0 ? len : rc;
+ return len;
}
static DEVICE_ATTR_RW(size);
@@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr,
if (rc)
return rc;
- rc = -ENXIO;
- device_lock(dax_region->dev);
- if (!dax_region->dev->driver) {
- device_unlock(dax_region->dev);
- return rc;
- }
- device_lock(dev);
+ guard(device)(dax_region->dev);
+ if (!dax_region->dev->driver)
+ return -ENXIO;
+ 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);
+ if (!alloc_is_aligned(dev_dax, to_alloc))
+ return -ENXIO;
- return rc == 0 ? len : rc;
+ rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
+ if (rc)
+ return rc;
+
+ return len;
}
static DEVICE_ATTR_WO(mapping);
@@ -1196,27 +1175,23 @@ 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)
+ if (rc) {
dev_dax->align = align_save;
-out_unlock:
- device_unlock(dev);
- device_unlock(dax_region->dev);
- return rc == 0 ? len : rc;
+ return rc;
+ }
+
+ return len;
}
static DEVICE_ATTR_RW(align);