linux-kernel - Re: [PATCH 2/4] interconnect: qcom: rpm: Set QoS parameters regardless of RPM bw setting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e2bdaec-b7ba-0474-8b80-8901fcc87a0f@linaro.org>
Date:   Tue, 3 Jan 2023 23:43:51 +0000
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org, andersson@...nel.org,
        agross@...nel.org, krzysztof.kozlowski@...aro.org
Cc:     marijn.suijten@...ainline.org, Georgi Djakov <djakov@...nel.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        AngeloGioacchino Del Regno <kholk11@...il.com>
Subject: Re: [PATCH 2/4] interconnect: qcom: rpm: Set QoS parameters
 regardless of RPM bw setting

On 03/01/2023 17:30, Konrad Dybcio wrote:
> QoS parameters and RPM bandwidth requests are wholly separate. Setting one
> should only depend on the description of the interconnect node and not
> whether the other is present. If we vote through RPM, QoS parameters
> should be set so that the bus controller can make better decisions.

Is that true ?

> If we don't vote through RPM, QoS parameters should be set regardless,
> as we're requesting additional bandwidth by setting the interconnect
> clock rates.
> 
> The Fixes tag references the commit in which this logic was added, it
> has since been shuffled around to a different file, but it's the one
> where it originates from.
> 
> Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 06e0fee547ab..a190a0a839c8 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -252,8 +252,10 @@ static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
>   		ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>   		if (ret)
>   			return ret;
> -	} else if (qn->qos.qos_mode != -1) {
> -		/* set bandwidth directly from the AP */
> +	}
> +
> +	if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
> +		/* Set QoS params from the AP */
>   		ret = qcom_icc_qos_set(n, sum_bw);
>   		if (ret)
>   			return ret;

Taking the example of

static struct qcom_icc_node bimc_snoc_slv = {
         .name = "bimc_snoc_slv",
         .id = MSM8939_BIMC_SNOC_SLV,
         .buswidth = 16,
         .mas_rpm_id = -1,
         .slv_rpm_id = 2,
         .num_links = ARRAY_SIZE(bimc_snoc_slv_links),
         .links = bimc_snoc_slv_links,
};

#define NOC_QOS_MODE_INVALID -1
ap_owned == false
qos_mode == NOC_QOS_MODE_FIXED


if (!qn->qos.ap_owned) {
	/* bod: this will run */
	/* send bandwidth request message to the RPM processor */
	ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
	if (ret)
		return ret;
} else if (qn->qos.qos_mode != -1) {
	/* bod: this will not run */
	/* set bandwidth directly from the AP */
	ret = qcom_icc_qos_set(n, sum_bw);
	if (ret)
		return ret;
}

and your proposed change

if (!qn->qos.ap_owned) {
	/* bod: this will run */
	/* send bandwidth request message to the RPM processor */
	ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
	if (ret)
		return ret;
}

if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
	/* bod: this will run */
	/* set bandwidth directly from the AP */
	ret = qcom_icc_qos_set(n, sum_bw);
	if (ret)
		return ret;
}

however if we look downstream we have the concept of ap_owned

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L194

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L208

In simple terms
if (node_info->ap_owned) {
	ret = fabdev->noc_ops.set_bw(node_info,
									} else {
	ret = send_rpm_msg(node_device);
}

I agree your code does what it says on the tin but, whats the overall 
justification to depart from the downstream logic ?

---
bod