Date:   Wed, 29 Jan 2020 12:47:40 +0000
From:   Madalin Bucur <madalin.bucur@....com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "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

Vladimir,

you had a thread opened about some of the points you are trying to make here
and as far as I read it, you were told it's a bad place for whatever you were
trying to accomplish there. You can use this thread if you feel like it, but
please try to stick to the actual proposal and reserve initiatives for further
submissions (you're planning to make?). Sorry, my proposal has not intent to
fix yours.

> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: Wednesday, January 29, 2020 1:58 PM
> To: Madalin Bucur (OSS) <madalin.bucur@....nxp.com>
> Cc: Florian Fainelli <f.fainelli@...il.com>; davem@...emloft.net;
> andrew@...n.ch; hkallweit1@...il.com; netdev@...r.kernel.org;
> 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.

Can you please share a link to that manual on the list?

There are hundreds of features and config knobs in the PHYs and very few
are supported in the upstream drivers yet, somehow, people use those PHYs
without issues.
 
> > > 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?

I'm just trying to fix a particular problem, if you are concerned by other
issues please submit a patch, I can try to review it.

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

I appreciate the humor attempt, but it does not help this conversation.

https://xkcd.com/386/

> > >
> > > 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.

Lookup "kiss principle"
 
> > > 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.

Sorry, I do not have a need for all this overhead. Let's not overengineer
a solution for hypothetical issues.

> - 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.

Which system? I've added a bit for a certain capability, to fix a particular
problem, I'm not proposing a "system" to cure all issues there. Do you have
such a proposal?

> - 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.

I'm glad you understood this, there have been messages on the mailing list
about it. You can always try to send some patches to make things better.

> 
> > "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.

Please also lookup "straw man", maybe it will help keep the discussion
focused.

Madalin

Powered by blists - more mailing lists