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    [W:0.036 / U:60.468 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site