linux-kernel - Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE controller 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: <4783c1aa-d918-4194-90d7-ebc69ddbb789@kernel.org>
Date: Tue, 13 May 2025 10:08:39 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Piotr Kubik <piotr.kubik@...ran.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>,
 Kory Maincent <kory.maincent@...tlin.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE controller
 driver

On 13/05/2025 00:06, Piotr Kubik wrote:
> +/* Parse pse-pis subnode into chan array of si3474_priv */
> +static int si3474_get_of_channels(struct si3474_priv *priv)
> +{
> +	struct device_node *pse_node, *node;
> +	struct pse_pi *pi;
> +	u32 pi_no, chan_id;
> +	s8 pairset_cnt;
> +	s32 ret = 0;
> +
> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
> +	if (!pse_node) {
> +		dev_warn(&priv->client[0]->dev,
> +			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_child_of_node(pse_node, node) {

Use scoped variant. One cleanup less.

> +		if (!of_node_name_eq(node, "pse-pi"))
> +			continue;

...

> +
> +	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +	fw_version = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",

dev_dbg or just drop. Drivers should be silent on success.

> +		 ret, fw_version);
> +
> +	priv->client[0] = client;
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "slave",
> +						   priv->client[0]->addr + 1);
> +	if (IS_ERR(priv->client[1]))
> +		return PTR_ERR(priv->client[1]);
> +
> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
> +	if (ret < 0) {
> +		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
> +		goto out_err_slave;
> +	}
> +
> +	if (ret != SI3474_DEVICE_ID) {
> +		dev_err(&priv->client[1]->dev,
> +			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
> +		ret = -ENXIO;
> +		goto out_err_slave;
> +	}
> +
> +	priv->np = dev->of_node;
> +	priv->pcdev.owner = THIS_MODULE;
> +	priv->pcdev.ops = &si3474_ops;
> +	priv->pcdev.dev = dev;
> +	priv->pcdev.types = ETHTOOL_PSE_C33;
> +	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
> +
> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
> +	if (ret) {

No need for {}

> +		return dev_err_probe(dev, ret,
> +				     "Failed to register PSE controller\n");

No cleanup here? That's odd.

> +	}
> +
> +	return ret;

return 0

> +
> +out_err_slave:
> +	i2c_unregister_device(priv->client[1]);
> +	return ret;
> +}
> +
> +static void si3474_i2c_remove(struct i2c_client *client)
> +{
> +	struct si3474_priv *priv = i2c_get_clientdata(client);
> +
> +	i2c_unregister_device(priv->client[1]);

So you first unregister i2c device and then unregister pse controller.
Feels like possible issues, difficult to debug.... Use devm reset
wrapper for that.

Best regards,
Krzysztof