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: <e440d252-90ba-45d2-80bd-cdb83261bc2c@kernel.org>
Date: Thu, 15 May 2025 17:32:08 +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: [EXTERNAL]Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE
 controller driver

On 15/05/2025 17:20, Piotr Kubik wrote:
> 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.

sysfs is the interface for this.

> This approach is similar to other drivers, all current PSE drivers log it this way.

And this approach was dropped for many, many more drivers. One poor
choice should not be reason to do the same.


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

Huh? I know it will, but this should be explicit 0. Don't make code more
complicated than it should be.



Best regards,
Krzysztof