linux-kernel - Re: [PATCH net-next v10 02/11] net: ti: prueth: Adds ICSSM Ethernet 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: <20250708174449.3e8744c5@kernel.org>
Date: Tue, 8 Jul 2025 17:44:49 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Parvathi Pudi <parvathi@...thit.com>
Cc: danishanwar@...com, rogerq@...nel.org, andrew+netdev@...n.ch,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 ssantosh@...nel.org, richardcochran@...il.com, s.hauer@...gutronix.de,
 m-karicheri2@...com, glaroque@...libre.com, afd@...com,
 saikrishnag@...vell.com, m-malladi@...com, jacob.e.keller@...el.com,
 diogo.ivo@...mens.com, javier.carrasco.cruz@...il.com, horms@...nel.org,
 s-anna@...com, basharath@...thit.com, linux-arm-kernel@...ts.infradead.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, vadim.fedorenko@...ux.dev, pratheesh@...com,
 prajith@...com, vigneshr@...com, praneeth@...com, srk@...com,
 rogerq@...com, krishna@...thit.com, pmohan@...thit.com, mohan@...thit.com
Subject: Re: [PATCH net-next v10 02/11] net: ti: prueth: Adds ICSSM Ethernet
 driver

On Wed,  2 Jul 2025 19:36:24 +0530 Parvathi Pudi wrote:
> +		if (ret < 0) {
> +			dev_err(dev, "%pOF error reading port_id %d\n",
> +				eth_node, ret);
> +		}

unnecessary parenthesis, but also did you mean to error out here?

> +			dev_err(dev, "port reg should be 0 or 1\n");
> +			of_node_put(eth_node);

this error will also trigger if same port is specified multiple times

+			ret = PTR_ERR(prueth->pru1);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "unable to get PRU1: %d\n", ret);
+			goto put_pru;

dev_err_probe() ?

> +/**
> + * struct prueth_private_data - PRU Ethernet private data
> + * @fw_pru: firmware names to be used for PRUSS ethernet usecases
> + * @support_lre: boolean to indicate if lre is enabled
> + * @support_switch: boolean to indicate if switch is enabled

Please improve or remove this, adding kdoc which doesn't explain
anything is discouraged per kernel coding style.

This one is actually more confusing than helpful the fields are
called "support" but kdoc says "enabled". Maybe name the fields
'enabled' ?

> + */
> +struct prueth_private_data {
> +	const struct prueth_firmware fw_pru[PRUSS_NUM_PRUS];
> +	bool support_lre;
> +	bool support_switch;
> +};
> +
> +/* data for each emac port */
> +struct prueth_emac {
> +	struct prueth *prueth;
> +	struct net_device *ndev;
> +
> +	struct rproc *pru;
> +	struct phy_device *phydev;
> +
> +	int link;
> +	int speed;
> +	int duplex;
> +
> +	enum prueth_port port_id;
> +	const char *phy_id;
> +	u8 mac_addr[6];
> +	phy_interface_t phy_if;
> +	spinlock_t lock;	/* serialize access */

'serialize access' to what? Which fields does it protect?
-- 
pw-bot: cr