| 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: <fedea75a-a5f1-fbe4-7f12-75bfdf9bfcf2@linaro.org>
Date: Wed, 19 Jul 2023 09:26:53 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Manikanta Mylavarapu <quic_mmanikan@...cinc.com>,
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 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?
> + 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.
> + 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