Re: [RFC PATCH 0/7] Add managed version of delayed work init

From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
To: mgross@linux.intel.com
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Hans de Goede <hdegoede@redhat.com>,
	Sebastian Reichel <sre@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Saravana Kannan <saravanak@google.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Joerg Roedel <jroedel@suse.de>,
	Dan Williams <dan.j.williams@intel.com>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-hwmon@vger.kernel.org, platform-driver-x86@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-watchdog@vger.kernel.org
Subject: Re: [RFC PATCH 0/7] Add managed version of delayed work init
Date: Fri, 19 Feb 2021 12:35:51 +0200	[thread overview]
Message-ID: <468a5c7820a510b8a12c10b1b8a107fb41e9d26c.camel@fi.rohmeurope.com> (raw)
In-Reply-To: <20210218162821.GP154917@linux.intel.com>

Hello Mark,

Thanks for taking a look at the series! This is the first time anyone
has been commenting on a cover-letter which is likely to fade away and
never be looked at again. Guess you are a thorough person :)

On Thu, 2021-02-18 at 08:28 -0800, mark gross wrote:
> On Sat, Feb 13, 2021 at 01:58:17PM +0200, Matti Vaittinen wrote:
> > It's not rare that device drivers need delayed work.
> > It's not rare that this work needs driver's data.
> > 
> > Often this means that driver must ensure the work is not queued
> > when
> > driver exits. Usually this is done by ensuring new work is not
> > added and
> > then calling cancel_delayed_work_sync() at remove(). In many cases
> > this
> > may also require cleanup at probe error path - which is easy to
> > forget.
> > 
> > It might be helpful for (a) few drivers if there was a work init
>  why the (a) and not just a?

I am not sure how many drivers are needed to change it from 'few' to 'a
few'. Additionally, this series converted only the drivers which I
found could easily get rid of the .remove() - I did not analyze how
many drivers would benefit from this by getting rid of mixed
devm/manual resource management.

So to sum up - I don't know how many drivers will benefit and what
people think makes 'few' to turn to 'a few'. '(a) few' leaves this
decision to readers - and (a) few of them know the drivers better than
I do.

> > Main reson why this is RFC is that I had hard time deciding where
> > this
> > function should be introduced. It's not nice to include all device
> > stuff
> > in workqueue - because many workqueue users are not interested in
> > devices. In same way, not all of the devices are interested in WQs.
> > OTOH, adding own file just for this sounds like an overkill.
> s/own/one

Hm. The 'own file for XXX' does not make sense for native English
speakers? Didn't now that. Thanks for pointing it out.

I will edit the cover letter when I respin this rebased on v5.12-rc1 -
and it is likely the series v2 will add this function inlined in a new
header dedicated for devm-helpers (as was suggested by Hans de Goede).

Best Regards
--Matti