From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id A4F45441D; Wed, 4 Sep 2024 15:46:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725464774; cv=none; b=c3nB2crpCaXEI8AImoAQOBlFDP4Xbggwgi0Pne2KI88UMPO5xe/uuQolSK2glRxPz3L0gOO9y865wYlcLKTeCYFSO1eA52QXdEQeZ9Z6pxLQ36IS1rTokjLe1Hl110T7MYE/L+YQn/6HHvU618EbTDdm8aD9ouAV6aw5vsOfEQ4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725464774; c=relaxed/simple; bh=rSffGdixxbC/Od72+7o2GMSwmBCx1bQlb/ZLxifB6kg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=C7KHJW7aEs3Rmu/ieIQkG/aiLCGGVGZPLgGZTihsk98ZSriZmbbC+St0VmzYR9DXbt+uUCE82fWp0SA8DtzHt976ABXmfct5Yo5h9mf24UpKnaALv83ngu4lOR88dwyX6EbWrYGlvhh0+wfdcmv0CHw0TmWliktmbEdEO3I4Aao= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 3E952FEC; Wed, 4 Sep 2024 08:46:38 -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 ADE7A3F73F; Wed, 4 Sep 2024 08:46:10 -0700 (PDT) Date: Wed, 4 Sep 2024 16:46:08 +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> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Wed, Sep 04, 2024 at 04:30:24PM +0100, Cristian Marussi wrote: > On Wed, Sep 04, 2024 at 04:21:49PM +0100, Cristian Marussi wrote: > > 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) > > Hold on... I sent you a diff that does not apply probably on your tree due > to some uncomitted local work of mine...my bad...let me resend. > This one should be good...apologies ---8<--- diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index 4b7f1cbb9b04..bca9c6e4a3ab 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -373,7 +373,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 perf_dom_info *dom, struct scmi_opp *opp, unsigned int loop_idx, const struct scmi_msg_resp_perf_describe_levels *r) @@ -386,12 +386,16 @@ process_response_opp(struct device *dev, struct perf_dom_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->info.name, ret); + return ret; + } + + return 0; } -static inline void +static inline int process_response_opp_v4(struct device *dev, struct perf_dom_info *dom, struct scmi_opp *opp, unsigned int loop_idx, const struct scmi_msg_resp_perf_describe_levels_v4 *r) @@ -404,9 +408,11 @@ process_response_opp_v4(struct device *dev, struct perf_dom_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->info.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); @@ -415,13 +421,21 @@ process_response_opp_v4(struct device *dev, struct perf_dom_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->info.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 @@ -429,16 +443,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----