| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 13 Feb 2021 16:03:50 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
mazziesaccount@...il.com
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>,
Mark Gross <mgross@...ux.intel.com>,
Sebastian Reichel <sre@...nel.org>,
Chen-Yu Tsai <wens@...e.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Saravana Kannan <saravanak@...gle.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Joerg Roedel <jroedel@...e.de>,
Dan Williams <dan.j.williams@...el.com>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-hwmon@...r.kernel.org, platform-driver-x86@...r.kernel.org,
linux-pm@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of
delayed work init
Hi,
On 2/13/21 12:58 PM, Matti Vaittinen wrote:
> A few drivers which need a delayed work-queue must cancel work at exit.
> Some of those implement remove solely for this purpose. Help drivers
> to avoid unnecessary remove and error-branch implementation by adding
> managed verision of delayed work initialization
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> ---
> drivers/base/devres.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/device.h | 5 +++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index fb9d5289a620..2879595bb5a4 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -1231,3 +1231,36 @@ void devm_free_percpu(struct device *dev, void __percpu *pdata)
> (void *)pdata));
> }
> EXPORT_SYMBOL_GPL(devm_free_percpu);
> +
> +static void dev_delayed_work_drop(struct device *dev, void *res)
> +{
> + cancel_delayed_work_sync(*(struct delayed_work **)res);
> +}
> +
> +/**
> + * devm_delayed_work_autocancel - Resource-managed work allocation
> + * @dev: Device which lifetime work is bound to
> + * @pdata: work to be cancelled when device exits
> + *
> + * Initialize work which is automatically cancelled when device exits.
> + * A few drivers need delayed work which must be cancelled before driver
> + * is unload to avoid accessing removed resources.
> + * devm_delayed_work_autocancel() can be used to omit the explicit
> + * cancelleation when driver is unload.
> + */
> +int devm_delayed_work_autocancel(struct device *dev, struct delayed_work *w,
> + void (*worker)(struct work_struct *work))
> +{
> + struct delayed_work **ptr;
> +
> + ptr = devres_alloc(dev_delayed_work_drop, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(w, worker);
> + *ptr = w;
> + devres_add(dev, ptr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_delayed_work_autocancel);
This is a bit of a micro-optimization I must admit, but I think you can
just make this a static inline using devm_add_action, which would avoid
growing the base kernel image and avoid adding yet another symbol to
the exported symbols table.
I think something like this should work:
static int devm_delayed_work_autocancel(struct device *dev, struct delayed_work *w,
void (*worker)(struct work_struct *work)) {
INIT_DELAYED_WORK(w, worker);
return devm_add_action(dev, (void (*action)(void *))cancel_delayed_work_sync, w);
}
I'm not sure about the cast, that may need something like this instead:
typedef void (*devm_action_func)(void *);
static int devm_delayed_work_autocancel(struct device *dev, struct delayed_work *w,
void (*worker)(struct work_struct *work)) {
INIT_DELAYED_WORK(w, worker);
return devm_add_action(dev, (devm_action_func)cancel_delayed_work_sync, w);
}
As said this is a bit of a micro-optimization. But if we want to add others too,
say one for non delayed works, then at some point in time this is going to start
to add up (a bit) wrt symbol-table size and base kernel-image size.
And if you go the static inline route, I guess you could add this in
include/linux/workqueue.h
instead and putting workqueue devm cleanup helpers there seems better
then putting random devm cleanup helpers in include/linux/device.h .
Regards,
Hans
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1779f90eeb4c..192456198de7 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -27,6 +27,7 @@
> #include <linux/uidgid.h>
> #include <linux/gfp.h>
> #include <linux/overflow.h>
> +#include <linux/workqueue.h>
> #include <linux/device/bus.h>
> #include <linux/device/class.h>
> #include <linux/device/driver.h>
> @@ -249,6 +250,10 @@ void __iomem *devm_of_iomap(struct device *dev,
> struct device_node *node, int index,
> resource_size_t *size);
>
> +/* delayed work which is cancelled when driver exits */
> +int devm_delayed_work_autocancel(struct device *dev, struct delayed_work *w,
> + void (*worker)(struct work_struct *work));
> +
> /* allows to add/remove a custom action to devres stack */
> int devm_add_action(struct device *dev, void (*action)(void *), void *data);
> void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
>
Powered by blists - more mailing lists