Re: [PATCH v1] regmap: Add can_sleep configuration option

[Marek Szyprowski]

Hi Dmitry,

On 02.09.2020 16:18, Dmitry Osipenko wrote:
> Regmap can't sleep if spinlock is used for the locking protection.
> This patch fixes regression caused by a previous commit that switched
> regmap to use fsleep() and this broke Amlogic S922X platform.
>
> This patch adds new configuration option for regmap users, allowing to
> specify whether regmap operations can sleep and assuming that sleep is
> allowed if mutex is used for the regmap locking protection.
>
> Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Fixes: 2b32d2f7ce0a ("regmap: Use flexible sleep")
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>

This fixes the issue I've reported. Thanks!

Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>

> ---
>   drivers/base/regmap/internal.h |  3 +++
>   drivers/base/regmap/regmap.c   | 19 +++++++++++++++----
>   include/linux/regmap.h         |  3 +++
>   3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 3d80c4b43f72..8a59359e145f 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -161,6 +161,9 @@ struct regmap {
>   	void *selector_work_buf;	/* Scratch buffer used for selector */
>   
>   	struct hwspinlock *hwlock;
> +
> +	/* if set, the regmap core can sleep */
> +	bool can_sleep;
>   };
>   
>   struct regcache_ops {
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index a417cb1a11dc..2807e544658e 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -697,11 +697,13 @@ struct regmap *__regmap_init(struct device *dev,
>   
>   	if (config->disable_locking) {
>   		map->lock = map->unlock = regmap_lock_unlock_none;
> +		map->can_sleep = config->can_sleep;
>   		regmap_debugfs_disable(map);
>   	} else if (config->lock && config->unlock) {
>   		map->lock = config->lock;
>   		map->unlock = config->unlock;
>   		map->lock_arg = config->lock_arg;
> +		map->can_sleep = config->can_sleep;
>   	} else if (config->use_hwlock) {
>   		map->hwlock = hwspin_lock_request_specific(config->hwlock_id);
>   		if (!map->hwlock) {
> @@ -737,6 +739,7 @@ struct regmap *__regmap_init(struct device *dev,
>   			mutex_init(&map->mutex);
>   			map->lock = regmap_lock_mutex;
>   			map->unlock = regmap_unlock_mutex;
> +			map->can_sleep = true;
>   			lockdep_set_class_and_name(&map->mutex,
>   						   lock_key, lock_name);
>   		}
> @@ -2230,8 +2233,12 @@ static int _regmap_range_multi_paged_reg_write(struct regmap *map,
>   				if (ret != 0)
>   					return ret;
>   
> -				if (regs[i].delay_us)
> -					fsleep(regs[i].delay_us);
> +				if (regs[i].delay_us) {
> +					if (map->can_sleep)
> +						fsleep(regs[i].delay_us);
> +					else
> +						udelay(regs[i].delay_us);
> +				}
>   
>   				base += n;
>   				n = 0;
> @@ -2267,8 +2274,12 @@ static int _regmap_multi_reg_write(struct regmap *map,
>   			if (ret != 0)
>   				return ret;
>   
> -			if (regs[i].delay_us)
> -				fsleep(regs[i].delay_us);
> +			if (regs[i].delay_us) {
> +				if (map->can_sleep)
> +					fsleep(regs[i].delay_us);
> +				else
> +					udelay(regs[i].delay_us);
> +			}
>   		}
>   		return 0;
>   	}
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index d865d8fea535..0c49d59168b5 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -342,6 +342,7 @@ typedef void (*regmap_unlock)(void *);
>    * @hwlock_id: Specify the hardware spinlock id.
>    * @hwlock_mode: The hardware spinlock mode, should be HWLOCK_IRQSTATE,
>    *		 HWLOCK_IRQ or 0.
> + * @can_sleep: Optional, specifies whether regmap operations can sleep.
>    */
>   struct regmap_config {
>   	const char *name;
> @@ -398,6 +399,8 @@ struct regmap_config {
>   	bool use_hwlock;
>   	unsigned int hwlock_id;
>   	unsigned int hwlock_mode;
> +
> +	bool can_sleep;
>   };
>   
>   /**

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland