[{"id":3687572,"web_url":"http://patchwork.ozlabs.org/comment/3687572/","msgid":"<20260507080003.365FBC2BCB8@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-07T08:00:02","subject":"Re: [PATCH v8 phy-next 13/31] scsi: ufs: qcom: keep separate track\n of PHY power state","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:\n- [High] Failing to clear `phy_powered_on` upon a `phy_power_off()` error leads to calling `phy_power_off()` on an exited PHY during driver removal.\n- [High] Silently ignoring `phy_power_off()` errors alters error propagation and can lead to system hangs.\n--\n\ncommit 22afdb4c537193d799444f149b239ffbc8dfa455\nAuthor: Vladimir Oltean \n\nscsi: ufs: qcom: keep separate track of PHY power state\n\nThis commit adds a phy_powered_on boolean to the Qualcomm UFS driver to\ntrack the PHY power state. It wraps phy_power_on() and phy_power_off()\nin helper functions that check and update this state to prevent unbalanced\ncalls.\n\n> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c\n> --- a/drivers/ufs/host/ufs-qcom.c\n> +++ b/drivers/ufs/host/ufs-qcom.c\n\n[ ... ]\n\n> +static void ufs_qcom_phy_power_off(struct ufs_qcom_host *host)\n> +{\n> +\tint ret;\n> +\n> +\tif (!host->phy_powered_on)\n> +\t\treturn;\n> +\n> +\tret = phy_power_off(host->generic_phy);\n> +\tif (ret) {\n> +\t\tdev_warn(host->hba->dev, \"Failed to power off PHY: %pe\\n\",\n> +\t\t\t ERR_PTR(ret));\n> +\t\treturn;\n> +\t}\n> +\n> +\thost->phy_powered_on = false;\n> +}\n\nIf phy_power_off() returns an error, the early return leaves\nhost->phy_powered_on set to true.\n\nDuring driver removal, ufshcd_hba_exit() invokes ufs_qcom_exit(), which\ncalls ufs_qcom_phy_power_off() followed by phy_exit(). If the power off\nfails here, phy_powered_on remains true.\n\nWhen ufshcd_hba_exit() subsequently calls ufshcd_setup_clocks(hba, false),\nit will invoke ufs_qcom_phy_power_off() again. Because the flag is still\ntrue, could this result in calling phy_power_off() on an already exited PHY?\n\n[ ... ]\n\n> @@ -1420,20 +1453,14 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,\n> \t\t\t\tufs_qcom_dev_ref_clk_ctrl(host, false);\n> \t\t\t}\n> \n> -\t\t\terr = phy_power_off(phy);\n> -\t\t\tif (err) {\n> -\t\t\t\tdev_err(hba->dev, \"phy power off failed, ret=%d\\n\", err);\n> -\t\t\t\treturn err;\n> -\t\t\t}\n> +\t\t\tufs_qcom_phy_power_off(host);\n> \t\t}\n> \t\tbreak;\n\nBecause ufs_qcom_phy_power_off() returns void, it swallows the error from\nphy_power_off(). Previously, this error was propagated back to the UFS core,\nwhich correctly aborted the suspend or teardown flow.\n\nBy no longer returning the error, ufs_qcom_setup_clocks() silently returns 0.\nThe UFS core will then proceed to disable the clocks while the PHY is still\nactive and powered on.\n\nWould this early disabling of clocks lead to unclocked register accesses\nand asynchronous aborts causing system hangs?","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=V6Mu/l/c;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.234.253.10; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-54053-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"V6Mu/l/c\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org [172.234.253.10])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gB4Rm0WSNz1yCg\n\tfor ; Thu, 07 May 2026 18:00:20 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id B3B3F302D537\n\tfor ; Thu, 7 May 2026 08:00:12 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 1A41A37C933;\n\tThu, 7 May 2026 08:00:06 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id A0B2E37C111\n\tfor ; Thu, 7 May 2026 08:00:04 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 365FBC2BCB8;\n\tThu, 7 May 2026 08:00:03 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778140804; cv=none;\n b=VAzJuID6P7Tl9gSGJlzE1dF6Zq+YVbDLfBuoZLLAQw1Luf0nWl2AI2UKuukuugOCTv9cPRoDmVPaPrBAR1vPBhW3W1gvIAN/Rn8ODx2+akCY9ZRFi00NywCEpSZQD36WCvslxpOpqtZpypFJ7y9ztO2FiySgVnA0hM1yXlDblnk=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778140804; c=relaxed/simple;\n\tbh=zWJSTblMoNIj9xsEQg6h5VxF5Fho7k+w1XgwjqaVy0M=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=PxPTurDcm4l14w2tjmIiK+oLbP1MIkmC/5MbBfGDYOydlUs6ZuHRXQV5AexJOkEWg8IrFsIDg+FlONhZTac6R1ThB6L3ljGJYAdVV4NLaK8b0hk8My+PxMrUOLu9k6r1exqDI1QL+KpkyVjNRg0jXQ2IYZykX3U0C0bMyXai72Q=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=V6Mu/l/c; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778140803;\n\tbh=zWJSTblMoNIj9xsEQg6h5VxF5Fho7k+w1XgwjqaVy0M=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=V6Mu/l/cBsbj5S3RLsAfsZVs0EHd1RbsSS+IMjpvVQQOR/zll9eRXhOQDBHA2Dedr\n\t uhM7nDEtj/zT+z2tRMMVGar18yOkkTqWHqNuOv6Fx4DS/T3rH4EbZvUr/mMVhcOy2v\n\t f/EPePN0wmxFVSf21qa++TWn8zMJH/phYDkfX1HmZSYqdS7+kantC04Tvc8zYrH5II\n\t +eWiX2Y8TZtLmqZ32hWZoCFgoVAdamd/EN395LFmXJ91DsBWSes1pgkk9sCCMp0WrM\n\t u5s29tZy8LzttAJmsxrCF8D0sFAWBLHneP5fN69WjOpY9zQBE7NI1R9x9GKK0cXWAG\n\t bE+p6oaejqa5w==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH v8 phy-next 13/31] scsi: ufs: qcom: keep separate track\n of PHY power state","Reply-To":"sashiko@lists.linux.dev","To":"\"Vladimir Oltean\" ","Cc":"linux-pci@vger.kernel.org, wsa+renesas@sang-engineering.com","In-Reply-To":"<20260505100523.1922388-14-vladimir.oltean@nxp.com>","References":"<20260505100523.1922388-14-vladimir.oltean@nxp.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Thu, 07 May 2026 08:00:02 +0000","Message-Id":"<20260507080003.365FBC2BCB8@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":""}}]