From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D6E5D270577 for ; Mon, 16 Jun 2025 09:37:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750066668; cv=none; b=sTAzO79iBaRIyKedZRZp//LyQP7BQ7vgJmYczLWJx7jpD2atGxOvhl+rxSyUVxfVxQi2nO6zXlmJS83pZ0OPZuDNst6XT9gRFwYq0YzJmVLDacsu1jUB5tyAehF3RylGoWNnLkv/10CHbv+zrebBNPMKDcOacRK1wBGhkwKN1jg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750066668; c=relaxed/simple; bh=v7iwsXmtI/7HuEw8E5jMH8gjGCJ/PmdbDct0UukEscI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=CRAu8vK3HMLkVs8H62G2N8ric3liLWmr/9D/5gZI5VOSODL4u+CvtHKS2ZIoL8Vb3QzTtJ1UB81Qya6kXRoJmbWMNgxD7fcDHdap0lrOm8mf28yJvGiKBOMSmXkVFI/ciYXGT+OyEio+SxIL5UMMkmwWT20UHohqUpqWudx22kU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev; spf=pass smtp.mailfrom=tuxon.dev; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b=W7+MozSm; arc=none smtp.client-ip=209.85.208.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b="W7+MozSm" Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-607b59b447bso7528470a12.1 for ; Mon, 16 Jun 2025 02:37:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1750066664; x=1750671464; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=dfP8v62XGv6w7z/ZmGpzfijGDXvy28lSonmS90eMeCc=; b=W7+MozSmJ0KttRNUxdO2NaTes8jsh6mtJkE40GPIZHfMhgoWxq2VW7+1KnpgkdA933 bY8dZsdRaZck6xWd/OnGzO+o2NR+cJ+Nx25Mz+OrDcekVJf6zgrAedgZRPV6Tf+FUtDZ xrwLb+8K3BvFq5a0SAywDWXOT75BAc+vVTo7a2uHxTA09UixEhyQFRteBTYH6TmIWfaJ St/CGkf8CZtkZLD5RvoVIz7Y3y0T5JYxd8AfKeK98PDDW6zL1kj+nZYicAa9qERwKJfV UVTwTCUy1RT+Yy9XM32X9TmqVQrFxLb/ik9wd1YqDxyzuyEC242Y5ZJNTBlxp1HgbuZF F/Xg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750066664; x=1750671464; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dfP8v62XGv6w7z/ZmGpzfijGDXvy28lSonmS90eMeCc=; b=jFmUMWg3RJ7kQTEIm5MVwm2G+09bFQCC8dLP+q71EP0lBoeakq0UnukqWre2+ommTc RmE7YwnD/P7ko7B7IiKXHRw+IEtOZLjv8hJ+9m5eoCURw4P1z70Z8IwjHwzS6A8poc5D HG4UwX8bnSMF3HA0Zy103kMwpF/DgsbxC2o/9PuXWZNSuGj18sBAJLJYDWN0UaE0aUtL E0rtFiUikl0CwBnB3kbnt2593tBy590ouz13fcOG+bGRBwv3O2Xohy4yDpj+FeIDlecD 9UOUjbWDM0didUoiuNLWjCFwOYWtl5tDocBo9uTQxWQePiaxV6xuRp25g5IATI/C4YmF aPHw== X-Forwarded-Encrypted: i=1; AJvYcCWVQVDvX0dBkZvoWEtuYab4pKgkM+3+6RnaGKzCgb1ypwrcjRG+oF7X88fbc6nerN26mBFGWZrQxBQKwJs=@vger.kernel.org X-Gm-Message-State: AOJu0YzJwqYO68HZmELcLlrP6Nk53vLycvZYoTtm6qv/dqwBS8Rn/gN9 JcXzuF03mOmL/4Vsqx1nP/vSNEfGevifPHl6UzJ9fWalcHZWfEJrJC0rOOQDdr2zWSw= X-Gm-Gg: ASbGncuPVARydJzYkHCeRkQ9eQi1GjLFw5RBRjhkELcgJYN8I31a6IYBoqBa8Sxac1+ yNSWp+oRfk5l/YWXOjoVaqCMs+RztBRrf8nRaC7x9NSo2irUyP6ZBgvDpmPRcXzZDoZ+c7uYx+R VeghTeDKANwnSOUjkpCRQGd2f4kh1ihCs2qB7yktBv/FPx0TDLcxgCxJH+bfshBJqs4v6qMZX4/ ztuDsEYzp6VGhSGUE94pnHZXsAYCMZ+Ax3teoQ4QqcOnibK1KSHR2f/PffGLeIedUU1TzRb5rp4 s2bXhgFLjXIdeKlEbBNoJIf4kkln+Vi0C1eYh+1uVlQKUkQd8ZvuAAVKprF/D4PKDtyiAnp5a/X 3cyVtFg== X-Google-Smtp-Source: AGHT+IGstgDvCZ7kkheENEgwtKA2GsdZiS2gQeWTudsZRkiraVvavy0/6m9lYQPhBNBoWFt98BlCYg== X-Received: by 2002:a17:906:9f88:b0:ad8:96d2:f3e with SMTP id a640c23a62f3a-adfad326d9cmr714122866b.22.1750066663863; Mon, 16 Jun 2025 02:37:43 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.110]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-adfddc96709sm126821866b.134.2025.06.16.02.37.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Jun 2025 02:37:43 -0700 (PDT) Message-ID: <4360ee7a-d85a-4fa0-a1d6-d09a3b9d57c0@tuxon.dev> Date: Mon, 16 Jun 2025 12:37:41 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/2] PM: domains: Add devres variant for dev_pm_domain_attach() To: "Rafael J. Wysocki" Cc: Jonathan Cameron , Dmitry Torokhov , gregkh@linuxfoundation.org, dakr@kernel.org, len.brown@intel.com, pavel@kernel.org, ulf.hansson@linaro.org, daniel.lezcano@linaro.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, bhelgaas@google.com, geert@linux-m68k.org, linux-iio@vger.kernel.org, linux-renesas-soc@vger.kernel.org, fabrizio.castro.jz@renesas.com, Claudiu Beznea References: <20250606111749.3142348-1-claudiu.beznea.uj@bp.renesas.com> <20250606111749.3142348-2-claudiu.beznea.uj@bp.renesas.com> <20250607140600.76e87ea5@jic23-huawei> <486a1110-5336-42fd-82b8-a7b1452bad65@tuxon.dev> From: Claudiu Beznea Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi, Rafael, On 13.06.2025 13:02, Rafael J. Wysocki wrote: > On Fri, Jun 13, 2025 at 9:39 AM Claudiu Beznea wrote: >> >> Hi, Rafael, >> >> On 09.06.2025 22:59, Rafael J. Wysocki wrote: >>> On Sat, Jun 7, 2025 at 3:06 PM Jonathan Cameron wrote: >>>> >>>> On Fri, 6 Jun 2025 22:01:52 +0200 >>>> "Rafael J. Wysocki" wrote: >>>> >>>> Hi Rafael, >>>> >>>>> On Fri, Jun 6, 2025 at 8:55 PM Dmitry Torokhov >>>>> wrote: >>>>>> >>>>>> On Fri, Jun 06, 2025 at 06:00:34PM +0200, Rafael J. Wysocki wrote: >>>>>>> On Fri, Jun 6, 2025 at 1:18 PM Claudiu wrote: >>>>>>>> >>>>>>>> From: Claudiu Beznea >>>>>>>> >>>>>>>> The dev_pm_domain_attach() function is typically used in bus code alongside >>>>>>>> dev_pm_domain_detach(), often following patterns like: >>>>>>>> >>>>>>>> static int bus_probe(struct device *_dev) >>>>>>>> { >>>>>>>> struct bus_driver *drv = to_bus_driver(dev->driver); >>>>>>>> struct bus_device *dev = to_bus_device(_dev); >>>>>>>> int ret; >>>>>>>> >>>>>>>> // ... >>>>>>>> >>>>>>>> ret = dev_pm_domain_attach(_dev, true); >>>>>>>> if (ret) >>>>>>>> return ret; >>>>>>>> >>>>>>>> if (drv->probe) >>>>>>>> ret = drv->probe(dev); >>>>>>>> >>>>>>>> // ... >>>>>>>> } >>>>>>>> >>>>>>>> static void bus_remove(struct device *_dev) >>>>>>>> { >>>>>>>> struct bus_driver *drv = to_bus_driver(dev->driver); >>>>>>>> struct bus_device *dev = to_bus_device(_dev); >>>>>>>> >>>>>>>> if (drv->remove) >>>>>>>> drv->remove(dev); >>>>>>>> dev_pm_domain_detach(_dev); >>>>>>>> } >>>>>>>> >>>>>>>> When the driver's probe function uses devres-managed resources that depend >>>>>>>> on the power domain state, those resources are released later during >>>>>>>> device_unbind_cleanup(). >>>>>>>> >>>>>>>> Releasing devres-managed resources that depend on the power domain state >>>>>>>> after detaching the device from its PM domain can cause failures. >>>>>>>> >>>>>>>> For example, if the driver uses devm_pm_runtime_enable() in its probe >>>>>>>> function, and the device's clocks are managed by the PM domain, then >>>>>>>> during removal the runtime PM is disabled in device_unbind_cleanup() after >>>>>>>> the clocks have been removed from the PM domain. It may happen that the >>>>>>>> devm_pm_runtime_enable() action causes the device to be runtime-resumed. >>>>>>> >>>>>>> Don't use devm_pm_runtime_enable() then. >>>>>> >>>>>> What about other devm_ APIs? Are you suggesting that platform drivers >>>>>> should not be using devm_clk*(), devm_regulator_*(), >>>>>> devm_request_*_irq() and devm_add_action_or_reset()? Because again, >>>>>> dev_pm_domain_detach() that is called by platform bus_remove() may shut >>>>>> off the device too early, before cleanup code has a chance to execute >>>>>> proper cleanup. >>>>>> >>>>>> The issue is not limited to runtime PM. >>>>>> >>>>>>> >>>>>>>> If the driver specific runtime PM APIs access registers directly, this >>>>>>>> will lead to accessing device registers without clocks being enabled. >>>>>>>> Similar issues may occur with other devres actions that access device >>>>>>>> registers. >>>>>>>> >>>>>>>> Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and >>>>>>>> dev_pm_domain_detach() in bus probe and bus remove, it ensures that the >>>>>>>> device is detached from its PM domain in device_unbind_cleanup(), only >>>>>>>> after all driver's devres-managed resources have been release. >>>>>>>> >>>>>>>> For flexibility, the implemented devm_pm_domain_attach() has 2 state >>>>>>>> arguments, one for the domain state on attach, one for the domain state on >>>>>>>> detach. >>>>>>> >>>>>>> dev_pm_domain_attach() is not part driver API and I'm not convinced at >>>>>> >>>>>> Is the concern that devm_pm_domain_attach() will be [ab]used by drivers? >>>>> >>>>> Yes, among other things. >>>> >>>> Maybe naming could make abuse at least obvious to spot? e.g. >>>> pm_domain_attach_with_devm_release() >>> >>> If I'm not mistaken, it is not even necessary to use devres for this. >>> >>> You might as well add a dev_pm_domain_detach() call to >>> device_unbind_cleanup() after devres_release_all(). There is a slight >>> complication related to the second argument of it, but I suppose that >>> this can be determined at the attach time and stored in a new device >>> PM flag, or similar. >>> >> >> I looked into this solution. I've tested it for all my failure cases and >> went good. > > OK > >>> Note that dev->pm_domain is expected to be cleared by ->detach(), so >>> this should not cause the domain to be detached twice in a row from >>> the same device, but that needs to be double-checked. >> >> The genpd_dev_pm_detach() calls genpd_remove_device() -> >> dev_pm_domain_set(dev, NULL) which sets the dev->pm_domain = NULL. I can't >> find any other detach function in the current code base. > > There is also acpi_dev_pm_detach() which can be somewhat hard to find, > but it calls dev_pm_domain_set(dev, NULL) either. > >> The code I've tested for this solution is this one: >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index b526e0e0f52d..5e9750d007b4 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -552,8 +553,11 @@ static void device_unbind_cleanup(struct device *dev) >> dev->dma_range_map = NULL; >> device_set_driver(dev, NULL); >> dev_set_drvdata(dev, NULL); >> - if (dev->pm_domain && dev->pm_domain->dismiss) >> - dev->pm_domain->dismiss(dev); >> + if (dev->pm_domain) { >> + if (dev->pm_domain->dismiss) >> + dev->pm_domain->dismiss(dev); >> + dev_pm_domain_detach(dev, dev->pm_domain->detach_power_off); > > I would do the "detach" before the "dismiss" to retain the current ordering. I applied on my local development branch all your suggestions except this one because genpd_dev_pm_detach() as well as acpi_dev_pm_detach() set dev->pm_domain = NULL. Due to this I would call first ->dismiss() then ->detach(), as initially proposed. Please let me know if you consider it otherwise. Thank you, Claudiu