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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c298465-893a-4a18-9955-4769288ec010@kernel.org>
Date: Sun, 14 Sep 2025 15:28:29 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Xiangzhi Tang <xiangzhi.tang@...iatek.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 "irving.ch.lin" <irving-ch.lin@...iatek.com>,
 Vince-WL Liu (劉文龍) <vince-wl.liu@...iatek.com>,
 Jh Hsu (許希孜) <jh.hsu@...iatek.com>,
 Project_Global_Chrome_Upstream_Group
 <Project_Global_Chrome_Upstream_Group@...iatek.com>,
 Sirius Wang (王皓昱) <sirius.wang@...iatek.com>
Cc: Rob Herring <robh@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>,
 Bjorn Andersson <andersson@...nel.org>, linux-remoteproc@...r.kernel.org,
 Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, Jjian Zhou <Jjian.Zhou@...iatek.com>,
 Hailong Fan <Hailong.Fan@...iatek.com>
Subject: Re: [PATCH v2 2/4] remoterpoc: mediatek: vcp: Add vcp remoteproc
 driver

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