On 29 January 2013 04:10, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: Mark Einon <mark.einon@xxxxxxxxx>
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
