Date:   Wed, 29 Jan 2020 13:57:30 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ykaukab@...e.de" <ykaukab@...e.de>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>
Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication

Hi Madalin,

On Wed, 29 Jan 2020 at 12:53, Madalin Bucur (OSS)
<madalin.bucur@....nxp.com> wrote:
>
> > As far as I understand, for Aquantia devices this is a 3-way switch for:
> > - No rate adaptation
> > - USX rate adaptation
> > - Pause rate adaptation.
> > So what does your "phydev->rate_adaptation = 1" assignment mean?
>
> phydev->rate_adaptation = 1 means the PHY is able to perform rate adaptation
>
> There is not such thing as USX, if you refer to USXGMII, that's something you
> can check for in the interface mode.
>

I did nothing more than just read aloud the AQR412 description for
register 1E.31F.

> > From context and datasheet I deduced that you mean "the PHY is
> > configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
> > which are probably the modes in which you're using it.
> >
> > But how do you _know_ that the PHY has this switch set correctly? It
> > is either set by firmware or by MDIO, but I don't see any of that
> > being checked for.
>
> I know it is set because someone does put some work in designing a system,
> in provisioning a correct firmware image.
>

So you don't consider "firmware without this flag set, but software
(bootloader, kernel) enables it" to be a valid way of designing a
system? And your position is "well, I don't care if the person
integrating the system didn't take care of all the hardware/firmware
prerequisites, because software isn't going to attempt to do anything
helpful here even if it can"?

So the generic Linux kernel will just ask the person who put the work
in designing some system, right?

What is it that you are trying to prove?

If anything, this reminds me of the xkcd:
int getRandomNumber()
{
    return 4; // chosen by fair dice roll. guaranteed to be random.
}

> >
> > So you think that having a PHY firmware with rate adaptation disabled
> > is "stupid user"?
> > What if the rate adaptation feature is not enabled in firmware, but is
> > being enabled by U-Boot, but the user had the generic PHY driver
> > instead of the Aquantia one, or used a different or old bootloader?
> > "Stupid user"?
>
> Disabling rate adaptation is one of so many ways one could cripple a
> system interface, there are many that require polarity inversion, lane
> switching, etc. and still there is no support for that in the kernel.
>
> > The PHY and the Ethernet driver are strongly decoupled, so they need
> > to agree on an operating mode that will work for both of them.
> > Ideally the PHY would really know how it's configured, not just
> > hardcode "yeah, I can do rate adaptation, try me".
> > The fact that you can build sanity checks on top (like in this case
> > not allowing the user to disable pause frames in the Ethernet driver
> > on RX), that don't stand in the way, is just proof that the system is
> > well designed. If you can't build sanity checks, or if they stand in
> > the way, it isn't.
>
> If you don't need them, it's even better.
>

I am not really sure where you're heading with this one.

> > If anything, why haven't you considered the opposite logic here:
> > - Ethernet driver (dpaa_eth) supports all speeds 10G and below.
> > - PHY driver (or PHY core) removes the supported and advertised speeds
> > for anything other than 2.5G and 10G if it can't do this rate
> > adaptation thing, and its system side isn't USXGMII. It's not like
> > this is dpaa_eth specific in any way.
>
> "what about...", indeed. There are many situations one could imagine
> when things would not work, we can do some brain storming on improving
> this list. Meanwhile, a real issue is that in the current design, the
> ethernet driver needs to remove modes that are invalid from the PHY
> advertising, but needs to refrain from doing that when coupled with
> a PHY that does rate adaptation. This bit provides just an indication
> of that ability.

An incomplete one, at that.
Here's a list of things/potential design improvements that were
suggested to you but you only gave evasive answers on unrelated
topics:
- PHY says "I can do rate adaptation" [ via pause frames ]. Ethernet
driver knows it supports RX flow control. Good for them. But there's a
difference between "can" and "will", and somebody should bridge that
gap. The PHY should either (a) really check if rate adaptation is
enabled, before saying it is (b) say it is, for the interface modes
where that makes sense, but then actually enable it when necessary.
- The system that will be devised would work as an extensible way for
PHY to report capabilities depending on interface mode. Another
capability has been expressed (in-band autoneg) that is similar to
what you are trying to introduce, in that it requires PHY-MAC
coordination and that it is dependent on the system interface that is
being used.
- Why would the Ethernet driver be concerned about what media-side
link speed gets negotiated and used, as long as there's a PHY that can
deal with that. The approach you're taking doesn't really scale. It
scales better to have this sort of logic in the PHY driver only (if
possible), or in the PHY library (either one) too if necessary. _Yes_,
this is a larger problem and is present outside of dpaa_eth too, at
the moment.

> "what about...", indeed.

I didn't say anything about "anti-stupid devices", you did. I prefer
fail-fast systems because I don't enjoy debugging issues that can be
caught automatically. If you enjoy spending you and your users' time
like that, good for you.

>
> > > Madalin
> > >
> > >
> > > > > --
> > > > > Florian
> > > >
> > > > Regards,
> > > > -Vladimir
> >
> > -Vladimir

-Vladimir

Powered by blists - more mailing lists