On Thu, 2011-05-26 at 11:36 +0000, GuanXuetao wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/mac-puv3.c
[...]
> +/**********************************************************************
> + * Simple types
> + ********************************************************************* */
> +enum umal_speed {
> + umal_speed_none = 0,
> + umal_speed_10 = SPEED_10,
> + umal_speed_100 = SPEED_100,
> + umal_speed_1000 = SPEED_1000,
> +};
Just use the numbers directly: 0, 10, 100, 1000.
[...]
> +#define ETHER_ADDR_LEN 6
This is already called ETH_ALEN.
[...]
> +/**********************************************************************
> + * DMA Descriptor structure
> + ********************************************************************* */
> +struct umaldmadscr {
> + dma_addr_t PacketStartAddr;
> + int PacketSize;
> + dma_addr_t NextDescriptor;
> + struct umaldmadscr *NextDescriptor_Virt;
> +};
Linux naming convention for structure fields is words_with_underscores
not TitleCase.
[...]
> +/**********************************************************************
> + * UMAL_MII_RESET(bus)
> + *
> + * Reset MII bus.
> + *
> + * Input parameters:
> + * bus - MDIO bus handle
> + *
> + * Return value:
> + * 0 if ok
> + ********************************************************************* */
Function documentation comments must follow the kernel-doc format.
[...]
> +static int umaldma_add_rcvbuffer(struct umal_softc *sc, struct umaldma *d,
> + struct sk_buff *sb)
> +{
[...]
> + /*
> + * Allocate a sk_buff if we don't already have one.
> + * If we do have an sk_buff, reset it so that it's empty.
> + *
> + * Note: sk_buffs don't seem to be guaranteed to have any sort
> + * of alignment when they are allocated. Therefore, allocate enough
> + * extra space to make sure that:
> + *
> + * 1. the data does not start in the middle of a cache line.
> + * 2. The data does not end in the middle of a cache line
> + * 3. The buffer can be aligned such that the IP addresses are
> + * naturally aligned.
> + *
> + * Remember, the SOCs MAC writes whole cache lines at a time,
> + * without reading the old contents first. So, if the sk_buff's
> + * data portion starts in the middle of a cache line, the SOC
> + * DMA will trash the beginning (and ending) portions.
> + */
> + if (sb == NULL) {
> + sb_new = netdev_alloc_skb(dev, ENET_PACKET_SIZE +
> + SMP_CACHE_BYTES * 2 + NET_IP_ALIGN);
> + if (sb_new == NULL) {
> + pr_info("%s: sk_buff allocation failed\n",
> + d->umaldma_eth->umal_dev->name);
> + return -ENOBUFS;
Surely -ENOMEM.
> + }
> + skb_reserve(sb_new, 2);
This presumably needs to be:
skb_reserve(sb_new, SMP_CACHE_BYTES + NET_IP_ALIGN);
unless you assume that the skb allocator will always ensure cache line
alignment (in which case, why use padding of SMP_CACHE_BYTES * 2?).
[...]
> +static void umaldma_tx_process(struct umal_softc *sc, struct umaldma *d,
> + int poll)
> +{
> + struct net_device *dev = sc->umal_dev;
> + int curidx;
> + int hwidx;
> + struct umaldmadscr *dsc;
> + struct sk_buff *sb;
> + unsigned long flags;
> + int packets_handled = 0;
> + unsigned int int_status;
> +
> + spin_lock_irqsave(&(sc->umal_lock), flags);
> +
> + if (!netif_device_present(dev))
> + return;
[...]
This returns with the lock held!
(This is not by any means a thorough review.)
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
