Message-ID: <449cdcb1-c2f9-7800-9451-cb2dc289b108@linaro.org>
Date:   Thu, 23 Feb 2023 13:01:45 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [PATCH v5 01/10] interconnect: qcom: rpm: make QoS INVALID
 default, separate out driver data
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Georgi Djakov <djakov@kernel.org>,
        Evan Green <evgreen@chromium.org>,
        Jun Nie <jun.nie@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Brian Masney <masneyb@onstation.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Yassine Oudjana <y.oudjana@protonmail.com>
Cc:     Georgi Djakov <georgi.djakov@linaro.org>,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230217-topic-icc-fixes-v5-v5-0-c9a550f9fdb9@linaro.org>
 <20230217-topic-icc-fixes-v5-v5-1-c9a550f9fdb9@linaro.org>
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
In-Reply-To: <20230217-topic-icc-fixes-v5-v5-1-c9a550f9fdb9@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/02/2023 10:46, Konrad Dybcio wrote:

I find this commit log difficult to understand.

Could you reduce it down ?

> Currently, NOC_QOS_MODE_FIXED is defined as 0x0, which makes it the
> "default" option (as that's what uninitialized members of partially
> initialized structs are set to), which should really have been
> NOC_QOS_MODE_INVALID, and that's what people (wrongly) assumed was
> the case when .qos.qos_mode was not defined and what really makes
> the most sense..

"Currently NOC_QOS_MODE_FIXED is defined as 0x0 which makes it the 
default option. The default option however should be NOC_QOS_MODE_INVALID"

> That resulted in {port 0, prio 0, areq_prio 0, urg_fwd = false, rpm-voted}
> QoS being always voted for, because the code flow assumed "hey, it's fixed
> QoS, so let's just roll with whatever parameters are set" [again, set by
> partial struct initialization, as these fields were left unfilled by the
> developers]. That is of course incorrect, and on many of these platforms
> port 0 is MAS_APPS_PROC, which 9/10 times is supposed to be handled by
> the ap_owned path, not to mention the rest of the parameters may differ.
> Arguably, the APPS node is the most important one, next to EBI0..

This paragraph in particular is difficult to decipher, at least for me 
with my native Dublinese


> The modes are defined as preprocessor constants. They are not used
> anywhere outside the driver or sent to any remote processor outside
> qcom_icc_set_noc_qos(), which is easily worked around.
> Separate the type specified in driver data from the value sent to msmbus.
> Make the former an enum for better mainainability.
> 
> This is an implicit fix for every SMD RPM ICC driver that didn't
> explicitly specify NOC_QOS_MODE_INVALID on non-AP_owned nodes that
> don't have QoS settings.

It would be nice to reduce the commit log down to say three paragraphs 
of no more than three sentences each.

---
bod