From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 95B6C11C90 for ; Thu, 31 Aug 2023 14:21:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693491693; x=1725027693; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=7WT3EG9zGp7Fa0vDneWL1fnehQfIMEL/lXJrnlpi7YU=; b=BSxltfCqhlzvq1ULT1CgFUpYV03x7LYzzWfi5Oia4jVRF1JdPZr5f06r FfSJceRoUPlyFx5ozUtUnhJut/HqYeQ+e2rVDoNgtYr5vwAPeZWEAQ0Hl eYEfAkRSeDCUPGWzRadqTHwFBc1dM/TsnQBUCz8MQ8Ukjq5jaXp0Eb7s0 q3Ee9uOnSTmbDsVrl/VOztPymQPt0e83SivAjAzGMfDLyF66+aa6haR8K f1w1Z7V35zttRvJS7pGsYLLOpLV0xIgWFgLTKdXicPFbkPiaJ2W8mSajF Syg8TrRtJa34a4uWgzbsCaKWkK9LVNwEEmoqEMNt6mh25L8w7N9fZPTCu A==; X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="355456356" X-IronPort-AV: E=Sophos;i="6.02,217,1688454000"; d="scan'208";a="355456356" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Aug 2023 07:15:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="805013471" X-IronPort-AV: E=Sophos;i="6.02,217,1688454000"; d="scan'208";a="805013471" Received: from bcnelson-mobl2.amr.corp.intel.com (HELO [10.209.16.42]) ([10.209.16.42]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Aug 2023 07:15:11 -0700 Message-ID: Date: Thu, 31 Aug 2023 07:15:11 -0700 Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/3] platform/x86: intel_scu_ipc: Check status after timeouts in busy_loop() Content-Language: en-US To: Stephen Boyd , Mika Westerberg , Hans de Goede , Mark Gross Cc: linux-kernel@vger.kernel.org, patches@lists.linux.dev, platform-driver-x86@vger.kernel.org, Andy Shevchenko , Prashant Malani References: <20230831011405.3246849-1-swboyd@chromium.org> <20230831011405.3246849-2-swboyd@chromium.org> From: Kuppuswamy Sathyanarayanan In-Reply-To: <20230831011405.3246849-2-swboyd@chromium.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 8/30/2023 6:14 PM, Stephen Boyd wrote: > It's possible for the polling loop in busy_loop() to get scheduled away > for a long time. > > status = ipc_read_status(scu); > > if (!(status & IPC_STATUS_BUSY)) > > If this happens, then the status bit could change and this function > would never test it again after checking the jiffies against the timeout > limit. Polling code should check the condition one more time after the > timeout in case this happens. I think it is not checking the condition, but reading the status one more time to reflect the latest status. > > The read_poll_timeout() helper implements this logic, and is shorter, so > simply use that helper here. > > Cc: Prashant Malani > Cc: Andy Shevchenko > Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling") > Signed-off-by: Stephen Boyd > --- Otherwise code looks fine to me. Reviewed-by: Kuppuswamy Sathyanarayanan > drivers/platform/x86/intel_scu_ipc.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c > index 6851d10d6582..5a37becc65aa 100644 > --- a/drivers/platform/x86/intel_scu_ipc.c > +++ b/drivers/platform/x86/intel_scu_ipc.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -231,19 +232,15 @@ static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset) > /* Wait till scu status is busy */ > static inline int busy_loop(struct intel_scu_ipc_dev *scu) > { > - unsigned long end = jiffies + IPC_TIMEOUT; > - > - do { > - u32 status; > - > - status = ipc_read_status(scu); > - if (!(status & IPC_STATUS_BUSY)) > - return (status & IPC_STATUS_ERR) ? -EIO : 0; > + u8 status; > + int err; > > - usleep_range(50, 100); > - } while (time_before(jiffies, end)); > + err = read_poll_timeout(ipc_read_status, status, !(status & IPC_STATUS_BUSY), > + 100, jiffies_to_usecs(IPC_TIMEOUT), false, scu); > + if (err) > + return err; > > - return -ETIMEDOUT; > + return (status & IPC_STATUS_ERR) ? -EIO : 0; > } > > /* Wait till ipc ioc interrupt is received or timeout in 10 HZ */ -- Sathyanarayanan Kuppuswamy Linux Kernel Developer