Re: [PATCH] net: add Faraday FTMAC100 10/100 Ethernet driver

[Po-Yu Chuang]

Dear Ben,

On Thu, Jan 13, 2011 at 10:03 PM, Ben Hutchings
<bhutchings@xxxxxxxxxxxxxx> wrote:
> On Thu, 2011-01-13 at 19:49 +0800, Po-Yu Chuang wrote:
>> From: Po-Yu Chuang <ratbert@xxxxxxxxxxxxxxxx>
>>
>> FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and
>> MII. ÂThis driver has been working on some ARM/NDS32 SoC including
>> Faraday A320 and Andes AG101.
> [...]
>> +#define USE_NAPI
>
> All new network drivers should use NAPI only, so please remove the
> definition of USE_NAPI and change the conditional code to use NAPI
> always.

OK, fixed.

> [...]
>> + Â Â struct net_device_stats stats;
>
> There is a net_device_stats structure in the net_device structure; you
> should normally use that instead of adding another one.

OK, fixed.

> [...]
>> +static int ftmac100_reset(struct ftmac100 *priv)
>> +{
>> + Â Â struct device *dev = &priv->netdev->dev;
>> + Â Â unsigned long flags;
>> + Â Â int i;
>> +
>> + Â Â /* NOTE: reset clears all registers */
>> +
>> + Â Â spin_lock_irqsave(&priv->hw_lock, flags);
>> + Â Â iowrite32(FTMAC100_MACCR_SW_RST, priv->base + FTMAC100_OFFSET_MACCR);
>> + Â Â spin_unlock_irqrestore(&priv->hw_lock, flags);
>> +
>> + Â Â for (i = 0; i < 5; i++) {
>> + Â Â Â Â Â Â int maccr;
>> +
>> + Â Â Â Â Â Â spin_lock_irqsave(&priv->hw_lock, flags);
>> + Â Â Â Â Â Â maccr = ioread32(priv->base + FTMAC100_OFFSET_MACCR);
>> + Â Â Â Â Â Â spin_unlock_irqrestore(&priv->hw_lock, flags);
>> + Â Â Â Â Â Â if (!(maccr & FTMAC100_MACCR_SW_RST)) {
>> + Â Â Â Â Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â Â Â Â Â* FTMAC100_MACCR_SW_RST cleared does not indicate
>> + Â Â Â Â Â Â Â Â Â Â Â* that hardware reset completed (what the f*ck).
>> + Â Â Â Â Â Â Â Â Â Â Â* We still need to wait for a while.
>> + Â Â Â Â Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â Â Â Â Â usleep_range(500, 1000);
>
> Sleeping here means this must be running in process context. ÂBut you
> used spin_lock_irqsave() above which implies you're not sure what the
> context is. ÂOne of these must be wrong.
>
> I wonder whether hw_lock is even needed; you seem to acquire and release
> it around each PIO (read or write). ÂThis should be unnecessary since
> each PIO is atomic.

OK, fixed.

> I think you can also get rid of rx_lock; it's only used in the RX data
> path which is already serialised by NAPI.

OK, fixed.

> [...]
>> + Â Â netdev->last_rx = jiffies;
>
> Don't set last_rx; the networking core takes care of it now.

OK, fixed.

>> + Â Â priv->stats.rx_packets++;
>> + Â Â priv->stats.rx_bytes += skb->len;
>
> This should be done earlier, so that these stats include packets that
> are dropped for any reason.

OK, fixed.

> [...]
>> + Â Â netdev->trans_start = jiffies;
>
> Don't set trans_start; the networking core takes care of it now.

OK, fixed.

> [...]
>> + Â Â priv->descs = dma_alloc_coherent(priv->dev,
>> + Â Â Â Â Â Â sizeof(struct ftmac100_descs), &priv->descs_dma_addr,
>> + Â Â Â Â Â Â GFP_KERNEL | GFP_DMA);
>
> On x86, GFP_DMA means the memory must be within the ISA DMA range
> (< 16 MB). ÂI don't know quite what it means on ARM but it may not be
> what you want.

On ARM, all 4G address space can be used by DMA (at least for all the
hardwares I have ever used). All memory pages are in DMA zone AFAIK.
I put GFP_DMA here just to be safe if there were any constraints.

By checking drivers in drivers/net/arm/, ep93xx_eth.c also uses this flag,
so I guess this is acceptable?

> [...]
>> + Â Â if (status & (FTMAC100_INT_XPKT_OK | FTMAC100_INT_XPKT_LOST)) {
>> + Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â* FTMAC100_INT_XPKT_OK:
>> + Â Â Â Â Â Â Â* Â Â Â packet transmitted to ethernet successfully
>> + Â Â Â Â Â Â Â*
>> + Â Â Â Â Â Â Â* FTMAC100_INT_XPKT_LOST:
>> + Â Â Â Â Â Â Â* Â Â Âpacket transmitted to ethernet lost due to late
>> + Â Â Â Â Â Â Â* Â Â Âcollision or excessive collision
>> + Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â ftmac100_tx_complete(priv);
>> + Â Â }
>
> TX completions should also be handled through NAPI if possible.

OK, I'll study how to do that.

> [...]
>> + Â Â priv->rx_pointer = 0;
>> + Â Â priv->tx_clean_pointer = 0;
>> + Â Â priv->tx_pointer = 0;
>> + Â Â spin_lock_init(&priv->hw_lock);
>> + Â Â spin_lock_init(&priv->rx_lock);
>> + Â Â spin_lock_init(&priv->tx_lock);
>
> These locks should be initialised in your probe function.

OK, fixed.

> [...]
>> + Â Â unregister_netdev(netdev);
>
> There should be a netif_napi_del() before this.

OK, fixed.

> A general comment: please use netdev_err(), netdev_info() etc. for
> logging. ÂThis ensures that both the platform device address and the
> network device name appears in the log messages.

OK, fixed.

Thanks a lot for your detailed review. I'll submit a new version ASAP.

Thanks,
Po-Yu Chuang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/