linux-kernel - Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

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: <8f431197-474e-4cd5-9c3e-d573c3f3e6b5@lunn.ch>
Date: Wed, 2 Apr 2025 14:47:56 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Lukasz Majewski <lukma@...x.de>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 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>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 4/4] net: mtip: The L2 switch driver for imx287

> +static void read_atable(struct switch_enet_private *fep, int index,
> +			unsigned long *read_lo, unsigned long *read_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	*read_lo = readl((const void *)atable_base + (index << 3));
> +	*read_hi = readl((const void *)atable_base + (index << 3) + 4);
> +}
> +
> +static void write_atable(struct switch_enet_private *fep, int index,
> +			 unsigned long write_lo, unsigned long write_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	writel(write_lo, (void *)atable_base + (index << 3));
> +	writel(write_hi, (void *)atable_base + (index << 3) + 4);
> +}

It would be nice to have the mtip_ prefix on all functions.

> +static int mtip_open(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int ret, port_idx = priv->portnum - 1;
> +
> +	if (fep->usage_count == 0) {
> +		clk_enable(fep->clk_ipg);
> +		netif_napi_add(dev, &fep->napi, mtip_rx_napi);
> +
> +		ret = mtip_alloc_buffers(dev);
> +		if (ret)
> +			return ret;

nitpick: You might want to turn the clock off before returning the
error.

> +	}
> +
> +	fep->link[port_idx] = 0;
> +
> +	/* Probe and connect to PHY when open the interface, if already
> +	 * NOT done in the switch driver probe (or when the device is
> +	 * re-opened).
> +	 */
> +	ret = mtip_mii_probe(dev);
> +	if (ret) {
> +		mtip_free_buffers(dev);

I've not checked. Does this do the opposite of netif_napi_add()?

> +static void mtip_set_multicast_list(struct net_device *dev)
> +{
> +	unsigned int i, bit, data, crc;
> +
> +	if (dev->flags & IFF_PROMISC) {
> +		dev_info(&dev->dev, "%s: IFF_PROMISC\n", __func__);

You can save one level of indentation with a return here.

> +	} else {
> +		if (dev->flags & IFF_ALLMULTI) {
> +			dev_info(&dev->dev, "%s: IFF_ALLMULTI\n", __func__);

and other level here.

> +		} else {
> +			struct netdev_hw_addr *ha;
> +			u_char *addrs;
> +
> +			netdev_for_each_mc_addr(ha, dev) {
> +				addrs = ha->addr;
> +				/* Only support group multicast for now */
> +				if (!(*addrs & 1))
> +					continue;

You could pull there CRC caluclation out into a helper. You might also
want to search the tree and see if it exists somewhere else.

> +
> +				/* calculate crc32 value of mac address */
> +				crc = 0xffffffff;
> +
> +				for (i = 0; i < 6; i++) {

Is 6 the lengh of a MAC address? There is a #define for that.

> +					data = addrs[i];
> +					for (bit = 0; bit < 8;
> +					     bit++, data >>= 1) {
> +						crc = (crc >> 1) ^
> +						(((crc ^ data) & 1) ?
> +						CRC32_POLY : 0);
> +					}
> +				}
> +			}
> +		}
> +	}
> +}
> +

> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev)
> +{
> +	if (ndev->netdev_ops == &mtip_netdev_ops)
> +		return netdev_priv(ndev);
> +
> +	return NULL;
> +}
> +

> +static int __init mtip_switch_dma_init(struct switch_enet_private *fep)
> +{
> +	struct cbd_t *bdp, *cbd_base;
> +	int ret, i;
> +
> +	/* Check mask of the streaming and coherent API */
> +	ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_warn(&fep->pdev->dev, "No suitable DMA available\n");

Can you recover from this? Or should it be dev_err()?

More later...

	Andrew