Kiran Thota <[EMAIL PROTECTED]> :
[...]
> - 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
