linux-kernel - Re: [PATCHv2 3/6] hwmon: (socfpga) Add hardware monitoring support on SoCFPGA platforms

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: <9459547a-0ff9-9972-602d-3098b616602b@linaro.org>
Date:   Tue, 9 May 2023 08:57:26 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     dinh.nguyen@...ux.intel.com, linux-hwmon@...r.kernel.org
Cc:     dinguyen@...nel.org, devicetree@...r.kernel.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-kernel@...r.kernel.org, jdelvare@...e.com,
        linux@...ck-us.net,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCHv2 3/6] hwmon: (socfpga) Add hardware monitoring support on
 SoCFPGA platforms

On 08/05/2023 23:28, dinh.nguyen@...ux.intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@...ux.intel.com>
> 
> The driver supports 64-bit SoCFPGA platforms for temperature and voltage
> reading using the platform's SDM(Secure Device Manager). The driver
> also uses the Stratix10 Service layer driver.
> 
> This driver only supports OF SoCFPGA 64-bit platforms.
> 

(...)

> +static int socfpga_probe_child_from_dt(struct device *dev,
> +				       struct device_node *child,
> +				       struct socfpga_hwmon_priv *priv)
> +{
> +	struct device_node *grandchild;
> +	const char *label;
> +	const char *type;
> +	u32 val;
> +	int ret;
> +
> +	if (of_property_read_string(child, "name", &type))
> +		return dev_err_probe(dev, -EINVAL, "No type for %pOF\n", child);
> +
> +	for_each_child_of_node(child, grandchild) {
> +		ret = of_property_read_u32(grandchild, "reg", &val);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "missing reg property of %pOF\n",
> +					     grandchild);

Where do you drop child reference?

> +
> +		ret = of_property_read_string(grandchild, "label", &label);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "missing label propoerty of %pOF\n",
> +					     grandchild);
> +		ret = socfpga_add_channel(dev, type, val, label, priv);
> +		if (ret == -ENOSPC)
> +			return dev_err_probe(dev, ret, "too many channels, only %d supported\n",
> +					     SOCFPGA_HWMON_MAXSENSORS);
> +	}
> +	return 0;
> +}
> +
> +static int socfpga_probe_from_dt(struct device *dev,
> +				 struct socfpga_hwmon_priv *priv)
> +{
> +	const struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int ret = 0;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = socfpga_probe_child_from_dt(dev, child, priv);
> +		if (ret)
> +			break;
> +	}
> +	of_node_put(child);

Hm, and if the loop does not break, is this still correct?

> +
> +	return ret;
> +}
> +
> +static int socfpga_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct socfpga_hwmon_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client.dev = dev;
> +	priv->client.priv = priv;
> +
> +	ret = socfpga_probe_from_dt(dev, priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to probe from device tree\n");
> +
> +	mutex_init(&priv->lock);
> +	init_completion(&priv->completion);
> +	priv->chan = stratix10_svc_request_channel_byname(&priv->client,
> +							  SVC_CLIENT_HWMON);
> +	if (IS_ERR(priv->chan))
> +		return dev_err_probe(dev, PTR_ERR(priv->chan),
> +				     "couldn't get service channel %s\n",
> +				     SVC_CLIENT_RSU);
> +
> +	priv->hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpgahwmon",
> +							       priv,
> +							       &socfpga_chip_info,
> +							       NULL);
> +	if (IS_ERR(priv->hwmon_dev))
> +		return PTR_ERR(priv->hwmon_dev);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int socfpga_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct socfpga_hwmon_priv *priv = platform_get_drvdata(pdev);
> +
> +	hwmon_device_unregister(priv->hwmon_dev);

Please test it. I am pretty sure you will have double free here.

> +	stratix10_svc_free_channel(priv->chan);
> +	return 0;
> +}
> +
> +static const struct of_device_id socfpga_of_match[] = {
> +	{ .compatible = "intel,socfpga-hwmon" },
> +	{ .compatible = "intel,socfpga-agilex-hwmon" },
> +	{ .compatible = "intel,socfpga-n5x-hwmon" },
> +	{ .compatible = "intel,socfpga-stratix10-hwmon" },

These are all compatible, so why having 4 entries?

Best regards,
Krzysztof