| 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
| ||
|
Message-ID: <00cb0942-08cb-4cd2-b84a-bc265686eae2@quicinc.com>
Date: Fri, 22 Nov 2024 12:48:57 +0530
From: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Krzysztof Kozlowski <krzk@...nel.org>, Georgi Djakov <djakov@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Odelu Kukatla
<quic_okukatla@...cinc.com>,
Mike Tipton <quic_mdtipton@...cinc.com>,
"Sibi
Sankar" <quic_sibis@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <linux-pm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V5 3/4] interconnect: qcom: Add EPSS L3 support on SA8775P
On 11/22/2024 3:44 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>>> Add Epoch Subsystem (EPSS) L3 interconnect provider on
>>>> SA8775P SoCs with multiple device support.
>>>>
>>>
>>>
>>> ...
>>>
>>>> -DEFINE_QNODE(osm_l3_master, OSM_L3_MASTER_NODE, 16, OSM_L3_SLAVE_NODE);
>>>> -DEFINE_QNODE(osm_l3_slave, OSM_L3_SLAVE_NODE, 16);
>>>> +DEFINE_QNODE(osm_l3_master, 16, osm_l3_slave);
>>>> +DEFINE_QNODE(osm_l3_slave, 16);
>>>>
>>>> -static const struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>>>> +static struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>>>> [MASTER_OSM_L3_APPS] = &osm_l3_master,
>>>> [SLAVE_OSM_L3] = &osm_l3_slave,
>>>> };
>>>>
>>>> -DEFINE_QNODE(epss_l3_master, OSM_L3_MASTER_NODE, 32, OSM_L3_SLAVE_NODE);
>>>> -DEFINE_QNODE(epss_l3_slave, OSM_L3_SLAVE_NODE, 32);
>>>> +DEFINE_QNODE(epss_l3_master, 32, epss_l3_slave);
>>>> +DEFINE_QNODE(epss_l3_slave, 32);
>>>>
>>>> -static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>> +static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>>>
>>>
>>> I think dropping const makes the code worse, not better. Commit msg does
>>> not explain all these changes and I could not figure out the intention
>>> (except modifying but that's just obvious).
>>
>> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
>> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
>> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA
>> and the node name is made dynamic (nodename@...ress).
>> Updating node id and node name is not possible with const.
>
> Has this been described in the commit message? No. Have you had similar
> questions since v1? Yes. What does that tell us?
I will update the commit message in the next patch revision.
>
> Also, while we are at it. Please fix your email client settings to wrap
> message body text on some sensible (72-75) width.
Thanks for suggesting!
>
>>>
>>>
>>>
>>>> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
>>>> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>>>> };
>>>> @@ -123,6 +125,19 @@ static const struct qcom_osm_l3_desc epss_l3_l3_vote = {
>>>> .reg_perf_state = EPSS_REG_L3_VOTE,
>>>> };
>>>>
>>>> +static u16 get_node_id_by_name(const char *node_name,
>>>> + const struct qcom_osm_l3_desc *desc)
>>>> +{
>>>> + struct qcom_osm_l3_node *const *nodes = desc->nodes;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < desc->num_nodes; i++) {
>>>> + if (!strcmp(nodes[i]->name, node_name))
>>>> + return nodes[i]->id;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int qcom_osm_l3_set(struct icc_node *src, struct icc_node *dst)
>>>> {
>>>> struct qcom_osm_l3_icc_provider *qp;
>>>> @@ -164,10 +179,11 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> const struct qcom_osm_l3_desc *desc;
>>>> struct icc_onecell_data *data;
>>>> struct icc_provider *provider;
>>>> - const struct qcom_osm_l3_node * const *qnodes;
>>>> + struct qcom_osm_l3_node * const *qnodes;
>>>> struct icc_node *node;
>>>> size_t num_nodes;
>>>> struct clk *clk;
>>>> + u64 addr;
>>>> int ret;
>>>>
>>>> clk = clk_get(&pdev->dev, "xo");
>>>> @@ -188,6 +204,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> if (!qp)
>>>> return -ENOMEM;
>>>>
>>>> + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> qp->base = devm_platform_ioremap_resource(pdev, 0);
>>>> if (IS_ERR(qp->base))
>>>> return PTR_ERR(qp->base);
>>>> @@ -242,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>
>>>> icc_provider_init(provider);
>>>>
>>>> + /* Allocate unique id for qnodes */
>>>> + for (i = 0; i < num_nodes; i++)
>>>> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
>>>> +
>>>> for (i = 0; i < num_nodes; i++) {
>>>> - size_t j;
>>>> + char *node_name;
>>>> + size_t j, len;
>>>>
>>>> node = icc_node_create(qnodes[i]->id);
>>>> if (IS_ERR(node)) {
>>>> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> goto err;
>>>> }
>>>>
>>>> - node->name = qnodes[i]->name;
>>>> + /* len = strlen(node->name) + @ + 8 (base-address) + NULL */
>>>> + len = strlen(qnodes[i]->name) + OSM_NODE_NAME_SUFFIX_SIZE;
>>>> + node_name = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
>>>> + if (!node_name) {
>>>> + ret = -ENOMEM;
>>>> + goto err;
>>>> + }
>>>> +
>>>> + snprintf(node_name, len, "%s@...llx", qnodes[i]->name, addr);
>>>> + node->name = node_name;
>>>
>>>
>>> Why the node name becomes dynamic?
>>>
>>> Best regards,
>>> Krzysztof
>>
>
Powered by blists - more mailing lists