Re: [V3,09/11] remoteproc: qcom: Add Hexagon based multipd rproc driver

[Manikanta Mylavarapu]

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@xxxxxxxxxx/
>
> > +		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