netdev - Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY

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: <20230910174453.GK775887@kernel.org>
Date: Sun, 10 Sep 2023 19:44:53 +0200
From: Simon Horman <horms@...nel.org>
To: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	corbet@....net, steen.hegelund@...rochip.com, rdunlap@...radead.org,
	casper.casan@...il.com, andrew@...n.ch, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, horatiu.vultur@...rochip.com,
	Woojung.Huh@...rochip.com, Nicolas.Ferre@...rochip.com,
	UNGLinuxDriver@...rochip.com, Thorsten.Kummermehr@...rochip.com
Subject: Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support
 for Microchip's LAN865X MACPHY

On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
> MAC integration provides the low pin count standard SPI interface to any
> microcontroller therefore providing Ethernet functionality without
> requiring MAC integration within the microcontroller. The LAN8650/1
> operates as an SPI client supporting SCLK clock rates up to a maximum of
> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
> frames) and control (register access).
> 
> By default, the chunk data payload is 64 bytes in size. A smaller payload
> data size of 32 bytes is also supported and may be configured in the
> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
> register. Changing the chunk payload size requires the LAN8650/1 be reset
> and shall not be done during normal operation.
> 
> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
> PHY and MAC are connected via an internal Media Independent Interface
> (MII).
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>

Hi Parthiban,

thanks for your patches.
Some minor feedback on this one follows.

...

> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c

...

> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
> +{
> +	u32 regval;
> +	bool ret;
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	const u8 *mac = netdev->dev_addr;

Please arrange local variables in Networking code in reverse xmas tree
order - longest line to shortest.

This tool can be of assistance here:
https://github.com/ecree-solarflare/xmastree

...

> +static int lan865x_probe(struct spi_device *spi)
> +{
> +	struct net_device *netdev;
> +	struct lan865x_priv *priv;
> +	u32 regval;
> +	int ret;
> +
> +	netdev = alloc_etherdev(sizeof(struct lan865x_priv));
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(netdev);
> +	priv->netdev = netdev;
> +	priv->spi = spi;
> +	priv->msg_enable = 0;
> +	spi_set_drvdata(spi, priv);
> +	SET_NETDEV_DEV(netdev, &spi->dev);
> +
> +	ret = lan865x_get_dt_data(priv);
> +	if (ret)
> +		return ret;
> +
> +	spi->rt = true;
> +	spi_setup(spi);
> +
> +	priv->tc6 = oa_tc6_init(spi, netdev);
> +	if (!priv->tc6) {
> +		ret = -ENOMEM;
> +		goto error_oa_tc6_init;
> +	}
> +
> +	if (priv->cps == 32) {
> +		regval = CCS_Q0_TX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;
> +
> +		regval = CCS_Q0_RX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
> +			     priv->rxcte))
> +		goto err_macphy_config;

Jumping to err_macphy_config will result in this function returning ret.
However, ret will be 0 at this point. Perhaps it should be set to an
error value.

Flagged by Smatch.

> +
> +	ret = lan865x_phy_init(priv);
> +	if (ret)
> +		goto error_phy;
> +
> +	if (device_get_ethdev_address(&spi->dev, netdev))
> +		eth_hw_addr_random(netdev);
> +
> +	ret = lan865x_set_hw_macaddr(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Failed to configure MAC");
> +		goto error_set_mac;
> +	}
> +
> +	netdev->if_port = IF_PORT_10BASET;
> +	netdev->irq = spi->irq;
> +	netdev->netdev_ops = &lan865x_netdev_ops;
> +	netdev->watchdog_timeo = TX_TIMEOUT;
> +	netdev->ethtool_ops = &lan865x_ethtool_ops;
> +	ret = register_netdev(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Register netdev failed (ret = %d)",
> +				ret);
> +		goto error_netdev_register;
> +	}
> +
> +	phy_start(priv->phydev);
> +	return 0;
> +
> +error_netdev_register:
> +error_set_mac:
> +	phy_disconnect(priv->phydev);
> +	mdiobus_unregister(priv->mdiobus);
> +	mdiobus_free(priv->mdiobus);
> +error_phy:
> +err_macphy_config:
> +	oa_tc6_deinit(priv->tc6);
> +error_oa_tc6_init:
> +	free_netdev(priv->netdev);
> +	return ret;
> +}


...