Re: [PATCH net-next] auxiliary: Implement refcounting

[Leon Romanovsky <leon@kernel.org>]

+ Greg KH

On Thu, Feb 16, 2023 at 01:16:21PM +0100, Temerkhanov, Sergey wrote:
> From: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> 
> Implement reference counting to make it possible to synchronize
> deinitialization and removal of interfaces published via aux bus
> with the client modules.
> Reference counting can be used in both sleeping and non-sleeping
> contexts so this approach is intended to replace device_lock()
> (mutex acquisition) with an additional lock on top of it
> which is not always possible to take in client code.

I want to see this patch as part of your client code series.
It is unclear why same aux device is used from different drivers.

Otherwise, this whole refcnt is useless and just hides bugs in driver
which accesses released devices.

> 
> Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> ---
>  drivers/base/auxiliary.c      | 18 ++++++++++++++++++
>  include/linux/auxiliary_bus.h | 34 +++++++++++++++++++++++++---------
>  2 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> index 8c5e65930617..082b3ebd143d 100644
> --- a/drivers/base/auxiliary.c
> +++ b/drivers/base/auxiliary.c
> @@ -287,10 +287,28 @@ int auxiliary_device_init(struct auxiliary_device *auxdev)
>  
>  	dev->bus = &auxiliary_bus_type;
>  	device_initialize(&auxdev->dev);
> +	init_waitqueue_head(&auxdev->wq_head);
> +	refcount_set(&auxdev->refcnt, 1);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(auxiliary_device_init);
>  
> +void auxiliary_device_uninit(struct auxiliary_device *auxdev)
> +{
> +	wait_event_interruptible(auxdev->wq_head,
> +				 refcount_dec_if_one(&auxdev->refcnt));
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_uninit);
> +
> +void auxiliary_device_delete(struct auxiliary_device *auxdev)
> +{
> +	WARN_ON(refcount_read(&auxdev->refcnt));
> +
> +	device_del(&auxdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_delete);
> +
>  /**
>   * __auxiliary_device_add - add an auxiliary bus device
>   * @auxdev: auxiliary bus device to add to the bus
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index de21d9d24a95..0610ccee320e 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -10,6 +10,8 @@
>  
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/wait.h>
> +#include <linux/refcount.h>
>  
>  /**
>   * DOC: DEVICE_LIFESPAN
> @@ -137,7 +139,9 @@
>   */
>  struct auxiliary_device {
>  	struct device dev;
> +	refcount_t refcnt;
>  	const char *name;
> +	struct wait_queue_head wq_head;
>  	u32 id;
>  };
>  
> @@ -198,6 +202,25 @@ static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *
>  	dev_set_drvdata(&auxdev->dev, data);
>  }
>  
> +static inline bool __must_check
> +auxiliary_device_get(struct auxiliary_device *adev)
> +{
> +	if (!adev)
> +		return false;

Please don't check for validity of adev, it is caller's job.

> +
> +	return refcount_inc_not_zero(&adev->refcnt);
> +}
> +
> +static inline void auxiliary_device_put(struct auxiliary_device *adev)
> +{
> +	if (!adev)
> +		return;

Same.

Thanks