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

[Kiran Thota <Kiran_Thota@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx [mailto:netdev-owner@xxxxxxxxxxxxxxx] On Behalf Of Francois Romieu
Sent: Monday, June 26, 2006 3:31 PM
To: Kiran Thota
Cc: 'Yoichi Yuasa'; linux-mips@xxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Raj Palani; ralf@xxxxxxxxxxxxxx
Subject: Re: [Repost PATCH 6/6] PMC MSP85x0 gigabit ethernet driver

Kiran Thota <Kiran_Thota@xxxxxxxxxxxxxx> :
[...]
>  - 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.

[...]
> 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@xxxxxxxxxxxxxxx More majordomo info at  http://vger.kernel.org/majordomo-info.html