| Message ID | 20230216121621.37063-1-sergey.temerkhanov@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] auxiliary: Implement refcounting | expand |
+ 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
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
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
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);