| Message ID | 1395141600-24558-1-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Mar 18, 2014 at 12:20:00PM +0100, Takashi Iwai wrote: > regmap deploys the spinlock for the protection when set up in fast_io > mode. But some places are still left with possible sleep in the lock, > typically allocating memory with GFP_KERNEL. This patch fixes these > calls by moving the allocations out of the lock. You should have split these two unrelated changes into two patches. I already have a fix for the cache sync in my tree locally which I will test just a soon as I fix whatever broke the -next build for me.
At Tue, 18 Mar 2014 11:27:56 +0000, Mark Brown wrote: > > On Tue, Mar 18, 2014 at 12:20:00PM +0100, Takashi Iwai wrote: > > regmap deploys the spinlock for the protection when set up in fast_io > > mode. But some places are still left with possible sleep in the lock, > > typically allocating memory with GFP_KERNEL. This patch fixes these > > calls by moving the allocations out of the lock. > > You should have split these two unrelated changes into two patches. You're suggesting to split the patch to fix for regmap_bulk_write() and regmap_register_patch()? I don't mind to split, but basically both are the very same bug, and the solution is very same, too. > I already have a fix for the cache sync in my tree locally which I will > test just a soon as I fix whatever broke the -next build for me. I don't know why you're talking about the build fix... Takashi
On Tue, Mar 18, 2014 at 12:38:42PM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > On Tue, Mar 18, 2014 at 12:20:00PM +0100, Takashi Iwai wrote: > > > regmap deploys the spinlock for the protection when set up in fast_io > > > mode. But some places are still left with possible sleep in the lock, > > > typically allocating memory with GFP_KERNEL. This patch fixes these > > > calls by moving the allocations out of the lock. > > You should have split these two unrelated changes into two patches. > You're suggesting to split the patch to fix for regmap_bulk_write() > and regmap_register_patch()? I don't mind to split, but basically > both are the very same bug, and the solution is very same, too. Yes (especially given that the considerations for patch are a bit different). > > I already have a fix for the cache sync in my tree locally which I will > > test just a soon as I fix whatever broke the -next build for me. > I don't know why you're talking about the build fix... I can't exactly test my code if the underlying kernel won't even compile.
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 6a19515f8a45..e6827cee29a3 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); @@ -2173,6 +2174,10 @@ EXPORT_SYMBOL_GPL(regmap_async_complete); * apply them immediately. Typically this is used to apply * corrections to be applied to the device defaults on startup, such * as the updates some vendors provide to undocumented registers. + * + * Note that the function gives no protection over concurrent access + * to map->patch and map->patch_regs. If needed, apply some lock in + * the caller side. */ int regmap_register_patch(struct regmap *map, const struct reg_default *regs, int num_regs) @@ -2185,6 +2190,16 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs, num_regs)) return 0; + p = krealloc(map->patch, + sizeof(struct reg_default) * (map->patch_regs + num_regs), + GFP_KERNEL); + if (!p) + return -ENOMEM; + + memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs)); + map->patch = p; + map->patch_regs += num_regs; + map->lock(map->lock_arg); bypass = map->cache_bypass; @@ -2202,17 +2217,6 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs, } } - p = krealloc(map->patch, - sizeof(struct reg_default) * (map->patch_regs + num_regs), - GFP_KERNEL); - if (p) { - memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs)); - map->patch = p; - map->patch_regs += num_regs; - } else { - ret = -ENOMEM; - } - out: map->async = false; map->cache_bypass = bypass;
regmap deploys the spinlock for the protection when set up in fast_io mode. But some places are still left with possible sleep in the lock, typically allocating memory with GFP_KERNEL. This patch fixes these calls by moving the allocations out of the lock. The krealloc() call in regmap_register_patch() makes it unsafe for concurrent accesses, however. But such a use case must be very rare, thus we take rather simplicity over complexity, and add a note in the function description mentioning this restriction. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- Only compile tested. drivers/base/regmap/regmap.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-)