[PATCH net-next] auxiliary: Implement refcounting

[PATCH net-next] auxiliary: Implement refcounting

[Temerkhanov, Sergey]

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.

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;
+
+	return refcount_inc_not_zero(&adev->refcnt);
+}
+
+static inline void auxiliary_device_put(struct auxiliary_device *adev)
+{
+	if (!adev)
+		return;
+
+	refcount_dec(&adev->refcnt);
+
+	wake_up_interruptible(&adev->wq_head);
+}
+
 static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
 {
 	return container_of(dev, struct auxiliary_device, dev);
@@ -212,15 +235,8 @@ int auxiliary_device_init(struct auxiliary_device *auxdev);
 int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname);
 #define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev, KBUILD_MODNAME)
 
-static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
-{
-	put_device(&auxdev->dev);
-}
-
-static inline void auxiliary_device_delete(struct auxiliary_device *auxdev)
-{
-	device_del(&auxdev->dev);
-}
+void auxiliary_device_uninit(struct auxiliary_device *auxdev);
+void auxiliary_device_delete(struct auxiliary_device *auxdev);
 
 int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner,
 				const char *modname);
-- 
2.35.3

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

[Leon Romanovsky]

+ 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

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

[Greg Kroah-Hartman]

On Thu, Feb 16, 2023 at 02:42:41PM +0200, Leon Romanovsky wrote:
> + 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));

There are 3 things wrong with this single line of code, pretty
impressive!

First off, never use WARN_ON unless you want to reboot people's
machines.  Handle the issue if it can really happen and keep on moving.
Don't just throw up your hands and yell at userspace and stop the box
(remember panic-on-warn is real.)

Second, if this is a real problem, HANDLE IT!  Don't just free memory
as obviously someone actually has the memory in use!

Third, NEVER read the reference count and do something based on it.
That's not how reference counts work at all, unless you are using the
correct reference count function (hint, this is not it.)

> > +
> > +	device_del(&auxdev->dev);

No, no no no no .

I see why you didn't cc me on this thread to start with, you were trying
to get this by without review, that's not ok.

You now have the required penance of having to get an internal-Intel
review and signed-off-by FIRST before sending this patch series out
again.

greg k-h

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

[Greg Kroah-Hartman]

Oh, one more fun thing:

On Thu, Feb 16, 2023 at 02:42:41PM +0200, Leon Romanovsky wrote:
> On Thu, Feb 16, 2023 at 01:16:21PM +0100, Temerkhanov, Sergey wrote:
> >  struct auxiliary_device {
> >  	struct device dev;
> > +	refcount_t refcnt;
> >  	const char *name;
> > +	struct wait_queue_head wq_head;
> >  	u32 id;

Sergey, you are adding an additional reference count to a structure that
is already properly reference counted!  So you are imposing 2 different
counts and lifecycles on a single tiny structure?  How is that even
supposed to work?

Again, please get reviews from internal Intel developers FIRST as is
required by all Intel kernel developers touching code like this.  You
know this, it is part of your internal development rules, please do not
try to ignore them, that's a sure way to get everyone to just ignore
your changes going forward.

thanks,

greg k-h