netdev - Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause

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: <20171115141707.GB2130@lunn.ch>
Date:   Wed, 15 Nov 2017 15:17:07 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Marc Gonzalez <marc_gonzalez@...madesigns.com>
Cc:     Mans Rullgard <mans@...sr.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Mason <slash.tmp@...e.fr>, netdev <netdev@...r.kernel.org>,
        Thibaud Cornic <thibaud_cornic@...madesigns.com>,
        David Miller <davem@...emloft.net>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause_config()

On Wed, Nov 15, 2017 at 11:53:23AM +0100, Marc Gonzalez wrote:
> On 14/11/2017 14:22, Måns Rullgård wrote:
> 
> > Marc Gonzalez wrote:
> > 
> >> On 14/11/2017 13:38, Måns Rullgård wrote:
> >>
> >>> Marc Gonzalez writes:
> >>>
> >>>> The "flow control enable" bit can be tweaked, even if DMA is enabled.
> >>>
> >>> No, it can't.  Maybe on some of your newer chips it can, but not on the
> >>> older ones.
> >>
> >> Again, I suppose you are referring to your SMP8642-based board.
> >>
> >> Are you saying you tested this patch, and it doesn't work?
> >> What are the symptoms?
> > 
> > I didn't test that patch per se.  I did initially try simply changing
> > that bit, and this didn't work.  Either it had no effect, or it locked
> > up the controller, I forget which.  The manual explicitly states that
> > writes are forbidden while the DMA enabled bit is set.
> > 
> > If shutting down the DMA really isn't possible, I would rather just
> > allow changing the flow control setting only when the interface is
> > stopped.  The normal case of getting the initial setting from the
> > auto-negotiation will still work properly.  It just won't be possible to
> > change it while the link is active.
> 
> Hello Mans,
> 
> With the default setup,
> 
> 	priv->pause_aneg = true;
> 	priv->pause_rx = true;
> 	priv->pause_tx = true;

Hi Marc

So you are assuming the peer supports pause? Is that a safe assumption
to make? Should you not have it disabled until auto-neg has completed
and you then know what the peer can do?
> 
> even the very first call to nb8800_pause_config() occurs with netif_running=1
> as well as the RX DMA bit enabled.
> 
> buildroot login: root
> # ip addr add 172.27.64.23/18 brd 172.27.127.255 dev eth0
> # ip link set eth0 up
> [   25.771000] ENTER nb8800_pause_config: netif_running=1
> [   25.776195] CPU: 0 PID: 193 Comm: kworker/0:1 Not tainted 4.9.54-1-rc6 #18
> [   25.783102] Hardware name: Sigma Tango DT
> [   25.787138] Workqueue: events_power_efficient phy_state_machine
> [   25.793107] [<c010f290>] (unwind_backtrace) from [<c010af34>] (show_stack+0x10/0x14)
> [   25.800896] [<c010af34>] (show_stack) from [<c02d06e8>] (dump_stack+0x88/0x9c)
> [   25.808160] [<c02d06e8>] (dump_stack) from [<c03967e4>] (nb8800_pause_config+0x28/0xf0)
> [   25.816208] [<c03967e4>] (nb8800_pause_config) from [<c03969e0>] (nb8800_link_reconfigure+0x134/0x148)
> [   25.825565] [<c03969e0>] (nb8800_link_reconfigure) from [<c0391d84>] (phy_state_machine+0x348/0x468)
> [   25.834750] [<c0391d84>] (phy_state_machine) from [<c012e858>] (process_one_work+0x1d8/0x3f0)
> [   25.843323] [<c012e858>] (process_one_work) from [<c012f6a4>] (worker_thread+0x4c/0x560)
> [   25.851459] [<c012f6a4>] (worker_thread) from [<c0134354>] (kthread+0xec/0xf4)
> [   25.858721] [<c0134354>] (kthread) from [<c01078f8>] (ret_from_fork+0x14/0x3c)
> [   25.874597] nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> 
> 
> This makes sense, since nb8800_open() calls nb8800_start_rx()
> before phy_start().
> 
> Moving nb8800_start_rx() to after nb8800_pause_config() does
> appear to work, but I'm not sure it is the correct action?

nb8800_link_reconfigure() can be called whenever the link to the peer
changes. auto-neg may happen later because the cable was not plugged
in until later, etc.

   Andrew