Date:	Tue, 9 Jun 2015 14:11:05 +0000
From:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:	"noamc@...hip.com" <noamc@...hip.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"cmetcalf@...hip.com" <cmetcalf@...hip.com>,
	"Vineet.Gupta1@...opsys.com" <Vineet.Gupta1@...opsys.com>,
	"talz@...hip.com" <talz@...hip.com>,
	"giladb@...hip.com" <giladb@...hip.com>
Subject: Re: [PATCH] NET: Add ezchip ethernet driver

Hi Noam, Tal,

On Tue, 2015-06-09 at 15:44 +0300, Noam Camus wrote:
> From: Tal Zilcer <talz@...hip.com>
> 
> Simple LAN device without multicast support.
> Device performance is not high and may be used for
> debug or management purposes.
> Device supports interrupts for RX and TX end.
> Device does not support NAPI and also does not support DMA.
> It is used in EZchip NPS devices.
> 
> Signed-off-by: Noam Camus <noamc@...hip.com>

Probably it's good to add Tal in "Signed-off-by" here as well.

[snip]

> +static void nps_enet_read_rx_fifo(struct net_device *netdev,
> +                               unsigned char *dst, int length)
> +{
> +     struct nps_enet_priv *net_priv = netdev_priv(netdev);
> +     int i, len = (length >> 2);

I'd say brackets are not really required here, why not just "int i, len
= length >> 2;"?

> +     int last = length & 3;

Shouldn't it be "int last = length & (sizeof(int) - 1);"? Well,
assuming we're talking 4-byte words here.

> +     unsigned int *reg = (unsigned int *)dst;
> +     bool dst_is_align = !((unsigned int)dst & 0x3);
> +
> +     /* In case dst is not aligned we need an intermediate buffer 
> */
> +     if (dst_is_align)
> +             for (i = 0; i < len; i++, reg++)
> +                     *reg = nps_enet_reg_get(net_priv, 
> NPS_ENET_REG_RX_BUF);
> +     else { /* !dst_is_align */
> +             unsigned int buf;
> +
> +             for (i = 0; i < len; i++, reg++) {

I would move definition of "buf" right here in the "for" loop because
it is not used outside this "for", right?

> +                     buf = nps_enet_reg_get(net_priv, 
> NPS_ENET_REG_RX_BUF);
> +                     memcpy(reg, &buf, sizeof(buf));

Here I think it might be useful to add a comment saying why memcpy() is
used here, like to accommodate word-unaligned address of "reg" we have
to do memcpy() instead of simple "=".

> +             }
> +     }
> +
> +     /* copy last bytes (if any) */
> +     if (last) {
> +             unsigned int buf;
> +
> +             buf = nps_enet_reg_get(net_priv, 
> NPS_ENET_REG_RX_BUF);

Here as well no need for separate lines for "buf" declaration and real
usage, let's have more compact "unsigned int buf =
nps_enet_reg_get(net_priv, NPS_ENET_REG_RX_BUF);".

Also what might be good is to make "buf" of explicit storage type. Now
it's just "int" but once you move to 64-bit CPU core length of "int"
will change. So "u32" IMHO is a better option here - that's actually
applicable through all the driver, so please re-visit this topic.

> +             memcpy(reg, &buf, last);

BTW it might also be nice to add a check for "last" value so it never
gets larger than both "reg" and "buf" size.

> +     }
> +}
> +
> +static void nps_enet_rx_irq_handler(struct net_device *netdev,
> +                                 struct nps_enet_rx_ctl rx_ctrl)
> +{
> +     struct nps_enet_priv *net_priv = netdev_priv(netdev);
> +     struct sk_buff *skb;
> +     int frame_len = rx_ctrl.nr;
> +
> +     /* Check Rx error */
> +     if (rx_ctrl.er) {
> +             netdev->stats.rx_errors++;
> +             goto rx_irq_clean;

Why do you go straight to "rx_irq_clean" here and in branches below?
Isn't it possible to have simultaneously set following flags
"rx_ctrl.er", "rx_ctrl.crc" plus "frame_len < ETH_ZLEN" etc?

> +     }
> +
> +     /* Check Rx CRC error */
> +     if (rx_ctrl.crc) {
> +             netdev->stats.rx_crc_errors++;
> +             netdev->stats.rx_dropped++;
> +             goto rx_irq_clean;
> +     }

[snip]

> +/**
> + * nps_enet_irq_handler - Global interrupt handler for ENET.
> + * @irq:                irq number.
> + * @dev_instance:       device instance.
> + *
> + * returns: IRQ_HANDLED for all cases.
> + *
> + * EZchip ENET has 2 interrupt causes, and depending on bits raised 
> in
> + * CTRL register we may tell what is a reason for interrupt to fire.
> + * We got one for RX and the other for TX (end).
> + */
> +static irqreturn_t nps_enet_irq_handler(int irq, void *dev_instance)
> +{
> +     struct net_device *netdev = dev_instance;
> +     struct nps_enet_priv *net_priv = netdev_priv(netdev);
> +     struct nps_enet_rx_ctl rx_ctrl;
> +     struct nps_enet_tx_ctl tx_ctrl;

Both "rx_ctrl" and "tx_ctrl" could be put in one line.

[snip]

> +static void nps_enet_hw_reset(struct net_device *netdev)
> +{
> +     struct nps_enet_priv *net_priv = netdev_priv(netdev);
> +     struct nps_enet_ge_rst ge_rst = {.value = 0};
> +     struct nps_enet_phase_fifo_ctl phase_fifo_ctl = {.value = 
> 0};
> +
> +     /* Pcs reset sequence*/
> +     ge_rst.gmac_0 = NPS_ENET_ENABLE;
> +     nps_enet_reg_set(net_priv, NPS_ENET_REG_GE_RST, 
> ge_rst.value);
> +     usleep_range(10, 20);

I'm wondering if there's a possibility to read back for example the
same bit we set and once it gets reset by hardware we then know for
sure that hardware exited reset state? If there's such a possibility we
may just busywait for it... well probably with known timeout to report
a problem if hardware still in reset state.

Otherwise you cannot be sure hardware is ready to operate really.

Probably I'm nitpicking here but in general driver looks good to me so
feel free to add:

Acked-by: Alexey Brodkin <abrodkin@...opsys.com>

-Alexey--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists