linux-mips
[Top] [All Lists]

Re: [PATCH v2 1/2] NET: ethernet/netlogic: Netlogic XLR/XLS GMAC driver

To: ganesanr@broadcom.com
Subject: Re: [PATCH v2 1/2] NET: ethernet/netlogic: Netlogic XLR/XLS GMAC driver
From: David Miller <davem@davemloft.net>
Date: Wed, 06 Feb 2013 15:11:36 -0500 (EST)
Cc: linux-mips@linux-mips.org, netdev@vger.kernel.org
In-reply-to: <1360063819-17555-1-git-send-email-ganesanr@broadcom.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
References: <1360063819-17555-1-git-send-email-ganesanr@broadcom.com>
Sender: linux-mips-bounce@linux-mips.org
From: ganesanr@broadcom.com
Date: Tue, 5 Feb 2013 17:00:19 +0530

> +config NETLOGIC_XLR_NET
> +     tristate "Netlogic XLR/XLS network device"
> +     default y
> +     select PHYLIB
> +     depends on CPU_XLR
> +     ---help---
> +     This driver support Netlogic XLR/XLS on chip gigabit
> +     Ethernet.

No individual device driver should default to 'y'.   Vendor guards, yes, can
default to 'y', but not individual drivers.

> +/*
> + * The readl/writel implementation byteswaps on XLR/XLS, so
> + * we need to use __raw_ IO to read the NAE registers
> + * because they are in the big-endian MMIO area on the SoC.
> + */

Comments in the networking are to be formatted:

        /* Like
         * this.
         */

Please fix this up in your entire driver.

> +/*
> + * Table of net_device pointers indexed by port, this will be used to
> + * lookup the net_device corresponding to a port by the message ring handler.
> + *
> + * Maximum ports in XLR/XLS is 8(8 GMAC on XLS, 4 GMAC + 2 XGMAC on XLR)
> + */
> +static struct net_device *mac_to_ndev[8];

Make this a dynamic data structure, a parent device that the individual
netdevs are hung off of, it can still be an array.  That way you can have
a bonafide struct device instance and associated hierarchy of devices in
the kernel device list.

Also avoid this strange and non-standard usage of "MAC" as an integer
port index.  The canonical meaning of MAC is the link-layer address of
the device.

> +
> +static inline struct sk_buff *mac_get_skb_back_ptr(void *addr)
> +{
> +     struct sk_buff **back_ptr;
> +
> +     /* this function should be used only for newly allocated packets.
> +      * It assumes the first cacheline is for the back pointer related
> +      * book keeping info.
> +      */
> +     back_ptr = (struct sk_buff **)(addr - MAC_SKB_BACK_PTR_SIZE);
> +     return *back_ptr;
> +}

Use the skb->cb[] control block rather than mis-using the skb data area
for storing internal driver state.

> +     paddr = virt_to_bus(addr);

virt_to_bus() is verboten, use the proper DMA APIs.  I don't care if
this is a specialized driver for a special platform.

> +             addr = bus_to_virt(msg->msg0 & 0xffffffffffULL);

bus_to_virt() is verbotten, use the proper DMA APIs and store correct
references to packets in a translation data structure in your per-netdev
software state.

> +static void __maybe_unused xlr_wakeup_queue(unsigned long dev)

This is really unused, just delete it.

That's enough for me, this driver needs a lot of work.

<Prev in Thread] Current Thread [Next in Thread>