lkml.org 
[lkml]   [2013]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH v4 linux-next] et131x: Promote staging et131x driver to drivers/net
On 29 January 2013 04:10, David Miller <davem@davemloft.net> wrote:
> From: Mark Einon <mark.einon@gmail.com>
> Date: Wed, 23 Jan 2013 16:24:38 +0000
>
>> +endif # NET_VENDOR_AGERE
>> +
>
> Trailing empty line, delete it.
>
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for the Agere ET-131x ethernet driver
>> +#
>> +
>> +obj-$(CONFIG_ET131X) += et131x.o
>> +
>
> Likewise, get rid of this trailing empty line.
>
>> + /* Spinlocks */
>> + spinlock_t lock;
>> +
>> + spinlock_t tcb_send_qlock;
>> + spinlock_t tcb_ready_qlock;
>> + spinlock_t send_hw_lock;
>> +
>> + spinlock_t rcv_lock;
>> + spinlock_t rcv_pend_lock;
>> + spinlock_t fbr_lock;
>> +
>> + spinlock_t phy_lock;
>
> Do you really need _8_ spinlocks in an ethernet driver? To me that's
> way over the top and excessive.
>
> The most recent driver I've written, for the 10Gbit Sun NIU parts,
> has only one spinlock.
>
>> + /* Determine if this is a multicast packet coming in */
>
> What in the world are you doing in this huge code block?
>
> Such much complexity, multicast list iteration, etc. just to get some
> statistics correct? This stuff is going to run on every multicast
> packet receive.
>
> No other driver does crap like this, get rid of this stuff.
>
>> + skb->dev = adapter->netdev;
>> + skb->protocol = eth_type_trans(skb, adapter->netdev);
>
> eth_type_trans() sets skb->dev, you don't need to duplicate that.
>
>> + netif_rx_ni(skb);
>
> Why isn't this driver supporting NAPI? If you're going to put this
> much time into a driver, use the best interfaces for event processing
> rather than something that might have been appropriate in a new driver
> two decades ago.
>
>> +static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
>> +{
>> + int status = 0;
>> + struct et131x_adapter *adapter = netdev_priv(netdev);
>> +
>> + /* stop the queue if it's getting full */
>> + if (adapter->tx_ring.used >= NUM_TCB - 1 &&
>> + !netif_queue_stopped(netdev))
>> + netif_stop_queue(netdev);
>> +
>> + /* Save the timestamp for the TX timeout watchdog */
>> + netdev->trans_start = jiffies;
>> +
>> + /* Call the device-specific data Tx routine */
>> + status = et131x_send_packets(skb, netdev);
>> +
>> + /* Check status and manage the netif queue if necessary */
>> + if (status != 0) {
>> + if (status == -ENOMEM)
>> + status = NETDEV_TX_BUSY;
>> + else
>> + status = NETDEV_TX_OK;
>> + }
>> + return status;
>> +}
>
> Returning NETDEV_TX_BUSY is an error, no driver should ever return
> that status under normal circumstances. Some drivers return it
> when internal consistency checks fail, but that indicates a driver
> bug rather than a normal condition.
>
> Memory allocation erorrs do not denote NETDEV_TX_BUSY, simply drop
> the packet silently with kfree_skb() and return NETDEV_TX_OK.
>
> That's about enough for me, this driver still needs a lot of work
> and is far from being ready to move out of staging.

Thanks for taking the time to give feedback. I'll update the TODO list.

Cheers,

Mark


\
 
 \ /
  Last update: 2013-01-29 14:43    [W:0.047 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site