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 E72E918C034 for ; Mon, 16 Jun 2025 11:37:16 +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=1750073839; cv=none; b=PZjT5ulnH4Q5GKSPXe8GoK51JAwOdp6YZty/8s/MBidCb8p2AqlWYPrYPLmmqjTcUIZGO4MFt3O3maUOI/v63pj4byw0Dvy2Ff212O74VEJV21tfy4aP0JV/80uTP5a3gFbUE6xdbEW9My6a1TKZ+S8FuDXXSMD1/76R8bCAwPA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750073839; c=relaxed/simple; bh=JsWLZzQTRBPO80hHR2MkijbaHfX3q09pvTBXpssbON4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ME1sW2rVhdOdcVAq4VIhwkJb2AR2TqP/4fhg8oRR36lSadqUB0ByBV11zedpO2ohyFwfAuDNY5zmgwaU0IBbQO9u8ME+EwLBbzC0xRlW6j0MA/PIpL9D+rc7qVMmLjzTp2aMHp8BC8qjvD/sMFWsBgupeayyLZjjQvMWAqXVSL8= 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=nVKgF7iy; 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="nVKgF7iy" Received: by mail-ed1-f41.google.com with SMTP id 4fb4d7f45d1cf-6070293103cso7664021a12.0 for ; Mon, 16 Jun 2025 04:37:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1750073835; x=1750678635; 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=Fqps01xAn+ufX8cXNqnzTmjdMwbQ7+l3ycOFCn7T4CU=; b=nVKgF7iyImhD7Ir/LyE7Past2M6vlSqZsazRsEOsSMY0clL9+dYnpArxXd0+uDVGL9 Cnu50quzgsUYkT4Fht91UMxJ7WhVFp5im0Qccf8LCUwf8Vi1jF/SEeo1T+WFCPIWIGzt stJOpZ8qV3cb9Ma5OJqCI10J71G+yVyCx00Yp0DJBxwemIaex5yEztzchv5+SHIdapOw 5E6fDxScA+BwQE+vXq/Lyjaixe8vQFX0M9IBjcEge2KfA99hEqdSASzQ65JWS9LnFL+4 dHjfHrI49wYO9bxM6ySlERCzlmOujw8PbdrsM85E0koe+1fz8AYA2/bPmU+bxCWopD7T ajLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750073835; x=1750678635; 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=Fqps01xAn+ufX8cXNqnzTmjdMwbQ7+l3ycOFCn7T4CU=; b=GBFJfoT3BPC8FJ+ks9GS1FyA92VxU99jv5hpuLfgebi9Ca7PYE8tWqCa/HEYs3V5fZ m0jjmuazuigjSO/WK76J2jL99b88SifNnZx07rUMOyO3eH7JGBIP5bW9XwbnD9hPbtIY NN0F2/VtWLKb8RAAyKQ5ETsePTCSaqqxWoVPETpO9cgJvV30P4yBC3GL1J6G2bR4nyUe MEqzsmwvExPQlInXisErYso7ZEM9zkfJKw9ennNVmSLIlhzkzPCBS7wDkOJX1cOKMsPg VzM/RVNl1xgPUfe2NyEIZdDxSYFpiRGHttOUVgYEDA2K30kPR+mylz3hUCHZKoO9pR7p Lhxg== X-Forwarded-Encrypted: i=1; AJvYcCXmIlM2PhUyuVmFPFaoXmSZjjoJecHDEwDn0rko6IWPlN2Ei/jhHWJpb69iypQyHR+3twtC4jID3UJA520=@vger.kernel.org X-Gm-Message-State: AOJu0YxHNG+AsdroYMXa5I0L7hpi2BiyB9uiMfnTg9mQVbur04DffK+Z J9M9UsmaDJLODCM2eHaeW3H8gvLWfBnlBHYkQApoAqBnjOtcYc+Y8jq7jLihkMPIM/U= X-Gm-Gg: ASbGncsnjJ+uQ6IMGx0BOXB7PBpnqRUgLwyfO35OQZdiEenNl8Eiayw8xDtxqV8hrAp QjPytr/CSrNxpFIRytFOhxFrMGw8V8ZLXv/JwUBRCvjiyS94z0qUaHOQcw7gM8VG0bA2GG+4she YJvy3mIfe82hO9MNcSgqqRQH2AvlyycMA6/AOj2SI0SRFh85oPZo/rL2aX3L/fAC/x0L6L9jiq2 hPngVL6TW2n8JYAXJpRRRHutm6G2bneIw7f/zwxf0kX40o+wp0D8jDnmvSYsCqDeDJYpSxu0r8x 0HlQN8EzKHQD33REeH5x6y9j3cZJG2hTTiwhpl3UqYEL8vwWIr84hWIqoEo3n3Ye8wOcvGE= X-Google-Smtp-Source: AGHT+IF+V2wo9BZ0sFidE7RWAYeIfibmbljW8n79SLf0MLj2poZnzBE9gG1l54wN+yQSUl2Bn/YPiw== X-Received: by 2002:a50:9fa8:0:b0:608:f54b:5c81 with SMTP id 4fb4d7f45d1cf-608f54b5e6dmr5069142a12.1.1750073835049; Mon, 16 Jun 2025 04:37:15 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.110]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-608b48f33c6sm5959159a12.30.2025.06.16.04.37.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Jun 2025 04:37:14 -0700 (PDT) Message-ID: <1b83c587-76c2-4fa1-aef8-f94575a3627a@tuxon.dev> Date: Mon, 16 Jun 2025 14:37:13 +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> <4360ee7a-d85a-4fa0-a1d6-d09a3b9d57c0@tuxon.dev> From: Claudiu Beznea Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 16.06.2025 14:18, Rafael J. Wysocki wrote: > On Mon, Jun 16, 2025 at 11:37 AM Claudiu Beznea > wrote: >> >> 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. > > This is a matter of adding one more dev->pm_domain check AFAICS, but OK. I don't know all the subtleties around this, my concern with adding one more check on dev->pm_domain was that the dev->pm_domain->dismiss() will never be called if the ->detach() function will be called before ->dismiss() and it will set dev->pm_domain = NULL (as it does today (though genpd_dev_pm_detach() and acpi_dev_pm_detach())). For platform drivers that used to call dev_pm_domain_detach() in platform bus remove function, if I'm not wrong, the dev->pm_domain->dismiss() was never called previously. If that is a valid scenario, the code proposed in this thread will change the behavior for devices that have ->dismiss() implemented. Thank you, Claudiu