Date:   Thu, 25 Nov 2021 18:18:17 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        George McCollister <george.mccollister@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH RFC net-next 11/12] net: dsa: sja1105: convert to
 phylink_generic_validate()

On Thu, Nov 25, 2021 at 12:56:23PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 24, 2021 at 11:32:00PM +0000, Vladimir Oltean wrote:
> > On Wed, Nov 24, 2021 at 11:21:35PM +0000, Russell King (Oracle) wrote:
> > > Clearly, you have stopped listening to me. This can no longer be
> > > productive.
> > 
> > What is wrong with the second patch? You said I should split the change
> > that allows the SERDES protocol to be changed, and I did. You also said
> > I should make the change in behavior be the first patch, but that it's
> > up to me, and I decided not to make that change now at all.
> > 
> > As for why I prefer to send you a patch that I am testing, it is to make
> > the conversion process easier to you. For example you removed a comment
> > that said this MAC doesn't support flow control, and you declared flow
> > control in mac_capabilities anyway.
> > 
> > So no, I have not stopped listening to you, can you please tell me what
> > is not right?
> 
> Let's be clear: I find dealing with you extremely difficult and
> stressful. I don't find that with other people, such as Marek or
> Andrew. I don't know why this is, but every time we interact, it
> quickly becomes confrontational.

I assure you that I am no more confrontational with you than with others.
If I say something that is unpleasant, it isn't to bother you, it is
because it bothers me and I'd rather let you know. If what I am saying is
wrong, it's because I don't know any better. I'm sure it's the same for you.

> I don't want this, and the only way I can see to stop this is to stop
> interacting with you, which is obviously also detrimental.

You have practically stopped interacting with me already, I have phylink
patches for broken NXP boards that have been waiting for at least 2
months for you to review them:
https://patchwork.kernel.org/project/netdevbpf/cover/20210922181446.2677089-1-vladimir.oltean@nxp.com/

So yes, it is a problem that is blocking me as well. In fact, you are
also known to say that you're "getting to the point of wishing that
phylink did not have users except my own":
https://patchwork.kernel.org/project/linux-mediatek/cover/20200217172242.GZ25745@shell.armlinux.org.uk/#23436579

This is not what maintainers do, or say, and I'm not sure how a phylink
user is supposed to feel about it. Personally I am absolutely dreading
it, since phylink and Russell King the person are one and the same
thing, and if you don't share the same views as Russell King you are
effectively limited in what you can do. It is not fun.

I honestly don't know why you keep fabricating these strawmen that
conversations become confrontational due to me, it makes it appear to
the poor people on CC watching us fight that I am being overly
aggressive and you are out of strategies to defend. Do I need to remind
you how I was drawn into a totally unrelated discussion about dpaa2-eth
having a deadlock due to the rtnl_mutex being held at phylink_create()
time, and I tried to be helpful and understand the phylink and sfp-bus
design, and you ended up calling me flat-out stupid?
Here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210901225053.1205571-2-vladimir.oltean@nxp.com/

And you said: "if you think that's a valid approach, then quite frankly
I don't want you touching my code, because you clearly don't know what
you're doing as you aren't willing to put the necessary effort in to
understanding the code."

So if you treat in this way a person who is trying to help, then what
help do you need, and why do you complain that you aren't getting more
attention with your phylink conversions? You are Russell King, you don't
need any help.

> I don't have a solution to this.

I think a good start would be to reply strictly to actual code, and to
read code before replying, and try to avoid using phrases such as
"I don't want you touching my code", "I can bring a horse to water but
can't make it drink", "please stop being a problem", "you constantly
bitch and moan".

You might notice that things will start to improve.

> Now, as for your second patch, it didn't contain a changelog to
> indicate what had changed, and it looked like it was merely a re-post
> of the previously posted patch. Given how noisy the patch is due to
> the size of the changes being made, this is hardly surprising. There
> is a reason why we ask for changelogs when patches are modified, and
> this is *exactly* why.

Yes, it didn't contain a changelog. A non-confrontational reaction to
that would have been either to read the patch before commenting, or to
ask for a changelog if it was so noisy as you say it was. But instead,
your reaction was "Clearly, you have stopped listening to me. This can
no longer be productive." Talk about me being confrontational.

It is not even the first time you attack me personally on a patch
without reading it:
https://patchwork.ozlabs.org/project/netdev/patch/20200625152331.3784018-5-olteanv@gmail.com/
This time I didn't even tell you that it's annoying as I did last time,
I just asked what is wrong.

> Having saved out and diffed the two patches, I can now see the
> changes you've made. Now:
> 
> -       if (priv->info->supports_2500basex[port]) {
> -               phylink_set(mask, 2500baseT_Full);
> -               phylink_set(mask, 2500baseX_Full);
> +       if (phy_mode == PHY_INTERFACE_MODE_2500BASEX) {
> +               config->mac_capabilities = MAC_2500FD;
> +       } else if (phy_interface_mode_is_rgmii(phy_mode) ||
> +                  phy_mode == PHY_INTERFACE_MODE_SGMII) {
> +               config->mac_capabilities = MAC_10FD | MAC_100FD | MAC_1000FD;
> +       } else {
> +               config->mac_capabilities = MAC_10FD | MAC_100FD;
>         }
> 
> This limitation according to the interface mode is done by the generic
> validation, so is unnecessary unless there really is a restriction on
> the capabilities of the MAC.
> 
> Given that the generic validation will only permit the 2.5G ethtool
> link modes, RGMII and SGMII will permit the 10, 100 and 1G ethtool link
> modes, and MII/RevMII/RMII/RevRMII will only permit the 10 and 100
> ethtool link modes, recoding this in the get_caps is rather pointless.

This is nitpicking. The variable is called "mac_capabilities" and that
is what I am reporting - MAC capabilities. I know what this variable is
being used for right now, which is limiting the PHY advertisement, but I
don't know what it is going to be used for in the future. Clearly there
is nothing wrong in reporting only the actual capabilities supported by
the MAC. I don't care if phylink_get_linkmodes() will automagically
remove MAC_2500FD for an RMII port, since RMII ports really do not
support MAC_2500FD I don't see why I would report it, in fact, as I've
explained to you in the comments to the ocelot patch, I believe that
phylink_get_linkmodes(), plus the way in which mac_capabilities is used
(specifically, the way in which we report it without it being a function
of PHY mode) does gratuitously break PAUSE-based rate adaptation if we
were to use the generic phylink implementation. I therefore find the
generic validation function to be of limited use.

> This also becomes less obvious that it is a correct conversion - one
> can't look at the old validate() code and the new get_caps() code and
> check that it's making the same decisions. The old validate code
> did:
> 
> - allow 10 and 100 FD
> - if mii->xmii_mode[port] is XMII_MODE_RGMII or XMII_MODE_SGMII
>   - allow 1000 FD
> - if priv->info->supports_2500basex[port]
>   - allow 2500 FD
> 
> The new code bases it off the PHY interface mode, and now one has to
> refer to the code in sja1105_init_mii_settings() to see what that is
> doing to work out whether it is making equivalent decisions.

And after looking at sja1105_init_mii_settings(), are the decisions
equivalent or not? My point being that even if I create a separate patch
which changes the decision making process from mii->xmii_mode to
priv->phy_mode, you'd still need to look at that function.

The patch has my sign off on it, and I tested it. Nobody will blame you
for any breakage. If you don't want to carry it, don't carry it, you
don't even need a reason to.

> In other words, it's changing how the decisions are made concerning
> which speeds (whether they are the MAC capabilities or ethtool link
> modes) _and_ converting to the new way of specifying those speeds.
> 
> I've made the decision to drop the sja1105 patch from this series as
> well as ocelot. Do whatever you want there, I no longer care, unless
> what you do causes me problems for phylink.

This is perfectly acceptable to me.

> Thanks.

No, thank you!

Powered by blists - more mailing lists