Hi,

quoted

quoted

quoted

+//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
 */

quoted

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.

quoted

quoted

quoted

+/* 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.

quoted

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