| 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
| ||
|
Message-ID: <20250406232431.48e837e0@wsk>
Date: Sun, 6 Apr 2025 23:24:31 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Andrew Lunn <andrew@...n.ch>
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
Hi Andrew,
> > +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.
Ok.
>
> > +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.
Ok.
>
> > + }
> > +
> > + 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()?
No, the netif_napi_add() is required here as well.
>
> > +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.
Ok.
>
> > + } else {
> > + if (dev->flags & IFF_ALLMULTI) {
> > + dev_info(&dev->dev, "%s: IFF_ALLMULTI\n",
> > __func__);
>
> and other level here.
Ok.
>
> > + } 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.
>
The ether_crc_le(ndev->addr_len,·ha->addr); could be the replacement.
However, when I look on the code and compare it with fec_main.c's
set_multicast_list() - it looks like a dead code.
The calculated hash is not used at all (in fec_main.c it is written to
some registers).
I've refactored the code to do similar things, but taking into account
already set switch setup (promisc must be enabled from the outset).
> > +
> > + /* 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.
This is not needed and can be replaced with already present function.
>
> > + 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()?
>
It was my mistake - of course there shall be dev_err().
> More later...
>
> Andrew
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists