| 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: <a996fcc8-381d-6534-8b08-549c4ecfb748@quicinc.com>
Date: Fri, 21 Jul 2023 12:44:22 +0530
From: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <mathieu.poirier@...aro.org>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <mturquette@...libre.com>,
<sboyd@...nel.org>, <quic_eberman@...cinc.com>, <kvalo@...nel.org>,
<loic.poulain@...aro.org>, <linux-arm-msm@...r.kernel.org>,
<linux-remoteproc@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>
CC: <quic_srichara@...cinc.com>, <quic_sjaganat@...cinc.com>,
<quic_kathirav@...cinc.com>, <quic_anusha@...cinc.com>
Subject: Re: [V3,09/11] remoteproc: qcom: Add Hexagon based multipd rproc
driver
On 7/19/2023 12:56 PM, Krzysztof Kozlowski wrote:
> On 18/07/2023 14:04, Manikanta Mylavarapu wrote:
>> It adds support to bring up remoteproc's on multipd model.
>> Pd means protection domain. It's similar to process in Linux.
>> Here QDSP6 processor runs each wifi radio functionality on a
>> separate process. One process can't access other process
>> resources, so this is termed as PD i.e protection domain.
>>
>> Here we have two pd's called root and user pd. We can correlate
>> Root pd as root and user pd as user in linux. Root pd has more
>> privileges than user pd. Root will provide services to user pd.
>>
>
>
>> +static int q6_get_inbound_irq(struct qcom_q6v5 *q6,
>> + struct platform_device *pdev,
>> + const char *int_name,
>> + int index, int *pirq,
>> + irqreturn_t (*handler)(int irq, void *data))
>> +{
>> + int ret, irq;
>> + char *interrupt, *tmp = (char *)int_name;
>> + struct q6_wcss *wcss = q6->rproc->priv;
>> +
>> + irq = platform_get_irq(pdev, index);
>> + if (irq < 0) {
>> + if (irq != -EPROBE_DEFER)
>> + dev_err_probe(&pdev->dev, irq,
>> + "failed to retrieve %s IRQ: %d\n",
>> + int_name, irq);
>
> Wait, what? This does not make any sense. dev_err_probe is to replace
> all this dance. return dev_err_probe which I explicitly asked last time:
>
> https://lore.kernel.org/all/2061a641-4b97-1aa6-27cd-99f01a785033@linaro.org/
>
>> + return irq;
>> + }
>> +
>> + *pirq = irq;
>> +
>> + interrupt = devm_kzalloc(&pdev->dev, BUF_SIZE, GFP_KERNEL);
>> + if (!interrupt)
>> + return -ENOMEM;
>> +
>> + snprintf(interrupt, BUF_SIZE, "q6v5_wcss_userpd%d_%s", wcss->pd_asid, tmp);
>> +
>> + ret = devm_request_threaded_irq(&pdev->dev, *pirq,
>> + NULL, handler,
>> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> + interrupt, q6);
>> + if (ret) {
>> + dev_err_probe(&pdev->dev, ret,
>> + "failed to acquire %s irq\n", interrupt);
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> +static int q6_get_outbound_irq(struct qcom_q6v5 *q6,
>> + struct platform_device *pdev,
>> + const char *int_name)
>> +{
>> + struct qcom_smem_state *tmp_state;
>> + unsigned bit;
>> +
>> + tmp_state = qcom_smem_state_get(&pdev->dev, int_name, &bit);
>> + if (IS_ERR(tmp_state)) {
>> + dev_err_probe(&pdev->dev, IS_ERR(tmp_state),
>> + "failed to acquire %s state\n", int_name);
>> + return PTR_ERR(tmp_state);
>
> So it is everywhere...
>
>
>> + }
>> +
>> + if (!strcmp(int_name, "stop")) {
>> + q6->state = tmp_state;
>> + q6->stop_bit = bit;
>> + } else if (!strcmp(int_name, "spawn")) {
>> + q6->spawn_state = tmp_state;
>> + q6->spawn_bit = bit;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int init_irq(struct qcom_q6v5 *q6,
>> + struct platform_device *pdev, struct rproc *rproc,
>> + int crash_reason, const char *load_state,
>> + void (*handover)(struct qcom_q6v5 *q6))
>> +{
>> + int ret;
>> + struct q6_wcss *wcss = rproc->priv;
>> +
>> + q6->rproc = rproc;
>> + q6->dev = &pdev->dev;
>> + q6->crash_reason = crash_reason;
>> + q6->handover = handover;
>> +
>> + init_completion(&q6->start_done);
>> + init_completion(&q6->stop_done);
>> + init_completion(&q6->spawn_done);
>> +
>> + ret = q6_get_outbound_irq(q6, pdev, "stop");
>> + if (ret)
>> + return ret;
>> +
>> + ret = q6_get_outbound_irq(q6, pdev, "spawn");
>> + if (ret)
>> + return ret;
>> +
>> + /* Get pd_asid to prepare interrupt names */
>> + wcss->pd_asid = qcom_get_pd_asid(rproc);
>> +
>> + ret = q6_get_inbound_irq(q6, pdev, "fatal", 0, &q6->fatal_irq,
>> + q6v5_fatal_interrupt);
>> + if (ret)
>> + return ret;
>> +
>> + ret = q6_get_inbound_irq(q6, pdev, "ready", 1, &q6->ready_irq,
>> + q6v5_ready_interrupt);
>> + if (ret)
>> + return ret;
>> +
>> + ret = q6_get_inbound_irq(q6, pdev, "stop-ack", 3, &q6->stop_irq,
>> + q6v5_stop_interrupt);
>> + if (ret)
>> + return ret;
>> +
>> + ret = q6_get_inbound_irq(q6, pdev, "spawn-ack", 2, &q6->spawn_irq,
>> + q6v5_spawn_interrupt);
>> + if (ret)
>> + return ret;
>> + return 0;
>> +}
>> +
>> +static void q6_release_resources(struct platform_device *pdev)
>> +{
>> + struct rproc *upd_rproc;
>> + struct device_node *upd_np;
>> + struct platform_device *upd_pdev;
>> +
>> + /* Release userpd resources */
>> + for_each_available_child_of_node(pdev->dev.of_node, upd_np) {
>
> You should not iterate over OF to get devices to unregister. What if you
> have more nodes than before because of overlay?
>
>
Yeah correct, i won't iterate over OF to get devices.
>> + upd_pdev = of_find_device_by_node(upd_np);
>> + if (!upd_pdev)
>> + continue;
>> +
>> + upd_rproc = platform_get_drvdata(upd_pdev);
>> + if (!upd_rproc) {
>> + platform_device_unregister(upd_pdev);
>> + continue;
>> + }
>> +
>> + rproc_del(upd_rproc);
>> + rproc_free(upd_rproc);
>> + }
>> +}
>> +
>> +static int q6_register_userpd(struct platform_device *pdev)
>> +{
>> + struct q6_wcss *wcss;
>> + struct rproc *rproc = NULL;
>> + int ret;
>> + struct device_node *userpd_np;
>> + struct platform_device *userpd_pdev;
>> + const char *firmware_name = NULL;
>> +
>> + for_each_available_child_of_node(pdev->dev.of_node, userpd_np) {
>> + ret = of_property_read_string(userpd_np, "firmware-name",
>> + &firmware_name);
>> + if (ret < 0)
>> + continue;
>> +
>> + dev_info(&pdev->dev, "%s node found\n", userpd_np->name);
>> +
>> + userpd_pdev = of_platform_device_create(userpd_np,
>> + userpd_np->name,
>> + &pdev->dev);
>> + if (!userpd_pdev) {
>> + ret = -ENODEV;
>> + dev_err_probe(&pdev->dev, ret,
>> + "failed to create %s platform device\n",
>> + userpd_np->name);
>> + goto release_resource;
>> + }
>> + userpd_pdev->dev.driver = pdev->dev.driver;
>> + rproc = rproc_alloc(&userpd_pdev->dev, userpd_pdev->name,
>> + &wcss_ops, firmware_name,
>> + sizeof(*wcss));
>> + if (!rproc) {
>> + ret = -ENOMEM;
>> + goto release_resource;
>> + }
>> +
>> + wcss = rproc->priv;
>> + wcss->dev = &userpd_pdev->dev;
>> +
>> + ret = q6_alloc_memory_region(wcss);
>> + if (ret)
>
> How do you release the resource allocated in rproc_alloc() for this
> node? drvdata is not set, so your cleanup function will skip it.
>
Yeah correct. I will free the rproc pointer and call cleanup API
to unregister platform device.
Is it fine ?
Thanks & Regards,
Manikanta.
>> + goto release_resource;
>> +
>> + ret = init_irq(&wcss->q6, userpd_pdev, rproc,
>> + WCSS_CRASH_REASON, NULL, NULL);
>> + if (ret)
>> + goto release_resource;
>> +
>> + rproc->auto_boot = false;
>> + ret = rproc_add(rproc);
>> + if (ret)
>> + goto release_resource;
>> +
>> + platform_set_drvdata(userpd_pdev, rproc);
>> + qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, userpd_pdev->name);
>> + }
>> + return 0;
>> +
>> +release_resource:
>> + q6_release_resources(pdev);
>> + return ret;
>> +}
>> +
>> +static int q6_wcss_probe(struct platform_device *pdev)
>> +{
>> + const struct wcss_data *desc;
>> + struct q6_wcss *wcss;
>> + struct rproc *rproc;
>> + int ret;
>> + char *subdev_name;
>> + const char **firmware;
>> +
>> + desc = of_device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + firmware = devm_kcalloc(&pdev->dev, MAX_FIRMWARE,
>> + sizeof(*firmware), GFP_KERNEL);
>> + if (!firmware)
>> + return -ENOMEM;
>> +
>> + ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name",
>> + firmware, MAX_FIRMWARE);
>> + if (ret < 0)
>> + return ret;
>> +
>> + rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops,
>> + firmware[0], sizeof(*wcss));
>> + if (!rproc)
>> + return -ENOMEM;
>> +
>> + wcss = rproc->priv;
>> + wcss->dev = &pdev->dev;
>> + wcss->desc = desc;
>> + wcss->firmware = firmware;
>> +
>> + ret = q6_alloc_memory_region(wcss);
>> + if (ret)
>> + goto free_rproc;
>> +
>> + ret = desc->init_irq(&wcss->q6, pdev, rproc,
>> + desc->crash_reason_smem, NULL, NULL);
>> + if (ret)
>> + goto free_rproc;
>> +
>> + if (desc->glink_subdev_required)
>> + qcom_add_glink_subdev(rproc, &wcss->glink_subdev, desc->ssr_name);
>> +
>> + subdev_name = (char *)(desc->ssr_name ? desc->ssr_name : pdev->name);
>
> Wrong cast. Why are you dropping const? That's a bug.
>
>> + qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, subdev_name);
>> +
>> + rproc->auto_boot = false;
>> + ret = rproc_add(rproc);
>> + if (ret)
>> + goto free_rproc;
>> +
>> + platform_set_drvdata(pdev, rproc);
>> +
>> + ret = q6_register_userpd(pdev);
>> + if (ret) {
>> + dev_err_probe(&pdev->dev, ret, "Failed to register userpd\n");
>> + return ret;
>
> return dev_err_probe
>
>> + }
>> +
>> + return 0;
>> +
>> +free_rproc:
>> + rproc_free(rproc);
>> +
>> + return ret;
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists