Re: [PATCH v2 2/4] remoterpoc: mediatek: vcp: Add vcp remoteproc driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/09/2025 14:29, Xiangzhi Tang wrote:
> +
> +static const struct rproc_ops mtk_vcp_ops = {
> +	.load		= mtk_vcp_load,
> +	.start		= mtk_vcp_start,
> +	.stop		= mtk_vcp_stop,
> +};
> +
> +static int vcp_multi_core_init(struct platform_device *pdev,
> +			       struct mtk_vcp_of_cluster *vcp_cluster,
> +			       enum vcp_core_id core_id)
> +{
> +	int ret;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "mtk,vcp-core-twohart",
> +				   &vcp_cluster->twohart[core_id]);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to twohart property\n");
> +		return ret;
> +	}
> +	ret = of_property_read_u32(pdev->dev.of_node, "mtk,core-sram-offset",

Undocumented ABI, if you ever bothered to test your DTS you would see
clear warning.

> +				   &vcp_cluster->sram_offset[core_id]);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to sram-offset property\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct mtk_vcp_device *vcp_rproc_init(struct platform_device *pdev,
> +					     struct mtk_vcp_of_cluster *vcp_cluster)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev_of_node(dev);
> +	struct device_node *child;
> +	struct platform_device *cpdev;
> +	struct mtk_vcp_device *vcp;
> +	struct rproc *rproc;
> +	const struct mtk_vcp_of_data *vcp_of_data;
> +	u32 core_id;
> +	int ret;
> +
> +	vcp_of_data = of_device_get_match_data(dev);
> +	rproc = devm_rproc_alloc(dev, np->name, &mtk_vcp_ops,
> +				 vcp_of_data->platdata.fw_name,
> +				 sizeof(struct mtk_vcp_device));
> +	if (!rproc) {
> +		dev_err(dev, "unable to allocate remoteproc\n");

No, you never do this.

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	vcp  = rproc->priv;
> +	vcp->rproc = rproc;
> +	vcp->pdev = pdev;
> +	vcp->dev = dev;
> +	vcp->ops = &vcp_of_data->ops;
> +	vcp->platdata = &vcp_of_data->platdata;
> +	vcp->vcp_cluster = vcp_cluster;
> +
> +	rproc->auto_boot = vcp_of_data->platdata.auto_boot;
> +	rproc->sysfs_read_only = vcp_of_data->platdata.sysfs_read_only;
> +	mutex_init(&vcp->vcp_cluster->vcp_feature_mutex);
> +	mutex_init(&vcp->vcp_cluster->vcp_pw_clk_mutex);
> +	platform_set_drvdata(pdev, vcp);
> +
> +	ret = vcp_reserve_memory_ioremap(vcp);
> +	if (ret) {
> +		dev_err(dev, "vcp_reserve_memory_ioremap failed ret = %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	core_id = 0;
> +	for_each_available_child_of_node(np, child) {
> +		if (of_device_is_compatible(child, "mediatek,vcp-core")) {
> +			cpdev = of_find_device_by_node(child);
> +			if (!cpdev) {
> +				ret = -ENODEV;
> +				dev_err(dev, "Not found platform device for core\n");

1. That's probe path, so why are you using this old style? The syntax is
return dev_err_probe and I am dissapointed that your internal review did
not ask for that.

2. You just leak here everywhere device.

This patchset has trivial errors, which should be spotted easily by
internal review. Plus your other patch ignored EXITING feedback, so what
is even point of posting this if you are just going to ignore us?

Please confirm that you received extensive internal review before
posting this?

Mediatek posts so many patches, I complained so many times about poor
quality, things a bit improved two years ago and for a year the quality
deteriorated and Mediatek posts poor code again. It's huge company with
huge resources, so I do not understand why trivial bugs like this cannot
be found inside first, to offload the community reviewers.

NAK

Best regards,
Krzysztof