Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>
To:     Fabrice Gasnier <fabrice.gasnier@foss.st.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "maz@kernel.org" <maz@kernel.org>,
        "m.szyprowski@samsung.com" <m.szyprowski@samsung.com>
CC:     "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "amelie.delaunay@foss.st.com" <amelie.delaunay@foss.st.com>,
        "alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>
Subject: RE: [PATCH v2] usb: dwc2: fix a devres leak in hw_enable upon suspend
 resume
Thread-Topic: [PATCH v2] usb: dwc2: fix a devres leak in hw_enable upon
 suspend resume
Thread-Index: AQHZV+MsmbvIgrm46EGwKIDQHfFEzK8JbLfA
Date:   Fri, 24 Mar 2023 05:07:51 +0000
Message-ID: <BYAPR12MB33992472023C4D6BB4127B0CA7849@BYAPR12MB3399.namprd12.prod.outlook.com>
References: <20230316084127.126084-1-fabrice.gasnier@foss.st.com>
In-Reply-To: <20230316084127.126084-1-fabrice.gasnier@foss.st.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ouL+wI1k2+0IbePfwixP1SDbKTF/fqbV6DigpLtf3J2+oRBI+7DxsjAnpqiB?=
 =?us-ascii?Q?60AoDl2gO4RwDS9V5gCqxJpk6nXTM0UG9U871FSkVHZTbKODMjRvr/1n+79a?=
 =?us-ascii?Q?Su7ixI95gW4HY+SgQsKB6BXWyThdSf6+50vLq4RtgZToEXc+PTv+ej/bxIgY?=
 =?us-ascii?Q?0zOCjg7/vVyalm5o3vNf4PnOZFDuvMSun3DpRiZlZzj5yPrNRU87S45yPSH+?=
 =?us-ascii?Q?rMT0d9rECCLWQdJtHTDWn2XciYKIsj3H050bZNSV6ndsXM2f0/RexxcHUY78?=
 =?us-ascii?Q?rQX1z9ZqEk1N1demk0C+s9ewzLJW1i1zCpT0BsyvF/VsuAopCrEoGbt3PSLB?=
 =?us-ascii?Q?Ni3cBG84DaQP0Dh8KD8A+574mHgwoCcWRKbIoV5cb4WTojOFNj4WaLhaaJjT?=
 =?us-ascii?Q?4HDf0BngoatwHTg/I8h/6FywRWGA4Z+6mmYqmow+yitKDcxd7vhpBgwRFEUy?=
 =?us-ascii?Q?uhKg5+M3Qu6X+R6z0NNWos4PexeAUm9vtpd/9tetMR87+f1jjTbGRPVwdnNy?=
 =?us-ascii?Q?xY545sbo9xw+4SwoMBTrjQybGh5mQEoDSna66iZ6mKC9uyc9ZR6eQXnGiPUg?=
 =?us-ascii?Q?bfrVvzfHnf0cIzbcYO7l5azX1EREyecNTVG41SJkOWHsN5ApiNA9HXqhmDRk?=
 =?us-ascii?Q?4wyWlaOOGXkC4NLn2aDlYee0rNRQfl/NLaLq6/GvMviIVWANLNW/2LWT4Zrv?=
 =?us-ascii?Q?58BRNKgcaAKAKgNZJfJFGvEGILN6mqnD7QliyapO1OU/FFc+f2W+KO3JVsfx?=
 =?us-ascii?Q?Wo6WvzEfN1Eg7LxAUNWSJtU4okv3EOBr4EDmgl+n4Q6qBIjEpf0g9MdHyCUH?=
 =?us-ascii?Q?IS9wvwNZh1ttnxOSFDgxV74wd3UNQXdOEcoFPe+EwiWD6YIf01ZoqbzhY6X2?=
 =?us-ascii?Q?E3zBk6awQDYGLp6mXomjaXUkip/3pNOoq5rlBEiHijak5tIIl8HjH4ecsaAh?=
 =?us-ascii?Q?59M6mxlHLfns85k5v9Nwgtr/e6wtrjAFKOi06vf4mI1EgfFxzblsmZegIRRh?=
 =?us-ascii?Q?UR0uaE3/+xu79h19C/Jfg5KcSoKrkLZaL4pUHgzRscV6nTBrbz2W3rXRi6ol?=
 =?us-ascii?Q?l89m5hC68r11L9i2aDFHHrCwnn+pjLntFwkbnXnLCBrHBbPj8fP4gyzDN7Zq?=
 =?us-ascii?Q?MgsH2rNoWIW5GokYSYQMLGT9+Xc4vTXM6AZR2pij+ByMOApjUY7LEeXWdAlE?=
 =?us-ascii?Q?L/XQAR8GwPjOMWg+PyCnqx4d3dyNUbtm+bWXVo/2zAWJjPor2gLnSyyHWOY6?=
 =?us-ascii?Q?Hdv4RX8gwL2fvJnICG6ecfOLqC5ekOLpQQPq+YmDYECOCEd1pM4CE42hlAHY?=
 =?us-ascii?Q?LrPJvVd66xZpnfH5eRTF5ImIdzXSnB7hdTfGhG/Ih1TveUb77jnHugYTHhiM?=
 =?us-ascii?Q?1CYgrKHVBdkfRuZd2IJBtZu81AioCHHXRd1zU2RGJNvpTolPa0zmSJv21dLY?=
 =?us-ascii?Q?QFz05v+7saL9jRNvAvUricEW/1QoNXGo0++GRirgr4hKCVqViPVwiE5e/34X?=
 =?us-ascii?Q?O+v3UsqlayFAYcpUqcjNC6vs34euvFxLsXChXof5f37snvF3aZ4J7/+jF4lE?=
 =?us-ascii?Q?3c/hGjYKZWq+FAVMiqOxOQ50z1CBaHSkzp6Cpc6D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?gHtAZWcDNRcmsrazsAdvkE/t0lanchm6V5hXMVWfB9EMIvroEM0CzewTXq3V?=
 =?us-ascii?Q?TTdhDCP+xhHGJMEsgwyQp2Z53Zfi4+NkY7jzQI31HkS+i0O8x092OBY5OgJf?=
 =?us-ascii?Q?2tzV8CO/0BRSCIE0N9+53HG+eE/ubL+GTas9YfIs2GxjCG0oZFN9IzHvq2NG?=
 =?us-ascii?Q?fnK2WSafjnfRXLLjubYmET61IDIHb9JXPoBlXfxlRGwRmHpnYMmbFmaFOnuD?=
 =?us-ascii?Q?z3loC0LikYntyJI+UdecbmD55URr8B7Y5aVBordaPklWuLp02CQhH/bYOVoC?=
 =?us-ascii?Q?7QXL0s6SmfjTyLrDtQ9XY9FEFn98O8ybrdtJojU602wNYvLYLiMbaFnzWsDG?=
 =?us-ascii?Q?9AtFvDwZ8/ABD6xMyrIL6Gzp6jK1xjyVMkdLuA2hnyEFhXpBRi11vAbO8/tG?=
 =?us-ascii?Q?j1HlKG5F/a6W26ifIE4lWIfRJIMSaCCWtZZLcY1IERCawpNZ4QhQxhdcOS+W?=
 =?us-ascii?Q?qiRMxTpLowMo66R8inwXPM5rLwjjDbdBig3IDoLiQw47sznRXzBLWqSr0V+8?=
 =?us-ascii?Q?fLu1squf37iUHFoT/MGrD1lz9sg4+LZWworejeh2IrZPa8czfI0QRmbR0HUe?=
 =?us-ascii?Q?oKCpICQGsAPajwivL+HA5BxWaKmKhDYHCYbSWw3sZTOxkVDYwjqZoUts/y6f?=
 =?us-ascii?Q?0yBgLi+1SwkOiEWKLApe2CsIjkM8DzyLOue2Gh9nwRZBLGY58zu7Qk0znHeC?=
 =?us-ascii?Q?fKpIYKnRKtnVeupStnETAcf2ouDHQ/63lRORL4TfWHpAgh5ug51DzDzxITwQ?=
 =?us-ascii?Q?NVY1hc1v1M7PmclX8UaxdsAjQUveObLrxiv76mq7QE4X3uUuiHWFIAyj7Qae?=
 =?us-ascii?Q?OL7SEInjjKKIq/wHoUPWYTkgu0zAKXinKQX7lLf/qO3n/Qab9ked0nBCbwTZ?=
 =?us-ascii?Q?blOh77HuX+/tTCEMlISQwNDDlvTqOiKbK5ChcsYljfsXYxrPNEjkGx3TREk0?=
 =?us-ascii?Q?eJzZXCKpAvhDJxIclVMKJwdzVQGOKyyQ45ANJb9OUnawsFEu3/cVyaQDMUts?=
 =?us-ascii?Q?Wa5PjXt5FU5vPw+t/+6DEwgALQ=3D=3D?=
X-OriginatorOrg: synopsys.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3399.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c689d963-1491-4714-1154-08db2c25b7dc
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Mar 2023 05:07:51.2497
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2C5NgU1QDaGje3+YF4EvH1qPDGtZul5ZTWs1N3CF9Rp+Q9pLQ5P6B+/6YIYOmsECdl9TTp6T5ORdIMTfR4/4Kw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB6946
X-Proofpoint-GUID: bMVM6whM8Woy4OIy-oX9KE3ddiWBl9Lo
X-Proofpoint-ORIG-GUID: bMVM6whM8Woy4OIy-oX9KE3ddiWBl9Lo
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-24_02,2023-03-23_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=outbound_active_cloned_notspam policy=outbound_active_cloned score=0
 mlxscore=0 suspectscore=0 impostorscore=0 bulkscore=0 malwarescore=0
 spamscore=0 adultscore=0 clxscore=1011 priorityscore=1501 mlxlogscore=999
 lowpriorityscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2303200000 definitions=main-2303240040
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE
        autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Fabrice,

>On 3/16/2023 12:41 PM, Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote=
:
>From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>Sent: Thursday, March 16, 2023 12:41 PM
>To: Minas Harutyunyan <hminas@synopsys.com>; gregkh@linuxfoundation.org;
>maz@kernel.org; m.szyprowski@samsung.com
>Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-stm32@s=
t-
>md-mailman.stormreply.com; amelie.delaunay@foss.st.com;
>alexandre.torgue@foss.st.com; fabrice.gasnier@foss.st.com
>Subject: [PATCH v2] usb: dwc2: fix a devres leak in hw_enable upon suspend
>resume
>
>Each time the platform goes to low power, PM suspend / resume routines
>call: __dwc2_lowlevel_hw_enable -> devm_add_action_or_reset().
>This adds a new devres each time.
>This may also happen at runtime, as dwc2_lowlevel_hw_enable() can be calle=
d
>from udc_start().
>
>This can be seen with tracing:
>- echo 1 > /sys/kernel/debug/tracing/events/dev/devres_log/enable
>- go to low power
>- cat /sys/kernel/debug/tracing/trace
>
>A new "ADD" entry is found upon each low power cycle:
>... devres_log: 49000000.usb-otg ADD 82a13bba devm_action_release (8 bytes=
)
>... devres_log: 49000000.usb-otg ADD 49889daf devm_action_release (8 bytes=
)
>...
>
>A second issue is addressed here:
>- regulator_bulk_enable() is called upon each PM cycle (suspend/resume).
>- regulator_bulk_disable() never gets called.
>
>So the reference count for these regulators constantly increase, by one up=
on
>each low power cycle, due to missing regulator_bulk_disable() call in
>__dwc2_lowlevel_hw_disable().
>
>The original fix that introduced the devm_add_action_or_reset() call, fixe=
d
>an issue during probe, that happens due to other errors in
>dwc2_driver_probe() -> dwc2_core_reset(). Then the probe fails without
>disabling regulators, when dr_mode =3D=3D USB_DR_MODE_PERIPHERAL.
>
>Rather fix the error path: disable all the low level hardware in the error
>path, by using the "hsotg->ll_hw_enabled" flag. Checking dr_mode has been
>introduced to avoid a dual call to dwc2_lowlevel_hw_disable().
>"ll_hw_enabled" should achieve the same (and is used currently in the
>remove() routine).
>
>Fixes: 54c196060510 ("usb: dwc2: Always disable regulators on driver
>teardown")
>Fixes: 33a06f1300a7 ("usb: dwc2: Fix error path in gadget registration")
>Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Acked-by: Minas Harutyunyan <hminas@synopsys.com>

>---
>Changes in v2: commit message updated to remove useless tags
>---
> drivers/usb/dwc2/platform.c | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c ind=
ex
>262c13b6362a..d1589ba7d322 100644
>--- a/drivers/usb/dwc2/platform.c
>+++ b/drivers/usb/dwc2/platform.c
>@@ -91,13 +91,6 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
> 	return 0;
> }
>
>-static void __dwc2_disable_regulators(void *data) -{
>-	struct dwc2_hsotg *hsotg =3D data;
>-
>-	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
>-}
>-
> static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)  {
> 	struct platform_device *pdev =3D to_platform_device(hsotg->dev); @@ -
>108,11 +101,6 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg
>*hsotg)
> 	if (ret)
> 		return ret;
>
>-	ret =3D devm_add_action_or_reset(&pdev->dev,
>-				       __dwc2_disable_regulators, hsotg);
>-	if (ret)
>-		return ret;
>-
> 	if (hsotg->clk) {
> 		ret =3D clk_prepare_enable(hsotg->clk);
> 		if (ret)
>@@ -168,7 +156,7 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsot=
g
>*hsotg)
> 	if (hsotg->clk)
> 		clk_disable_unprepare(hsotg->clk);
>
>-	return 0;
>+	return regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>+hsotg->supplies);
> }
>
> /**
>@@ -607,7 +595,7 @@ static int dwc2_driver_probe(struct platform_device
>*dev)
> 	if (hsotg->params.activate_stm_id_vb_detection)
> 		regulator_disable(hsotg->usb33d);
> error:
>-	if (hsotg->dr_mode !=3D USB_DR_MODE_PERIPHERAL)
>+	if (hsotg->ll_hw_enabled)
> 		dwc2_lowlevel_hw_disable(hsotg);
> 	return retval;
> }
>--
>2.25.1