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

[Lukasz Majewski <lukma@denx.de>]

[-- 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 --]