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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 74F03CD4857 for ; Wed, 4 Sep 2024 15:23:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Z1aWku2bgRM1JwkaDFxwfrFjLEN/uNdeLhfvtxysy4A=; b=o90tutrh0FwryVxgKPAUc+iSsd gOGuOR4UEW3QzyncdALd7z0G1E1nnCEurSSSHAftFFpIP/g5UVPVGusWRpyGlPJMiOyifctIPUxaJ 938HixYIEIVjimNNwh0HiXnmo4kxw8qn0vDgw9pMO+pP1bFUiP/4f+Tmkaw1u42/bfh+GV8aKwlA6 eRVHh5ji7QEuHqAo3Si3PE2jZPB1aALpI/kpLJE81is8/m2F66aKneplAHrz6p6pJ3I2LqQBWKK3X gBOQnxe8RWQDsjFXgbz14HE4FTNf9RBk3hsRCbhPrNqW3N5O9Mt8VznhCH73YL+A30aAK93WJlpX7 RmzGevpQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slrqS-00000004yZ7-3aTj; Wed, 04 Sep 2024 15:22:56 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slrpU-00000004yPz-37QF for linux-arm-kernel@lists.infradead.org; Wed, 04 Sep 2024 15:21:58 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 48D26FEC; Wed, 4 Sep 2024 08:22:20 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 772303F73F; Wed, 4 Sep 2024 08:21:52 -0700 (PDT) Date: Wed, 4 Sep 2024 16:21:49 +0100 From: Cristian Marussi To: Sibi Sankar Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, linux-kernel@vger.kernel.org, arm-scmi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, johan@kernel.org, konradybcio@kernel.org Subject: Re: [PATCH V2 2/2] firmware: arm_scmi: Skip adding bad duplicates Message-ID: References: <20240904031324.2901114-1-quic_sibis@quicinc.com> <20240904031324.2901114-3-quic_sibis@quicinc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240904031324.2901114-3-quic_sibis@quicinc.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240904_082156_912943_BE695DE2 X-CRM114-Status: GOOD ( 17.50 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 04, 2024 at 08:43:24AM +0530, Sibi Sankar wrote: > Ensure that the bad duplicates reported by the platform firmware doesn't > get added to the opp-tables. > Hi Sibi, so if the idea is to make the code more robust when FW sends BAD duplicates, you necessarily need to properly drop opps in opp_count too. One other option would be to just loop with xa_for_each BUT opp_count is used in a number of places...so first of all let's try drop count properly. Can you try this patch down below, instead of your patch. If it solves, I will send a patch (after testing it a bit more :D) Thanks, Cristian P.S.: thanks for spotting this, I forgot NOT to trust FW replies as usual :P --->8--- diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index 397a39729e29..cbac29792d1e 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -340,7 +340,7 @@ static int iter_perf_levels_update_state(struct scmi_iterator_state *st, return 0; } -static inline void +static inline int process_response_opp(struct device *dev, struct scmi_perf_domain_info *dom, struct scmi_opp *opp, unsigned int loop_idx, const struct scmi_msg_resp_perf_describe_levels *r) @@ -353,12 +353,16 @@ process_response_opp(struct device *dev, struct scmi_perf_domain_info *dom, le16_to_cpu(r->opp[loop_idx].transition_latency_us); ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL); - if (ret) + if (ret) { dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n", opp->perf, dom->name, ret); + return ret; + } + + return 0; } -static inline void +static inline int process_response_opp_v4(struct device *dev, struct scmi_perf_domain_info *dom, struct scmi_opp *opp, unsigned int loop_idx, const struct scmi_msg_resp_perf_describe_levels_v4 *r) @@ -371,9 +375,11 @@ process_response_opp_v4(struct device *dev, struct scmi_perf_domain_info *dom, le16_to_cpu(r->opp[loop_idx].transition_latency_us); ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL); - if (ret) + if (ret) { dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n", opp->perf, dom->name, ret); + return ret; + } /* Note that PERF v4 reports always five 32-bit words */ opp->indicative_freq = le32_to_cpu(r->opp[loop_idx].indicative_freq); @@ -382,13 +388,21 @@ process_response_opp_v4(struct device *dev, struct scmi_perf_domain_info *dom, ret = xa_insert(&dom->opps_by_idx, opp->level_index, opp, GFP_KERNEL); - if (ret) + if (ret) { dev_warn(dev, "Failed to add opps_by_idx at %d for %s - ret:%d\n", opp->level_index, dom->name, ret); + /* Cleanup by_lvl too */ + xa_erase(&dom->opps_by_lvl, opp->perf); + + return ret; + } + hash_add(dom->opps_by_freq, &opp->hash, opp->indicative_freq); } + + return 0; } static int @@ -396,16 +410,22 @@ iter_perf_levels_process_response(const struct scmi_protocol_handle *ph, const void *response, struct scmi_iterator_state *st, void *priv) { + int ret; struct scmi_opp *opp; struct scmi_perf_ipriv *p = priv; opp = &p->perf_dom->opp[st->desc_index + st->loop_idx]; if (PROTOCOL_REV_MAJOR(p->version) <= 0x3) - process_response_opp(ph->dev, p->perf_dom, opp, st->loop_idx, - response); + ret = process_response_opp(ph->dev, p->perf_dom, opp, + st->loop_idx, response); else - process_response_opp_v4(ph->dev, p->perf_dom, opp, st->loop_idx, - response); + ret = process_response_opp_v4(ph->dev, p->perf_dom, opp, + st->loop_idx, response); + + /* Skip BAD duplicates received from firmware */ + if (ret) + return ret == -EBUSY ? 0 : ret; + p->perf_dom->opp_count++; dev_dbg(ph->dev, "Level %d Power %d Latency %dus Ifreq %d Index %d\n", ---8<---