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
