RE: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver

Francois,
 Thanks for your comments. I have absorbed almost all of your suggestions. You skimmed through 33% of the file before you dozed off... Waiting for the rest!

Kiran 

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Francois Romieu
Sent: Monday, June 26, 2006 3:31 PM
To: Kiran Thota
Cc: 'Yoichi Yuasa'; linux-mips@linux-mips.org; netdev@vger.kernel.org; Raj Palani; ralf@linux-mips.org
Subject: Re: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver

Kiran Thota [off-list ref] :
[...]

 - Based on linux-2.6.12 from
 
http://www.linux-mips.org/pub/linux/mips/kernel/v2.6/linux-2.6.12.tar.
gz

Is there a reason why the patch is not diffed against a more recent version of the mips tree ?

The patch includes ~130 lines ending with a tab or space delimiter.
They could be removed.

[...]

quoted hunk

diff -Naur a/drivers/net/msp85x0_ge.c b/drivers/net/msp85x0_ge.c

--- a/drivers/net/msp85x0_ge.c	1969-12-31 16:00:00.000000000 -0800
+++ b/drivers/net/msp85x0_ge.c	2006-06-26 12:12:39.000000000 -0700

[...]

+/* Static Function Declarations	 */
+static int msp85x0_ge_eth_open(struct net_device *); static int 
+msp85x0_ge_eth_reopen(struct net_device *netdev); static void 
+msp85x0_ge_eth_stop(struct net_device *); static struct 
+net_device_stats *msp85x0_ge_get_stats(struct net_device *); static 
+int msp85x0_ge_init_rx_desc_ring(msp85x0_ge_port_info *, int, int,
+				      unsigned long, unsigned long,
+				      unsigned long);
+static int msp85x0_ge_init_tx_desc_ring(msp85x0_ge_port_info *, int,
+				      unsigned long, unsigned long);
+
+static int msp85x0_ge_open(struct net_device *); static int 
+msp85x0_ge_start_xmit(struct sk_buff *, struct net_device *); static 
+int msp85x0_ge_stop(struct net_device *);
+
+static unsigned int msp85x0_ge_tx_coal(int);
+
+static void msp85x0_ge_port_reset(unsigned int); static int 
+msp85x0_ge_free_tx_queue(struct net_device *); static int 
+msp85x0_ge_rx_task(struct net_device *, msp85x0_ge_port_info *); 
+static int msp85x0_ge_port_start(struct net_device *); static int 
+msp85x0_eth_setup_tx_rx_fifo(struct net_device *dev); static int 
+msp85x0_ge_xdma_reset(void);

You should be able to remove the forward declarations by correctly reordering the code.

[...]

+static struct platform_device *msp85x0_ge_device[NO_PORTS]; static 
+struct net_device *msp85x0_eth[NO_PORTS]; static unsigned int 
+msp85x0_ge_sram; static unsigned int port_number;

The way it is used, port_number should be a local variable.

+
+/*
+ * The MSP85x0 GE has two alignment requirements:
+ * -> skb->data to be cacheline aligned (32 byte)
+ * -> IP header alignment to 16 bytes
+ *
+ * The latter is not implemented. So, that results in an extra copy 
+on
+ * the Rx. This is a big performance hog. For the former case, the
+ * dev_alloc_skb() has been replaced with msp85x0_ge_alloc_skb(). The 
+size
+ * requested is calculated:
+ *
+ * Ethernet Frame Size : 1518
+ * Ethernet Header     : 14
+ * Future MSP85x0 change for IP header alignment : 2
+ *
+ * Hence, we allocate (1518 + 14 + 2+ 64) = 1580 bytes.  For IP 
+header
+ * alignment, we use skb_reserve().
+ */
+#define ALIGNED_RX_SKB_ADDR(addr) \
+	((((unsigned long)(addr) + (32UL - 1UL)) \
+	& ~(32UL - 1UL)) - (unsigned long)(addr)) #define 
+msp85x0_ge_alloc_skb(__length, __gfp_flags) \
+({      struct sk_buff *__skb; \
+	__skb = alloc_skb((__length),(__gfp_flags)); \
+	if(__skb){ \
+		int __offset = (int) ALIGNED_RX_SKB_ADDR(__skb->data); \
+		if(__offset) \
+			skb_reserve(__skb, __offset); \
+	} \
+	__skb; \
+})

Please use a function for msp85x0_ge_alloc_skb.

+
+int increment_tx_pkt_count(unsigned int port)

static

+{
+	unsigned int flags;
+	spin_lock_irqsave(&msp85x0_lock,flags);
+	if(port==0){

CodingStyle: if (port == 0) {
(or 'if (!port)')

+		port0_pkt_count++;
+	}
+	else{

	} else {

+		port1_pkt_count++;	
+	}
+	spin_unlock_irqrestore(&msp85x0_lock,flags);

increment_tx_pkt_count() is only issued from msp85x0_ge_tx_queue(), itself only issued from msp85x0_ge_start_xmit and xmit handlers run with irq enabled. The save/restore is not needed.

+	return 0;

Useless return.

+}
+
+unsigned int get_tx_pkt_count(unsigned int port)

static

+{
+	unsigned int flags,pktCount,tx_pending_pkt;

Mixed case can be avoided here.

+	spin_lock_irqsave(&msp85x0_lock,flags);

See above.

+	tx_pending_pkt=MSP85x0_GE_READ(MSP85x0_GE_CHANNEL0_TX_DMA_STS + (port << XDMA_PORT_OFFSET));
+	if(port==0){
+		pktCount=port0_pkt_count - tx_pending_pkt;
+	}
+	else{

Sic.

+		pktCount=port1_pkt_count - tx_pending_pkt;	
+	}
+	spin_unlock_irqrestore(&msp85x0_lock,flags);
+	return pktCount;	
+}
+
+unsigned int decrement_tx_pkt_count(unsigned int port)

static

[...]

+void msp85x0_bringup_sequence(int port)

static

+{
+   	unsigned int reg_data;
+
+	/* SDQPF */
+	reg_data=MSP85x0_GE_READ(MSP85x0_GE_SDQPF_RXFIFO_CTL + (port << 8));

reg_data = MSP85x0_GE_READ(MSP85x0_GE_...


[...]

+static void msp85x0_ge_gmii_config(int port_num) {

[...]

+	if (phy_reg & 0x8000) {
+		if (phy_reg & 0x2000) {
+			/* Full Duplex and 1000 Mbps */
+			MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
+					(port_num << MII_PORT_OFFSET)), 0x201);
+		}  else {
+			/* Half Duplex and 1000 Mbps */
+			MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
+					(port_num << MII_PORT_OFFSET)), 0x2201);
+			}
+	}

:o(
		mode = (phy_reg & 0x2000) ? 0x0201 : 0x2201;
		MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
				 (port_num << MII_PORT_OFFSET)), mode);

+	if (phy_reg & 0x4000) {
+		if (phy_reg & 0x2000) {
+			/* Full Duplex and 100 Mbps */
+			MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
+					(port_num << MII_PORT_OFFSET)), 0x100);
+		} else {
+			/* Half Duplex and 100 Mbps */
+			MSP85x0_GE_WRITE((MSP85x0_GE_GMII_CONFIG_MODE +
+					(port_num << MII_PORT_OFFSET)), 0x2100);
+		}
+	}

I'd bet you could make a single MSP85x0_GE_WRITE for this one and the two above.

[...]

+static void msp85x0_ge_update_afx(msp85x0_ge_port_info * 
+msp85x0_ge_eth) {
+	int port = msp85x0_ge_eth->port_num;
+	unsigned int i;
+	volatile unsigned long reg_data = 0;

Mantra: volatile is not the answer.

Now, what's the question ? :o)


[...]

+static void msp85x0_ge_tx_timeout_task(struct net_device *netdev) {
+	msp85x0_ge_port_info *msp85x0_ge_eth = netdev_priv(netdev);
+	int port = msp85x0_ge_eth->port_num;
+
+	printk("MSP85x0 GE: Transmit timed out. Resetting ... \n");

Missing KERN_xyz

[...]

+static int msp85x0_ge_change_mtu(struct net_device *netdev, int 
+new_mtu) {

[...]

+	if (netif_running(netdev)) {
+		msp85x0_ge_eth_stop(netdev);
+            	MSP85x0_GE_WRITE((MSP85x0_GE_RMAC_MAX_FRAME_LEN + 
+(msp85x0_ge_eth->port_num << MAC_PORT_OFFSET)), new_mtu + 2); // for 
+the padded bytes
+
+		if (msp85x0_ge_eth_reopen(netdev) != 0) {
+			printk(KERN_ERR
+			       "%s: Fatal error on opening device\n",
+			       netdev->name);
+			spin_unlock_irqrestore(&msp85x0_ge_eth->lock, flags);
+			return -1;

Please use -Esomething.

You may consider goto to balance spin_{lock/unlock}.

[...]

+static irqreturn_t msp85x0_ge_sequoia_int_handler(int irq, void *dev_id,
+	struct pt_regs *regs)
+{
+	struct net_device *netdev = (struct net_device *) dev_id;

Useless cast from void *.

[...]

+        /* Handle the Rx next */
+        if (eth_int_cause1 & (RX_INT << (port_num * 16))) {
+	      clear_xdma_interrupts(port_num,RX_INT);
+              if (netif_rx_schedule_prep(netdev)) {
+		   msp85x0_ge_disable_rx_int(port_num);	
+                   __netif_rx_schedule(netdev);			       		

The indentation went badly wrong.

[...]

+static int msp85x0_ge_open(struct net_device *netdev) {
+	msp85x0_ge_port_info *msp85x0_ge_eth = netdev_priv(netdev);
+	unsigned int port_num = msp85x0_ge_eth->port_num;
+	unsigned int irq;
+	int retval;       
+
+	spin_lock_irq(&(msp85x0_ge_eth->lock));
+
+	if (msp85x0_ge_eth_open(netdev) != MSP85x0_OK) {
+		spin_unlock_irq(&(msp85x0_ge_eth->lock));
+		printk("%s: Error opening interface \n", netdev->name);

Missing KERN_xyz

+		free_irq(netdev->irq, netdev);

request_irq() comes later.

+		return -EBUSY;

Why not propagate an usual return status code from msp85x0_ge_eth_open ?

+	}
+
+	spin_unlock_irq(&(msp85x0_ge_eth->lock));
+
+        irq = MSP85x0_ETH_PORT_IRQ;
+
+	retval = request_irq(irq, INTERRUPT_HANDLER,
+		     SA_SAMPLE_RANDOM | SA_SHIRQ, netdev->name, netdev);
+
+	if (retval != 0) {
+		printk(KERN_ERR "Cannot assign IRQ number to MSP85x0 GE \n");
+		return -1;

- msp85x0_ge_eth_open() should be balanced a bit (stop queuing for instance) ;
- no need to throw away retval.

(some sleep needed here, sorry)

--
Ueimor
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html