Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp4873281pxb; Mon, 15 Feb 2021 03:41:04 -0800 (PST) X-Google-Smtp-Source: ABdhPJyP2ibPGoJtnPMijo4IrXO4hF7OCk1SYCOX7rgO8gfurQjeV7A6MMUPr1SxpMEBnlfrf8TR X-Received: by 2002:a17:907:724a:: with SMTP id ds10mr14751302ejc.28.1613389264714; Mon, 15 Feb 2021 03:41:04 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1613389264; cv=none; d=google.com; s=arc-20160816; b=yDAUQKeFtP+l7gnsLjlnqmcUapFJFlDQCdwyflUeL5/0STZT6CI/tU+Qu8E3EqKJjp Yo6n/78KOEky2WaU7O2uX8kwdqkLhgr/kVwL/zLBcK54ZGAkqfRSoKCTQgbljAB+Srbo 8N7oOHvOoivCg0Crnmwf/CIsBIJ/3FJZMhLjcEZSlZamSqU4xm3sY0kZEMDgvO/JLe8A J7iQ/dHvAkV9NpENnYs2pbYrpBi1QDU25aBkwbKWaCM342lEsYWhVC+o1EFJolEG+OAS +NcgRe5Cex5IceCCMJkNvJoAazo24ckSvyk4hq3AreE4cwLiCO2Tu+lIG5cUPWaYMetR wU1g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=iCognQjLNkIdpDBDTtUZ2UHWF6+aM9Aj0YlQMCssHRM=; b=OBCdVuYcSD1kevkIuwuCxaovpsePqcCfP5fJq3p3qgspAA62PFTsF8rhVGVU4erop3 ShMBvR0UvzSyNTsol7v7o8b15ZrTYLMi0Ix8VS7qvOPtz80FynO87skY5ccaIvdQvSev 8ZT/L7GHb84EtjpfGhzh3gK3rdZRJq69vPai9RTFxm+oMCbQVXw1na4dYzWIW7DUHPHl LbxG7dWmaH2PbcYrFwJSFckTDNDTt/xR4IYr5vBcOQW3i1aO2A4y9qSWPLekXjDIpOqa hubTVMOSQb2bYDwh/R/JnLqMujdeuF1BW6BMuK1jWVft/0oXAw9V1QjTskhkXVwarH5n Zb+Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b="2mO/lP34"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id sb16si4165956ejb.46.2021.02.15.03.40.41; Mon, 15 Feb 2021 03:41:04 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b="2mO/lP34"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229652AbhBOLjZ (ORCPT + 99 others); Mon, 15 Feb 2021 06:39:25 -0500 Received: from mail.kernel.org ([198.145.29.99]:55574 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229802AbhBOLjO (ORCPT ); Mon, 15 Feb 2021 06:39:14 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id 169C464DE0; Mon, 15 Feb 2021 11:38:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1613389113; bh=jmmj+Sj/0CdSrHbS95/UtmVOin2tjnR3EAmcJtWDvMw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=2mO/lP34yJ55Ulq4sZSmsz3l1aChIoTWHld0y66VyK36nAWf6SFBD7HyBx/0HmFtr 3WPGf0rnBuGU9wj6EpgaU/xiNaTYHmLE0B81mhLF8f4sNVZRBeKtdzT1K1mrazfANZ mN1bdJ4CVBm6IP5IMcNX6DrMvbOU4K6FnTHsKAhU= Date: Mon, 15 Feb 2021 12:31:22 +0100 From: "gregkh@linuxfoundation.org" To: Hans de Goede Cc: "Vaittinen, Matti" , "linux@roeck-us.net" , "linux-pm@vger.kernel.org" , "broonie@kernel.org" , "linux-watchdog@vger.kernel.org" , "bgolaszewski@baylibre.com" , "dan.j.williams@intel.com" , "wim@linux-watchdog.org" , "sre@kernel.org" , "linux-arm-msm@vger.kernel.org" , "linux-hwmon@vger.kernel.org" , "jdelvare@suse.com" , "mgross@linux.intel.com" , "bjorn.andersson@linaro.org" , "lgirdwood@gmail.com" , "linux-kernel@vger.kernel.org" , "platform-driver-x86@vger.kernel.org" , "wens@csie.org" , "saravanak@google.com" , "heikki.krogerus@linux.intel.com" , "jroedel@suse.de" , "rafael@kernel.org" , "myungjoo.ham@samsung.com" , "andriy.shevchenko@linux.intel.com" , "agross@kernel.org" , "cw00.choi@samsung.com" Subject: Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init Message-ID: References: <1230b0d2ba99ad546d72ab079e76cb1b3df32afb.1613216412.git.matti.vaittinen@fi.rohmeurope.com> <2fb4b305-a93f-f91e-3001-dab5057e39cc@redhat.com> <084893a3-0071-13e9-5ce6-b7b027e6cd2a@roeck-us.net> <16140f5b-c504-1c07-9f0c-3813d686d157@redhat.com> <74ec29cb5780e93cca3d4cdec221c65e764c8a3e.camel@fi.rohmeurope.com> <400d3e82-a76e-136c-0e03-ed7e40608e2a@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <400d3e82-a76e-136c-0e03-ed7e40608e2a@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 15, 2021 at 11:37:26AM +0100, Hans de Goede wrote: > 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 ? I don't know, sorry, let's revisit this after 5.12-rc1 is out, with a patch set that I can review again, and we can go from there as I can't do anything until then... thanks, greg k-h