lkml.org 
[lkml]   [2015]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] NET: Add ezchip ethernet driver
From
From: Noam Camus <noamc@ezchip.com>
Date: Thu, 11 Jun 2015 11:33:49 +0300

> +#define NPS_ENET_INT_MASK (sizeof(u32) - 1)
> +#define NPS_ENET_INT_OFFSET 2
> +#define NPS_ENET_WORDS_NUM(length) ((length + NPS_ENET_INT_MASK) >> 2)

This is a bit obfuscating in my opinion.

First of all "NPS_ENET_INT_OFFSET" is not an "offset", which you would
add or subtract from a value, but rather it is a "shift". All of
the uses would look clearer as "X / sizeof(u32)" rather than the
"X >> NPS_ENET_INET_OFFSET".

Same for NPS_ENET_WORDS_NUM(), this is simply "DIV_ROUND_UP(x, sizeof_u32))"
which is much more easy to understand.

And I would just say "sizeof(u32) - 1" outright for the mask as well.

So basically what I'm saying is that these macros make the code harder
to read and understand rather than making it easier.

> +
> + /* to accommodate word-unaligned address of "reg"
> + * we have to do memcpy() instead of simple "="
> + */
> + memcpy(reg, &buf, sizeof(buf));

This is not guaranteed to work. 'ret' is a "u32 *" type therefore the
compiler is allowed to assume the pointer is properly aligned and
therefore emit a 32-bit load/store sequence inline for the memcpy()
call.

Which means all of this unaligned handling code is going to accomplish
nothing at all. The code will still make unaligned accesses.

> + netif_rx(skb);

Please implement proper NAPI support for your driver so that receive
packets are processed via the ->poll() handler in software interrupt
context rather than via netif_rx() in hardware interrupt context.

> +static void nps_enet_tx_irq_handler(struct net_device *netdev,
> + struct nps_enet_tx_ctl tx_ctrl)
> +{

Likewise for TX completion handling.
> +static struct net_device_stats *nps_enet_get_stats(struct net_device *ndev)
> +{
> + return &ndev->stats;
> +}

If this is all that your get_stats() method does, you can leave it unspecified
in nps_netdev_ops, and the core code will do the right thing by default.


\
 
 \ /
  Last update: 2015-06-12 01:21    [W:2.537 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site