| Message ID | 1395143914-26929-1-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote: > wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL); > if (!wval) { > - ret = -ENOMEM; > dev_err(map->dev, "Error in memory allocation\n"); > - goto out; > + return -ENOMEM; > } > + map->lock(map->lock_arg); > for (i = 0; i < val_count * val_bytes; i += val_bytes) > map->format.parse_inplace(wval + i); > > ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); > + map->unlock(map->lock_arg); If we're reducing the locking region here then we should take the lock after doing the parse_inplace() to reduce the locked region. Nothing else can be referring to the data since we only just allocated it. I'll fix that by hand and apply. Please also send things to the list for the subsystem (linux-kernel if there's not a specific one).
At Tue, 18 Mar 2014 12:22:18 +0000, Mark Brown wrote: > > On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote: > > > wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL); > > if (!wval) { > > - ret = -ENOMEM; > > dev_err(map->dev, "Error in memory allocation\n"); > > - goto out; > > + return -ENOMEM; > > } > > + map->lock(map->lock_arg); > > for (i = 0; i < val_count * val_bytes; i += val_bytes) > > map->format.parse_inplace(wval + i); > > > > ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); > > + map->unlock(map->lock_arg); > > If we're reducing the locking region here then we should take the lock > after doing the parse_inplace() to reduce the locked region. Nothing > else can be referring to the data since we only just allocated it. I'll > fix that by hand and apply. I thought of that, too, but didn't take it because covering the lock there doesn't change the fact that it's still fundamentally racy. > Please also send things to the list for the subsystem (linux-kernel if > there's not a specific one). OK, I just copied the previous recipient of the thread... Takashi
On Tue, Mar 18, 2014 at 01:57:22PM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote: > > > + map->lock(map->lock_arg); > > > for (i = 0; i < val_count * val_bytes; i += val_bytes) > > > map->format.parse_inplace(wval + i); > > > > > > ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); > > > + map->unlock(map->lock_arg); > > If we're reducing the locking region here then we should take the lock > > after doing the parse_inplace() to reduce the locked region. Nothing > > else can be referring to the data since we only just allocated it. I'll > > fix that by hand and apply. > I thought of that, too, but didn't take it because covering the lock > there doesn't change the fact that it's still fundamentally racy. I'm not sure what you mean here - what do you mean yb "covering the lock"? > > Please also send things to the list for the subsystem (linux-kernel if > > there's not a specific one). > OK, I just copied the previous recipient of the thread... Sure, but if there's another subsystem adding that subsystem helps things along.
At Tue, 18 Mar 2014 13:04:26 +0000, Mark Brown wrote: > > On Tue, Mar 18, 2014 at 01:57:22PM +0100, Takashi Iwai wrote: > > Mark Brown wrote: > > > On Tue, Mar 18, 2014 at 12:58:33PM +0100, Takashi Iwai wrote: > > > > > + map->lock(map->lock_arg); > > > > for (i = 0; i < val_count * val_bytes; i += val_bytes) > > > > map->format.parse_inplace(wval + i); > > > > > > > > ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); > > > > + map->unlock(map->lock_arg); > > > > If we're reducing the locking region here then we should take the lock > > > after doing the parse_inplace() to reduce the locked region. Nothing > > > else can be referring to the data since we only just allocated it. I'll > > > fix that by hand and apply. > > > I thought of that, too, but didn't take it because covering the lock > > there doesn't change the fact that it's still fundamentally racy. > > I'm not sure what you mean here - what do you mean yb "covering the > lock"? I meant covering memcpy() and parse_inplace() & co in the lock. Takashi
On Tue, Mar 18, 2014 at 02:36:53PM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > > I thought of that, too, but didn't take it because covering the lock > > > there doesn't change the fact that it's still fundamentally racy. > > I'm not sure what you mean here - what do you mean yb "covering the > > lock"? > I meant covering memcpy() and parse_inplace() & co in the lock. Oh, right. A fix is definitely needed and your fix is certainly good from a correctness point of view but since we're narrowing the locked region we may as well make it as small as possible while we're at it both for comprehensibility ("why is that locked?") and performance.
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 6a19515f8a45..6d134a3cbfd3 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1520,12 +1520,12 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, if (reg % map->reg_stride) return -EINVAL; - map->lock(map->lock_arg); /* * Some devices don't support bulk write, for * them we have a series of single write operations. */ if (!map->bus || map->use_single_rw) { + map->lock(map->lock_arg); for (i = 0; i < val_count; i++) { unsigned int ival; @@ -1554,24 +1554,25 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, if (ret != 0) goto out; } +out: + map->unlock(map->lock_arg); } else { void *wval; wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL); if (!wval) { - ret = -ENOMEM; dev_err(map->dev, "Error in memory allocation\n"); - goto out; + return -ENOMEM; } + map->lock(map->lock_arg); for (i = 0; i < val_count * val_bytes; i += val_bytes) map->format.parse_inplace(wval + i); ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); + map->unlock(map->lock_arg); kfree(wval); } -out: - map->unlock(map->lock_arg); return ret; } EXPORT_SYMBOL_GPL(regmap_bulk_write);
regmap deploys the spinlock for the protection when set up in fast_io mode. This may lead to sleep-in-atomic by memory allocation with GFP_KERNEL in regmap_bulk_write(). This patch fixes it by moving the allocation out of the lock. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/base/regmap/regmap.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)