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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTi=b9ma=xV1ovbJmy94BrDT696VR4OcyR9vqwGtB@mail.gmail.com>
Date:	Fri, 21 Jan 2011 13:03:18 +0800
From:	Po-Yu Chuang <ratbert.chuang@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	bhutchings@...arflare.com, eric.dumazet@...il.com,
	dilinger@...ued.net, Po-Yu Chuang <ratbert@...aday-tech.com>
Subject: Re: [PATCH v3] net: add Faraday FTMAC100 10/100 Ethernet driver

Dear Joe,

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