netdev - Re: [PATCH net-next 4/8] net/funeth: ethtool operations

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:   Thu, 30 Dec 2021 17:23:56 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, Jakub Kicinski <kuba@...nel.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/8] net/funeth: ethtool operations

On Thu, Dec 30, 2021 at 2:26 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > > I _think_ this is wrong. pause->autoneg means we are autoneg'ing
> > > pause, not that we are using auto-neg in general. The user can have
> > > autoneg turned on, but force pause by setting pause->autoneg to False.
> > > In that case, the pause->rx_pause and pause->tx_pause are given direct
> > > to the MAC, not auto negotiated.
> >
> > Having this mixed mode needs device FW support, which isn't there today.
>
> So if you are asked to set pause with pause->autoneg False, return
> -EOPNOTSUPP. And pause get should always return True. It is O.K, to
> support a subset of a feature, and say you don't support the
> rest. That is much better than wrongly implementing it until your
> firmware gets the needed support.

include/uapi/linux/ethtool.h has this comment

 * If the link is autonegotiated, drivers should use
 * mii_advertise_flowctrl() or similar code to set the advertised
 * pause frame capabilities based on the @rx_pause and @tx_pause flags,
 * even if @autoneg is zero. ...

I read this as saying that pause->autoneg is ignored if AN is on and the
requested pause settings are fed to AN. I believe this is what the code
here implements.

Whereas you are saying that pause->autoneg == 0 should force
despite AN. Right?

> > If you force pause, then the link flaps and negotiates FW will apply the new
> > negotiated settings. For your scenario you'd want it to support only partial
> > application. Meanwhile the partner doesn't know we don't obey the negotiated
> > settings so I am suspicious that all of this would work.
>
> What happen when Linux is controlling the hardware, not firmware, is
> that phylib makes a callback into the MAC driver telling it the
> results of the autoneg. Part of those results are what pause has been
> negotiated, if pause is part of the negotiation. The MAC driver then
> needs to program the MAC hardware with that information. If pause
> autoneg is not being used, you directly program the hardware. When you
> have hidden the hardware from Linux, you need a similar API to the
> firmware to tell it how to program the hardware.
>
> Forcing pause is mostly there to work around broken link peers who get
> pause wrong. And unfortunately, lots of drivers get pause wrong, and
> it is not helped by the API being poorly defined, and people
> re-inventing the wheel by using firmware, not Linux to control the
> hardware. But it also means you don't care too much if the link peer
> is confused, it was probably doing the wrong thing anyway.
>
>       Andrew