Re: [RFC PATCH 03/11] drivers/base: convert deferred_trigger_count and probe_count to counter_atomic

[Greg KH]

On Tue, Sep 22, 2020 at 07:43:32PM -0600, Shuah Khan wrote:
> counter_atomic is introduced to be used when a variable is used as
> a simple counter and doesn't guard object lifetimes. This clearly
> differentiates atomic_t usages that guard object lifetimes.
> 
> counter_atomic variables will wrap around to 0 when it overflows and
> should not be used to guard resource lifetimes, device usage and
> open counts that control state changes, and pm states.
> 
> deferred_trigger_count gets incremented and read. It doesn't guard
> object lifetimes, device usage counts, device open counts, and pm
> states. There is very little chance of this counter overflowing.
> Convert it to use counter_atomic.
> 
> This conversion doesn't change the oveflow wrap around behavior.
> 
> probe_count gets incremented, decremented, read. It doesn't guard
> object lifetimes, device usage counts, device open counts, and pm
> states. There is very little chance of this counter overflowing.
> This counter controls the wait for known devices to complete their
> probing, and probe_count == 0 ends the wait. Other than that it
> meets the other criteria to be converted. Convert it to use
> counter_atomic.
> 
> Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/base/dd.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 857b0a928e8d..f49fe45960ac 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
>  #include <linux/slab.h>
> +#include <linux/counters.h>
>  
>  #include "base.h"
>  #include "power/power.h"
> @@ -54,7 +55,7 @@
>  static DEFINE_MUTEX(deferred_probe_mutex);
>  static LIST_HEAD(deferred_probe_pending_list);
>  static LIST_HEAD(deferred_probe_active_list);
> -static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
> +static struct counter_atomic deferred_trigger_count = COUNTER_ATOMIC_INIT(0);
>  static struct dentry *deferred_devices;
>  static bool initcalls_done;
>  
> @@ -173,7 +174,7 @@ static void driver_deferred_probe_trigger(void)
>  	 * into the active list so they can be retried by the workqueue
>  	 */
>  	mutex_lock(&deferred_probe_mutex);
> -	atomic_inc(&deferred_trigger_count);
> +	counter_atomic_inc(&deferred_trigger_count);
>  	list_splice_tail_init(&deferred_probe_pending_list,
>  			      &deferred_probe_active_list);
>  	mutex_unlock(&deferred_probe_mutex);
> @@ -466,7 +467,7 @@ int device_bind_driver(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_bind_driver);
>  
> -static atomic_t probe_count = ATOMIC_INIT(0);
> +static struct counter_atomic probe_count = COUNTER_ATOMIC_INIT(0);
>  static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>  
>  static void driver_deferred_probe_add_trigger(struct device *dev,
> @@ -474,7 +475,7 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
>  {
>  	driver_deferred_probe_add(dev);
>  	/* Did a trigger occur while probing? Need to re-trigger if yes */
> -	if (local_trigger_count != atomic_read(&deferred_trigger_count))
> +	if (local_trigger_count != counter_atomic_read(&deferred_trigger_count))
>  		driver_deferred_probe_trigger();
>  }
>  
> @@ -493,7 +494,7 @@ static DEVICE_ATTR_RO(state_synced);
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = -EPROBE_DEFER;
> -	int local_trigger_count = atomic_read(&deferred_trigger_count);
> +	int local_trigger_count = counter_atomic_read(&deferred_trigger_count);
>  	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
>  			   !drv->suppress_bind_attrs;
>  
> @@ -514,7 +515,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		return ret;
>  
> -	atomic_inc(&probe_count);
> +	counter_atomic_inc(&probe_count);
>  	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
>  		 drv->bus->name, __func__, drv->name, dev_name(dev));
>  	if (!list_empty(&dev->devres_head)) {
> @@ -648,7 +649,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	 */
>  	ret = 0;
>  done:
> -	atomic_dec(&probe_count);
> +	counter_atomic_dec(&probe_count);
>  	wake_up_all(&probe_waitqueue);
>  	return ret;
>  }
> @@ -678,7 +679,7 @@ static int really_probe_debug(struct device *dev, struct device_driver *drv)
>   */
>  int driver_probe_done(void)
>  {
> -	int local_probe_count = atomic_read(&probe_count);
> +	int local_probe_count = counter_atomic_read(&probe_count);
>  
>  	pr_debug("%s: probe_count = %d\n", __func__, local_probe_count);
>  	if (local_probe_count)
> @@ -699,7 +700,7 @@ void wait_for_device_probe(void)
>  	flush_work(&deferred_probe_work);
>  
>  	/* wait for the known devices to complete their probing */
> -	wait_event(probe_waitqueue, atomic_read(&probe_count) == 0);
> +	wait_event(probe_waitqueue, counter_atomic_read(&probe_count) == 0);
>  	async_synchronize_full();
>  }
>  EXPORT_SYMBOL_GPL(wait_for_device_probe);
> -- 
> 2.25.1
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>