lkml.org 
[lkml]   [2011]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] net: add Faraday FTMAC100 10/100 Ethernet driver
Dear Joe,

On Fri, Jan 21, 2011 at 3:00 AM, Joe Perches <joe@perches.com> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-01-21 06:05    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site