Messages in this thread | | | Subject | Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init | From | Hans de Goede <> | Date | Mon, 15 Feb 2021 11:37:26 +0100 |
| |
Hi,
On 2/15/21 8:22 AM, Vaittinen, Matti wrote: > > On Sat, 2021-02-13 at 16:59 +0100, Hans de Goede wrote: >> Hi, >> >> On 2/13/21 4:27 PM, Guenter Roeck wrote: >>> On 2/13/21 7:03 AM, Hans de Goede wrote: >>> [ ... ] >>>> 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); >>> >>> Unfortunately, you can not type cast function pointers in C. It is >>> against the C ABI. >>> I am sure it is done in a few places in the kernel anyway, but >>> those are wrong. >> >> I see, bummer. > > I think using devm_add_action() is still a good idea.
Yes, we could also just have a 1 line static inline function to do the function-cast. Like this:
static inline void devm_delayed_work_autocancel_func(void *work) { cancel_delayed_work_sync(work); }
static inline 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_delayed_work_autocancel_func, w); }
Both functions will then simply be compiled out in files which do not use them.
>> If we add a devm_clk_prepare_enable() helper that should probably be >> added >> to drivers/clk/clk-devres.c and not to drivers/base/devres.c . >> >> I also still wonder if we cannot find a better place for this new >> devm_delayed_work_autocancel() helper but nothing comes to mind. > > I don't like the idea of including device.h from workqueue.h - and I > think this would be necessary if we added > devm_delayed_work_autocancel() as inline in workqueue.h, right?
Yes.
> I also see strong objection towards the devm managed clean-ups.
Yes it seems that there are some people who don't like this, where as others do like them.
> How about adding some devm-helpers.c in drivers/base - where we could > collect devm-based helpers - and which could be enabled by own CONFIG - > and left out by those who dislike it?
I would make this something configurable through Kconfig, but if go the static inline route, which I'm in favor of then we could just have a:
include/linux/devm-cleanup-helpers.h
And put everything (including kdoc texts) there.
This way the functionality is 100% opt-in (by explicitly including the header if you want the helpers) which hopefully makes this a bit more acceptable to people who don't like this style of cleanups.
I would be even happy to act as the upstream maintainer for such a include/linux/devm-cleanup-helpers.h file, I can maintain it as part of the platform-drivers-x86 tree (with its own MAINTAINERS entry).
Greg, would this be an acceptable solution to you ?
Regards,
Hans
| |