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]

Message-ID: <Yc5p5iAELXFCuY9t@lunn.ch>
Date:   Fri, 31 Dec 2021 03:24:38 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Dimitris Michailidis <d.michailidis@...gible.com>
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 05:23:56PM -0800, Dimitris Michailidis wrote:
> 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?

Take a look at phylink_ethtool_set_pauseparam() and accompanying
functions. This is a newish central implementation for any MAC/PHY
with Linux controlling the hardware. There are ambiguities in the API
description, so it would be better if your driver/firmware combo does
the same a the core Linux code. When Russell wrote that code, there
was quite a bit of discussion what the documentation actually means.

    Andrew