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: <CAOkoqZnJfDUZX8oaE=UGU-Wc6YH5FzNwKFe+09KTxbw4tDYugA@mail.gmail.com>
Date:   Thu, 30 Dec 2021 19:23:47 -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 6:24 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> 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.

OK. If I understand correctly AN on and pause->autoneg != 0 means
negotiate requested settings and accept resolution result, while
AN on and pause->autoneg == 0 means negotiate settings and adopt
them as the resolution result. I'll mark the latter combination unsupported
for now.

>
>     Andrew