netdev - Re: [EXTERNAL]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: <45525374-413a-4381-8c73-4f708c72ad15@adtran.com>
Date: Thu, 15 May 2025 15:20:47 +0000
From: Piotr Kubik <piotr.kubik@...ran.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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: [EXTERNAL]Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE
 controller driver

Thanks Krzysztof for your review,

> 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.

good point

> 
> 
>> +		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.

Is there any rule for this I'm not aware of? 
I'd like to know that device is present and what versions it runs just by looking into dmesg.
This approach is similar to other drivers, all current PSE drivers log it this way.

> 
>> +		 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.
> 

Indeed, will fix

>> +	}
>> +
>> +	return ret;
> 
> return 0
> 

This is actually not needed. return above will return

>> +
>> +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.
> 

ok, right
since there is no devm_ version of i2c_new_ancillary_device() 
I'll register a remove callback

> 
> Best regards,
> Krzysztof

Thanks
/Piotr