RE: [PATCH v2 2/2] net: ethernet: Add driver for Sunplus SP7021

[Wells Lu 呂芳騰]

Hi,


> > > > +//define MAC interrupt status bit
> > > please embrace all comments with /* */
> >
> > Do you mean to modify comment, for example,
> >
> > //define MAC interrupt status bit
> >
> > to
> >
> > /* define MAC interrupt status bit */
> 
> Yes. The Kernel is written in C, so C style comments are preferred over C++ comments, even
> if later versions of the C standard allow C++ style comments.

I'll modify all comments to C style in next patch.


> You should also read the netdev FAQ, which makes some specific comments about how multi-line
> comments should be formatted.

Thanks for routing me to the document.
I'll use the new format for multi-line comments.
---
It is requested that you make it look like this:

/* foobar blah blah blah
 * another line of text
 */


> > Yes, I'll add error check in next patch as shown below:
> >
> > 		rx_skbinfo[j].mapping = dma_map_single(&comm->pdev->dev, skb->data,
> > 						       comm->rx_desc_buff_size,
> > 						       DMA_FROM_DEVICE);
> > 		if (dma_mapping_error(&comm->pdev->dev, rx_skbinfo[j].mapping))
> > 			goto mem_alloc_fail;
> 
> If it is clear how to fix the code, just do it. No need to tell us what you are going to
> do, we will see the change when reviewing the next version.

Thanks, I see.


> > > > +/* Transmit a packet (called by the kernel) */ static int
> > > > +ethernet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> > > > +{
> > > > +	struct sp_mac *mac = netdev_priv(ndev);
> > > > +	struct sp_common *comm = mac->comm;
> > > > +	u32 tx_pos;
> > > > +	u32 cmd1;
> > > > +	u32 cmd2;
> > > > +	struct mac_desc *txdesc;
> > > > +	struct skb_info *skbinfo;
> > > > +	unsigned long flags;
> > > > +
> > > > +	if (unlikely(comm->tx_desc_full == 1)) {
> > > > +		// No TX descriptors left. Wait for tx interrupt.
> > > > +		netdev_info(ndev, "TX descriptor queue full when xmit!\n");
> > > > +		return NETDEV_TX_BUSY;
> > > Do you really have to return NETDEV_TX_BUSY?
> >
> > (tx_desc_full == 1) means there is no TX descriptor left in ring buffer.
> > So there is no way to do new transmit. Return 'busy' directly.
> > I am not sure if this is a correct process or not.
> > Could you please teach is there any other way to take care of this case?
> > Drop directly?
> 
> There are a few hundred examples to follow, other MAC drivers. What do they do when out
> of TX buffers? Find the most common pattern, and follow it.

But some drivers return NETDEV_TX_BUSY, some drivers drop packet and return NETDEV_TX_OK
Some drivers seem do not take care this issue. I am not sure.


> You should also thinking about the netdev_info(). Do you really want to spam the kernel
> log? Say you are connected to a 10/Half link, and the application is trying to send UDP
> at 100Mbps, Won't you see a lot of these messages? change it to _debug(), or rate limit
> it.

Yes, I'll modify most netdev_info() to netdev_dbg() in next patch.


> > static void ethernet_tx_timeout(struct net_device *ndev, unsigned int
> > txqueue) {
> > 	struct sp_mac *mac = netdev_priv(ndev);
> > 	struct net_device *ndev2;
> > 	unsigned long flags;
> >
> > 	netdev_err(ndev, "TX timed out!\n");
> > 	ndev->stats.tx_errors++;
> >
> > 	spin_lock_irqsave(&mac->comm->tx_lock, flags);
> > 	netif_stop_queue(ndev);
> > 	ndev2 = mac->next_ndev;
> > 	if (ndev2)
> > 		netif_stop_queue(ndev2);
> >
> > 	hal_mac_stop(mac);
> > 	hal_mac_init(mac);
> > 	hal_mac_start(mac);
> >
> > 	// Accept TX packets again.
> > 	netif_trans_update(ndev);
> > 	netif_wake_queue(ndev);
> > 	if (ndev2) {
> > 		netif_trans_update(ndev2);
> > 		netif_wake_queue(ndev2);
> > 	}
> >
> > 	spin_unlock_irqrestore(&mac->comm->tx_lock, flags); }
> >
> > Is that ok?
> 
> This ndev2 stuff is not nice. You probably need a cleaner abstract of two netdev's sharing
> one TX and RX ring. See if there are any other switchdev drivers with a similar structure
> you can copy. Maybe cpsw_new.c? But be careful with that driver. cpsw is a bit of a mess
> due to an incorrect initial design with respect to its L2 switch. A lot of my initial comments
> are to stop you making the same mistakes.

I'll define a array (pointer to struct net_dev) in driver private (shared) 
structure to access to all net devices. No more mac->next_ndev;.

>     Andrew

Thank you very much for your review.


Best regards,
Wells