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

[Po-Yu Chuang]

Dear Joe,

On Fri, Jan 21, 2011 at 3:00 AM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Thu, 2011-01-20 at 23:30 +0800, Po-Yu Chuang wrote:
>> Âdrivers/net/ftmac100.c | 1243 ++++++++++++++++++++++++++++++++++++++++++++++++
>
> []
>
>> +/******************************************************************************
>> + * struct napi_struct functions
>> + *****************************************************************************/
>> +static int ftmac100_poll(struct napi_struct *napi, int budget)
>> +{
>> + Â Â struct ftmac100 *priv = container_of(napi, struct ftmac100, napi);
>> + Â Â struct net_device *netdev = priv->netdev;
>> + Â Â unsigned int status;
>> + Â Â bool completed = true;
>> + Â Â int rx = 0;
>> +
>> + Â Â status = ioread32(priv->base + FTMAC100_OFFSET_ISR);
>> +
>> + Â Â if (status & (FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF)) {
>> + Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â* FTMAC100_INT_RPKT_FINISH:
>> + Â Â Â Â Â Â Â* Â Â ÂRX DMA has received packets into RX buffer successfully
>> + Â Â Â Â Â Â Â*
>> + Â Â Â Â Â Â Â* FTMAC100_INT_NORXBUF:
>> + Â Â Â Â Â Â Â* Â Â ÂRX buffer unavailable
>> + Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â bool retry;
>> +
>> + Â Â Â Â Â Â do {
>> + Â Â Â Â Â Â Â Â Â Â retry = ftmac100_rx_packet(priv, &rx);
>> + Â Â Â Â Â Â } while (retry && rx < budget);
>> +
>> + Â Â Â Â Â Â if (retry && rx == budget)
>> + Â Â Â Â Â Â Â Â Â Â completed = false;
>
> Is it useful to retry the NORXBUF case?

The idea is that if I miss packet finished interrupts (then rx buffers used up),
I should retrieve the received packets ASAP to free buffers to HW.
Maybe this is really unnecessary.
I am not quite sure, but I'll do your advice now.

>> + Â Â }
>> +
>> + Â Â if (status & FTMAC100_INT_NORXBUF) {
>> + Â Â Â Â Â Â /* RX buffer unavailable */
>> + Â Â Â Â Â Â if (net_ratelimit())
>> + Â Â Â Â Â Â Â Â Â Â netdev_info(netdev, "INT_NORXBUF\n");
>> +
>> + Â Â Â Â Â Â netdev->stats.rx_over_errors++;
>> + Â Â }
>
> Perhaps this "if (status & FTMAC100_INT_NORXBUF)" block should be
> moved into the test block above it before the retry?

Since status is not changed in the function, it does not matter where
the test is.
But I agree that it is better to handle error cases earlier.

> It's possible to miss multiple states because of the ratelimit.
>
> If multiple ISR status bits are possible, it might be better to
> combine all netdev_info uses into a single call.
>
> Something like:
>
> Â Â Â Âif ((status & (FTMAC100_INT_NORXBUF | FTMAC100_INT_RPKT_LOST |
> Â Â Â Â Â Â Â Â Â Â Â FTMAC100_INT_AHB_ERR | FTMAC100_INT_PHYSTS_CHG)) &&
> Â Â Â Â Â Ânet_ratelimit())
> Â Â Â Â Â Â Â Ânetdev_info(netdev, "ISR status: %x%s%s%s%s\n",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âstatus & FTMAC100_INT_NORXBUF ? ": INT_NORXBUF" : "",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âstatus & FTMAC100_INT_RPKT_LOST ? ": INT_RPKT_LOST" : "",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âstatus & FTMAC100_INT_AHB_ERR ? ": INT_AHB_ERR" : "",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âstatus & FTMAC100_INT_PHYSTS_CHG ? " : INT_PHYSTS_CHG" : "");

Agree. Thanks.

best regards,
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/