Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: mdtipton=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org D13E2C433D3
Subject: Re: [PATCH] interconnect: Aggregate bandwidth votes for unused nodes
 in sync_state()
To:     Odelu Kukatla <okukatla@codeaurora.org>, georgi.djakov@linaro.org,
        bjorn.andersson@linaro.org, evgreen@google.com,
        Georgi Djakov <djakov@kernel.org>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     sboyd@kernel.org, sibis@codeaurora.org, saravanak@google.com,
        seansw@qti.qualcomm.com, elder@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org
References: <1624122509-17508-1-git-send-email-okukatla@codeaurora.org>
From:   Mike Tipton <mdtipton@codeaurora.org>
Message-ID: <f93ad53b-f413-f8dc-2068-2c217cbed543@codeaurora.org>
Date:   Fri, 25 Jun 2021 14:20:34 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1624122509-17508-1-git-send-email-okukatla@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/19/2021 10:08 AM, Odelu Kukatla wrote:
> When removing the initial bandwidth votes in sync_state(), make sure
> to call the aggregate() function for nodes which don't have any
> clients yet. aggregate_requests() does not invoke aggregate()
> for unused nodes.
> 
> Signed-off-by: Odelu Kukatla <okukatla@codeaurora.org>
> ---
>   drivers/interconnect/core.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 8a1e70e..1d9a00a 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1106,7 +1106,16 @@ void icc_sync_state(struct device *dev)
>   		dev_dbg(p->dev, "interconnect provider is in synced state\n");
>   		list_for_each_entry(n, &p->nodes, node_list) {
>   			if (n->init_avg || n->init_peak) {
> -				aggregate_requests(n);
> +				if (hlist_empty(&n->req_list)) {
> +					if (p->pre_aggregate)
> +						p->pre_aggregate(n);
> +
> +					p->aggregate(n, 0, 0, 0, &n->avg_bw,
> +						&n->peak_bw);
> +				} else {
> +					aggregate_requests(n);
> +				}
> +
>   				p->set(n, n);
>   			}
>   		}
> 

I ran into this issue myself last week. There is an alternative fix that 
I think is a little cleaner. The reason we need the aggregate() call 
here is that the icc-rpmh providers only add BCMs to the commit list in 
the aggregate() callback. If aggregate() isn't called, then we don't 
commit anything to HW. But, if we instead add BCMs to the commit list in 
pre_aggregate(), then the existing aggregate_requests() call is 
sufficient, since that always calls pre_aggregate() even when req_list 
is empty. That means we'd need to update icc_node_add() to call 
pre_aggregate() before aggregate(), otherwise we wouldn't commit the 
initial floors. But I think always calling pre_aggregate() before 
aggregate() is a reasonable requirement.

I've posted these changes in a patch series along with a separate 
sync-state-related fix. The solutions could co-exist, but they are 
largely redundant.