Re: [net-next v16 06/12] net: mtip: Add net_device_ops functions to the L2 switch driver

Lukasz Majewski
2025-07-26T21:12:00Z

[-- Attachment #1: Type: text/plain, Size: 3891 bytes --]

Hi Jakub,

> On Fri, 25 Jul 2025 00:33:12 +0200 Lukasz Majewski wrote:
> > +static void swap_buffer(void *bufaddr, int len)
> > +{
> > + int i;
> > + unsigned int *buf = bufaddr;
> > +
>
> nit: reverse xmas tree

Ok.

>
> > + if (status & BD_ENET_TX_READY) {
> > + /* All transmit buffers are full. Bail out.
> > + * This should not happen, since dev->tbusy should
> > be set.
> > + */
> > + netif_stop_queue(dev);
> > + dev_err_ratelimited(&fep->pdev->dev, "%s: tx queue
> > full!.\n",
> > + dev->name);
> > + spin_unlock(&fep->hw_lock);
>
> As we discussed on previous revision you have many to one mapping
> of netdevs to queues. I think the warning should only be printed
> if the drivers is in "single netdev" mode. Otherwise it _will_
> trigger.

It will not trigger (as far as I understand the HW) because no matter
which interface will call this function, there will be next, available
descriptor provided to perform the transmission to switch port 0.

>
> BTW you should put the print after the unlock, console writes are
> slow.

+1

>
> > +static void mtip_timeout(struct net_device *dev, unsigned int
> > txqueue) +{
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + struct switch_enet_private *fep = priv->fep;
> > + struct cbd_t *bdp;
> > + int i;
> > +
> > + dev->stats.tx_errors++;
> > +
> > + if (IS_ENABLED(CONFIG_SWITCH_DEBUG)) {
>
> why are you hiding the debug info under a CONFIG_ ?
> (which BTW appears not to be defined at all)

The CONFIG_SWITCH_DEBUG is rather a "local" variable.

> Seems useful to know the state of the HW when the queue hung.
> You can use a DO_ONCE() if you want to avoid spamming logs

Hmmm.... Good point. I will rewrite it.

>
> > + /* Set buffer length and buffer pointer */
> > + bufaddr = skb->data;
>
> You should call skb_cow_data() if you want to write to the skb data.

This is the place where I do send data... (the *xmit_port function).

For reading I do have pool of pages and then I do copy the data to skb.

>
> > +static void mtip_timeout(struct net_device *dev, unsigned int
> > txqueue) +{
> > + struct mtip_ndev_priv *priv = netdev_priv(dev);
> > + struct switch_enet_private *fep = priv->fep;
> > + struct cbd_t *bdp;
> > + int i;
> > +
> > + dev->stats.tx_errors++;
>
> timeouts are already counted by the stack, I think the statistic
> is exposed per-queue in sysfs

Ok. I will remove it.

>
> > + spin_lock_bh(&fep->hw_lock);
> > + dev_info(&dev->dev, "%s: transmit timed out.\n",
> > dev->name);
> > + dev_info(&dev->dev,
> > + "Ring data: cur_tx %lx%s, dirty_tx %lx
> > cur_rx: %lx\n",
> > + (unsigned long)fep->cur_tx,
> > + fep->tx_full ? " (full)" : "",
> > + (unsigned long)fep->dirty_tx,
> > + (unsigned long)fep->cur_rx);
> > +
> > + bdp = fep->tx_bd_base;
> > + dev_info(&dev->dev, " tx: %u buffers\n",
> > TX_RING_SIZE);
> > + for (i = 0; i < TX_RING_SIZE; i++) {
> > + dev_info(&dev->dev, " %08lx: %04x %04x
> > %08x\n",
> > + (kernel_ulong_t)bdp, bdp->cbd_sc,
> > + bdp->cbd_datlen,
> > (int)bdp->cbd_bufaddr);
> > + bdp++;
> > + }
> > +
> > + bdp = fep->rx_bd_base;
> > + dev_info(&dev->dev, " rx: %lu buffers\n",
> > + (unsigned long)RX_RING_SIZE);
> > + for (i = 0 ; i < RX_RING_SIZE; i++) {
> > + dev_info(&dev->dev, " %08lx: %04x %04x
> > %08x\n",
> > + (kernel_ulong_t)bdp,
> > + bdp->cbd_sc, bdp->cbd_datlen,
> > + (int)bdp->cbd_bufaddr);
> > + bdp++;
> > + }
> > + spin_unlock_bh(&fep->hw_lock);

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Johanna Denk,
Tabea Lutz HRB 165235 Munich, Office: Kirchenstr.5, D-82194
Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]