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=-8.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 D2368C3F2D1 for ; Mon, 2 Mar 2020 11:38:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A920020675 for ; Mon, 2 Mar 2020 11:38:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=mg.codeaurora.org header.i=@mg.codeaurora.org header.b="BeMSOf8G" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727719AbgCBLip (ORCPT ); Mon, 2 Mar 2020 06:38:45 -0500 Received: from mail27.static.mailgun.info ([104.130.122.27]:17335 "EHLO mail27.static.mailgun.info" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727470AbgCBLip (ORCPT ); Mon, 2 Mar 2020 06:38:45 -0500 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1583149124; h=Content-Transfer-Encoding: Content-Type: In-Reply-To: MIME-Version: Date: Message-ID: From: References: Cc: To: Subject: Sender; bh=SpecQ+3x7goJ87//8OV9O2P+2GcYcoXhPwlIoaAQZJk=; b=BeMSOf8GsPgqLkjix3GKSx70RNP0pENP0SkaI43ZuYPLJUWqaZ3tTKUrQGRekSfy2Yv3Uh7t oejrR0kY4iUBjHDhdycwTMzcC2Bv0HnnVE0+5NrYeUJ5VOJBcfLbd/OPXAA2/F96NhLE8Fij p4c0OlGHdMmbNDOwdntTqZrFDiE= X-Mailgun-Sending-Ip: 104.130.122.27 X-Mailgun-Sid: WyI0MWYwYSIsICJsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnIiwgImJlOWU0YSJd Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by mxa.mailgun.org with ESMTP id 5e5cf038.7fb06cd99e30-smtp-out-n01; Mon, 02 Mar 2020 11:38:32 -0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1001) id EA764C447A0; Mon, 2 Mar 2020 11:38:31 +0000 (UTC) Received: from [10.206.13.37] (blr-c-bdr-fw-01_GlobalNAT_AllZones-Outside.qualcomm.com [103.229.19.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: mkshah) by smtp.codeaurora.org (Postfix) with ESMTPSA id DD57AC433A2; Mon, 2 Mar 2020 11:38:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org DD57AC433A2 Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; spf=none smtp.mailfrom=mkshah@codeaurora.org Subject: Re: [PATCH v9 2/3] soc: qcom: rpmh: Update dirty flag only when data changes To: Evan Green Cc: Stephen Boyd , Matthias Kaehlcke , Bjorn Andersson , LKML , linux-arm-msm , Andy Gross , Doug Anderson , Rajendra Nayak , Lina Iyer , lsrao@codeaurora.org References: <1582889903-12890-1-git-send-email-mkshah@codeaurora.org> <1582889903-12890-3-git-send-email-mkshah@codeaurora.org> From: Maulik Shah Message-ID: <73906d43-cfac-59c4-5e50-51424f0c136f@codeaurora.org> Date: Mon, 2 Mar 2020 17:08:25 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2/29/2020 3:20 AM, Evan Green wrote: > On Fri, Feb 28, 2020 at 3:38 AM Maulik Shah wrote: >> Currently rpmh ctrlr dirty flag is set for all cases regardless of data >> is really changed or not. Add changes to update dirty flag when data is >> changed to newer values. >> >> Also move dirty flag updates to happen from within cache_lock and remove >> unnecessary INIT_LIST_HEAD() call and a default case from switch. >> >> Fixes: 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests") >> Signed-off-by: Maulik Shah >> Reviewed-by: Srinivas Rao L >> --- >> drivers/soc/qcom/rpmh.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >> index eb0ded0..f28afe4 100644 >> --- a/drivers/soc/qcom/rpmh.c >> +++ b/drivers/soc/qcom/rpmh.c >> @@ -133,26 +133,30 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, >> >> req->addr = cmd->addr; >> req->sleep_val = req->wake_val = UINT_MAX; >> - INIT_LIST_HEAD(&req->list); >> list_add_tail(&req->list, &ctrlr->cache); >> >> existing: >> switch (state) { >> case RPMH_ACTIVE_ONLY_STATE: >> - if (req->sleep_val != UINT_MAX) >> + if (req->sleep_val != UINT_MAX) { >> req->wake_val = cmd->data; >> + ctrlr->dirty = true; >> + } >> break; >> case RPMH_WAKE_ONLY_STATE: >> - req->wake_val = cmd->data; >> + if (req->wake_val != cmd->data) { >> + req->wake_val = cmd->data; >> + ctrlr->dirty = true; >> + } >> break; >> case RPMH_SLEEP_STATE: >> - req->sleep_val = cmd->data; >> - break; >> - default: >> + if (req->sleep_val != cmd->data) { >> + req->sleep_val = cmd->data; >> + ctrlr->dirty = true; >> + } >> break; >> } >> >> - ctrlr->dirty = true; >> unlock: >> spin_unlock_irqrestore(&ctrlr->cache_lock, flags); >> >> @@ -287,6 +291,7 @@ static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req) >> >> spin_lock_irqsave(&ctrlr->cache_lock, flags); >> list_add_tail(&req->list, &ctrlr->batch_cache); >> + ctrlr->dirty = true; > Is this fixing a case where we were not previously marking the > controller dirty but should have? Well, its not a missing case as such. Let me explain. Whenever rpmh_invalidate()  (lets call it event a) is called, controller dirty flag is set. post invalidating, the aggregator driver always sends new sleep data (event b) and wake data (event c). the last cpu going down flushes the cached data and dirty flag is unset.  (event d) hence setting dirty flag, only once during (event a) and not updating it during (event b, c) was not any issue/missing case, at least till now. However with changes to invoke rpmh_flush() whenever data is dirty, it is required to set this flag for each of these events. Otherwise as the rpmh_flush() call happened during event b would have marked as non-dirty data at the end of it, now when rpmh_flush() call happened during event c, it will not flush anything as caches are not dirty, which is not the expectation. > I notice there's a fixes tag, but it > would be helpful to add something to the commit text indicating that > you're fixing a missing case where the controller should have been > marked dirty. sure i can add details on why we need to update dirty flag everytime when touching caches. > With that fixed, you can add my tag: > > Reviewed-by: Evan Green Thanks -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation