From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY, URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83077C433E0 for ; Wed, 29 Jul 2020 10:28:09 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 51105206D4 for ; Wed, 29 Jul 2020 10:28:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="2ZlIQRg0"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mediatek.com header.i=@mediatek.com header.b="Ht1Tea7i" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 51105206D4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Date:To:From: Subject:Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=eBAawVNTKMinZZ3Yb8c70iY7TPl5LZYk3oQbKvgSjP0=; b=2ZlIQRg0omToDPaqaC9LC4Fbf /gOvZ/Wsde8WBIGPtDWf9bUgC79mz32OsufFD2iKehrVlU3vg8RPc/jBZMfb2fIQxBYrehNkO3soD 5w9in8QKJYiNEPzMh3JK8aW8cgJOo8Z6JC5BTQAqyIKNHKjGe4/BRhDLxR9u08Wx53EOwaumFvpGl gLxJ0izRVDEF8J0lPgnLlEfRYCky/hJ7PfciMsGWigt+1JTQfGCocoKSHI9XkAEAHuaxlNHYtlGVk bfINmwjSZPEbXSBYSgoQvE+WUG9G+QRA1AdLjBTU+PNTfsauZPKSMd3A5xTXMSy+9aRJKJ/QUg1rf iDke0CSWg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k0jI6-0000fO-Ay; Wed, 29 Jul 2020 10:26:30 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k0jI3-0000ei-UK; Wed, 29 Jul 2020 10:26:29 +0000 X-UUID: d65ffac5e3574664a6a05c23cdb08f23-20200729 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Transfer-Encoding:MIME-Version:Content-Type:References:In-Reply-To:Date:CC:To:From:Subject:Message-ID; bh=MPTSG+n/0Vm1h1PIQ5D8rTUTLY8/JFmBqw35qwYcLvU=; b=Ht1Tea7iLahWge2y/SdW44LdMcFc2GkR+fD57eB8qiyI290WFNy4nA0YuJBAEVjA2mmBPUgAHGQAvhQPx2F+hPVI88vBrtqK5BYGwVOTZe+dy3QLMIudlr1LZqM7EwqAhGCoBj0jBejJQIUSS+l3TQIjxXkevz63ForIRltypes=; X-UUID: d65ffac5e3574664a6a05c23cdb08f23-20200729 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 1770219959; Wed, 29 Jul 2020 02:26:21 -0800 Received: from MTKMBS02N2.mediatek.inc (172.21.101.101) by MTKMBS62N2.mediatek.inc (172.29.193.42) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 29 Jul 2020 03:26:23 -0700 Received: from mtkcas07.mediatek.inc (172.21.101.84) by mtkmbs02n2.mediatek.inc (172.21.101.101) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 29 Jul 2020 18:26:12 +0800 Received: from [172.21.77.33] (172.21.77.33) by mtkcas07.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Wed, 29 Jul 2020 18:26:13 +0800 Message-ID: <1596018374.17247.41.camel@mtkswgap22> Subject: Re: [PATCH v2] scsi: ufs: Fix possible infinite loop in ufshcd_hold From: Stanley Chu To: Can Guo Date: Wed, 29 Jul 2020 18:26:14 +0800 In-Reply-To: References: <20200729024037.23105-1-stanley.chu@mediatek.com> X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-TM-SNTS-SMTP: 5E697B60209E19EE040B75CE5B8A822265815B3269B1B7FDF6F122FDED418EC42000:8 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200729_062628_110126_811A5A7B X-CRM114-Status: GOOD ( 28.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-scsi@vger.kernel.org, martin.petersen@oracle.com, andy.teng@mediatek.com, jejb@linux.ibm.com, chun-hung.wu@mediatek.com, kuohong.wang@mediatek.com, linux-kernel@vger.kernel.org, asutoshd@codeaurora.org, avri.altman@wdc.com, linux-mediatek@lists.infradead.org, peter.wang@mediatek.com, alim.akhtar@samsung.com, matthias.bgg@gmail.com, beanhuo@micron.com, chaotian.jing@mediatek.com, cc.chou@mediatek.com, linux-arm-kernel@lists.infradead.org, bvanassche@acm.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Can, On Wed, 2020-07-29 at 16:43 +0800, Can Guo wrote: > Hi Stanley, > > On 2020-07-29 10:40, Stanley Chu wrote: > > In ufshcd_suspend(), after clk-gating is suspended and link is set > > as Hibern8 state, ufshcd_hold() is still possibly invoked before > > ufshcd_suspend() returns. For example, MediaTek's suspend vops may > > issue UIC commands which would call ufshcd_hold() during the command > > issuing flow. > > > > Now if UFSHCD_CAP_HIBERN8_WITH_CLK_GATING capability is enabled, > > then ufshcd_hold() may enter infinite loops because there is no > > clk-ungating work scheduled or pending. In this case, ufshcd_hold() > > shall just bypass, and keep the link as Hibern8 state. > > > > The infinite loop is expected as ufshcd_hold is called again after > link is put to hibern8 state, so in QCOM's code, we never do this. Sadly MediaTek have to do this to make our UniPro to enter low-power mode. > The cap UFSHCD_CAP_HIBERN8_WITH_CLK_GATING means UIC link state > must not be HIBERN8 after ufshcd_hold(async=false) returns. If driver is not in PM scenarios, e.g., suspended, above statement shall be always followed. But two obvious violations are existed, 1. In ufshcd_suspend(), link is set as HIBERN8 behind ufshcd_hold() 2. In ufshcd_resume(), link is set back as Active before ufshcd_release() is invoked So as my understanding, special conditions are allowed in PM scenarios, and this is why "hba->clk_gating.is_suspended" is introduced. By this thought, I used "hba->clk_gating.is_suspended" in this patch as the mandatory condition to allow ufshcd_hold() usage in vendor suspend and resume callbacks. > Instead of bailing out from that loop, which makes the logic of > ufshcd_hold and clk gating even more complex, how about removing > ufshcd_hold/release from ufshcd_send_uic_cmd()? I think they are > redundant and we should never send DME cmds if clocks/powers are > not ready. I mean callers should make sure they are ready to send > DME cmds (and only callers know when), but not leave that job to > ufshcd_send_uic_cmd(). It is convenient to remove ufshcd_hold/ > release from ufshcd_send_uic_cmd() as there are not many places > sending DME cmds without holding the clocks, ufs_bsg.c is one. > And I have tested my idea on my setup, it worked well for me. > Another benefit is that it also allows us to use DME cmds > in clk gating/ungating contexts if we need to in the future. > Brilliant idea! But this may not solve problems if vendor callbacks need more than UIC commands in the future. This simple patch could make all vendor operations on UFSHCI in PM callbacks possible with UFSHCD_CAP_HIBERN8_WITH_CLK_GATING enabled, and again, it allows those operations in PM scenarios only. > Please let me know your idea, thanks. > > Can Guo. Thanks, Stanley Chu > > > Signed-off-by: Stanley Chu > > Signed-off-by: Andy Teng > > > > --- > > > > Changes since v1: > > - Fix return value: Use unique bool variable to get the result of > > flush_work(). Thcan prevent incorrect returned value, i.e., rc, if > > flush_work() returns true > > - Fix commit message > > > > --- > > drivers/scsi/ufs/ufshcd.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index 577cc0d7487f..acba2271c5d3 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -1561,6 +1561,7 @@ static void ufshcd_ungate_work(struct work_struct > > *work) > > int ufshcd_hold(struct ufs_hba *hba, bool async) > > { > > int rc = 0; > > + bool flush_result; > > unsigned long flags; > > > > if (!ufshcd_is_clkgating_allowed(hba)) > > @@ -1592,7 +1593,9 @@ int ufshcd_hold(struct ufs_hba *hba, bool async) > > break; > > } > > spin_unlock_irqrestore(hba->host->host_lock, flags); > > - flush_work(&hba->clk_gating.ungate_work); > > + flush_result = flush_work(&hba->clk_gating.ungate_work); > > + if (hba->clk_gating.is_suspended && !flush_result) > > + goto out; > > spin_lock_irqsave(hba->host->host_lock, flags); > > goto start; > > } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel