netdev - Re: [PATCH net-next 1/2] be2net: Collect the transmit queue data in Tx timeout

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: <CAN0M_G5cUefWEaN09AqOpytc8iof8c-84EjZGqnLFf27XLPRoQ@mail.gmail.com>
Date:   Mon, 30 Jul 2018 16:23:12 +0530
From:   Suresh Kumar Reddy Reddygari <suresh.reddy@...adcom.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] be2net: Collect the transmit queue data in
 Tx timeout

On Wed, Jul 25, 2018 at 6:14 PM, Suresh Kumar Reddy Reddygari
<suresh.reddy@...adcom.com> wrote:
> On Mon, Jul 23, 2018 at 11:53 PM, David Miller <davem@...emloft.net> wrote:
>> From: Suresh Reddy <suresh.reddy@...adcom.com>
>> Date: Mon, 23 Jul 2018 10:25:23 -0400
>>
>>> Driver dumps tx_queue, tx_compl, pending SKBs information in tx_timeout.
>>> This debug data used to idenfiy the cause of the time out.
>>>
>>> Also reset Lancer chip in tx_timeout.
>>>
...
>>
>> The purpose of the tx timeout NDO operation is to do whatever is
>> necessary to handle the TX timeout.
>>
>> Outputting debugging information is useful, but is secondary.
>>
>> I see that you do reset the Lancer, but that is far from what really
>> needs to happen here.
>>
>> When you get a TX timeout, the hardware is not processing TX ring
>> entries, nor signalling completion any longer.
>>
>> Therefore the only way to get things going again is to reset all of
>> the TX side data structure and logic.  This means shutting down the TX
>> engine, freeing up all of the TX SKBs in the ring, resetting the TX
>> ring software state, and then finally reprogramming the head/tail
>> pointer registers and re-enabling TX DMA processing.

Hi David,

I am clarifying again about the patch (Lancer reset) as I didnt see a reply
from you after my clarification.

> +static void be_tx_timeout(struct net_device *netdev)
> +{
...
> +
> +       if (lancer_chip(adapter)) {
> +               dev_info(dev, "Initiating reset due to tx timeout\n");
> +               dev_info(dev, "Resetting adapter\n");
> +               status = lancer_physdev_ctrl(adapter,
> +                                            PHYSDEV_CONTROL_FW_RESET_MASK);

This patch does recover from a TX-timeout by resetting the chip itself
that *includes* resetting the TX block. Freeing up TX SKBs and rings and
re-creating/registering them is also being done.

Driver and Firmware does the following in chip reset path.

1. When driver sets the PHYSDEV_RESET_MASK bit, Lancer firmware
    goes into an error state and indicates this back to the driver via a bit in
    a doorbell register.
2. Driver detects this and calls be_err_recover().
    be_close() and be_clear() are called in this error recovery path.
    a) In be_close(), we cleanup all pending TX queue entries and SKBs.
    b) In be_clear(), we destroy TX, RX and all other queues.
3. be_resume() is called after be_cleanup() in error recover path.
    In this routine, we create TX, RX queues and other resources
    which were destroyed in cleanup path.

Now, this recovery is supported only on Lancer NICs (and not in  BE3
and Skyhawk NICs). That's why this patch  only gathers diagnostic information
in the case of BE3 and Skyhawk NICs,  but on Lancer NICs does the extra step
of resetting and recovering.

Please let me know if this makes sense. I send a v2 patch with this
explanation in the
commit message.

Regards,
Suresh.