netdev - Re: [RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in ndo

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]

Date:   Wed, 2 Aug 2017 13:54:29 +0200
From:   Mason <slash.tmp@...e.fr>
To:     Mans Rullgard <mans@...sr.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in
 ndo_open

On 02/08/2017 13:02, Måns Rullgård wrote:

> Mason wrote:
> 
>> Move all HW initializations to nb8800_init.
>> This provides the basis for suspend/resume support.
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 50 +++++++++++++++++-------------------
>>  drivers/net/ethernet/aurora/nb8800.h |  1 +
>>  2 files changed, 25 insertions(+), 26 deletions(-)
> 
> You're still not doing anything to preserve flow control and multicast
> configuration, and those are probably not the only ones.

I did handle flow control (as far as I can tell).
The current settings are stored in:
priv->pause_aneg, priv->pause_rx, priv->pause_tx
and nb8800_pause_config() is called from nb8800_link_reconfigure()

I'll take a closer look at multicast settings.

> Worse, you're now never tearing down dma properly, which means the
> hardware can be left with active pointers to memory no longer allocated
> to it.

You're making it sound like there is a risk those pointers
might be used somehow. There is no such risk AFAICT.
The PHY is disconnected, NAPI is stopped, RX and TX have
been disabled, and the ISR has been removed.

I have considered putting the HW block in reset (clock gated)
at the end of nb8800_stop() to save power, which would make
the issue you point out moot.

The reason I removed nb8800_dma_stop() in nb8800_stop()
is because it hangs when called from nb8800_suspend().
(It requires interrupts to make progress, and interrupts
seem to be disabled when nb8800_suspend() is called.)

Broader question for netdev:

I've been wondering about costly close operations.
If closing a device is complex (in terms of code), at which
point does it become simpler to just reset the block, and
avoid writing/maintaining/debugging the code performing
said close operation?

> Finally, you still haven't explained why the hw needs to be reset in
> ndo_open().  Whatever is causing your lockup can almost certainly be
> triggered in some other way too.  I will not accept this side-stepping
> of the issue.

(I was not aware that you were the final authority on which
patches are accepted upstream.)

FWIW, I have placed a formal request for the HW dev to
investigate, as I could not reproduce on tango4, despite
several days wasted on this issue.

In the mean time, the driver in its current form does not
support suspend/resume. How would *you* implement it?
(IMO, my way is great in its simplicity and code reuse.)

Regards.