Date:   Thu, 23 Feb 2023 02:22:24 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Marek Vasut <marex@...x.de>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        UNGLinuxDriver@...rochip.com,
        Woojung Huh <woojung.huh@...rochip.com>, stable@...r.kernel.org
Subject: Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function
 for KSZ87xx

Hi Marek,

On Thu, Feb 23, 2023 at 12:55:08AM +0100, Marek Vasut wrote:
> The old code, removed in:
> c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function")
> used ksz_write8() (this part is important):
> ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
> where:
> drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6
> 0x56
> 
> The new code, where the relevant part is added in (see Fixes tag)
> 46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get
> function")
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = {
> +       [P_XMII_CTRL_1]                 = 0x56,
> uses ksz_pwrite8() (with p in the function name, p means PORT):
> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
> which per drivers/net/dsa/microchip/ksz_common.h translates to
> ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data);
> and that dev->dev_ops->get_port_addr(port, offset) remapping function is per
> drivers/net/dsa/microchip/ksz8795.c really call to the following macro:
> PORT_CTRL_ADDR(port, offset)
> which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes
> #define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0))
> 
> That means:
> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
> writes register 0xa6 instead of register 0x56, because it calls the
> PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call
> PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the
> value 0x56 is already remapped .

I never had any objection to this part.

> All the call-sites which do
> ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
> or
> ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8)
> are affected by this, all six, that means the ksz_[gs]et_xmii() and the
> ksz_[gs]et_gbit().

I'm going to say this with a very calm tone, please tell me where it's wrong
and we can go from there.

 Not for the ksz_switch_chips[] elements which point to ksz8795_regs (which
 had the incorrect value you're fixing), it isn't. You're making an argument
 for code which never executes (5 out of 6 call paths), and basing your commit
 message off of it. Your commit message reads as if you didn't even notice
 that ksz_set_gbit() isn't called for ksz87xx, and that this is a bug in itself.
 Moreover, the problem you're seeing (I may speculate what that is, but
 I don't know) is surely not due to ksz_set_gbit() being called on the
 wrong register, because it's not called at all *for that hardware*.

That gigabit bug was pointed out to you by reviewers, and you refuse to
acknowledge this and keep bringing forth some other stuff which was never
debated by anyone. The lack of acknowledgement plus your continuation to
randomly keep singing another tune in another key completely is irritating
to me on a very personal (non-technical) level. To respond to you, I am
exercising some mental muscles which I wish I wouldn't have needed to,
here, in this context. The same muscles I use when I need to identify
manipulation on tass.com.

[ in case the message above is misinterpreted: I did not say that you
  willingly manipulate. I said that your lack of acknowledgement to what
  is being said to you is giving me the same kind of frustration ]

This is my feedback to the tone in your replies. I want you to give your
feedback to my tone now too. You disregarded that.

> ...
> 
> If all that should be changed in the commit message is "to access the
> P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()"
> function instead, then just say so.
> 
> [...]

No, this is not all that I want.

The gigabit bug changes things in ways in which I'm curious how you're
going to try to defend, with this attitude of responding to anything
except to what was asked. Your commit says it fixes gigabit on KSZ87xx,
but the gigabit bug which *was pointed out to you by others* is still
there. Your patch fixes something else, but *it says* it fixes a gigabit
bug. What I want is for you to describe exactly what it fixes, or if you
just don't know, say you noticed the bug during code review and you
don't know what is its real life impact (considering pin strapping).

I don't want a patch to be merged which says it fixes something it doesn't
fix, while leaving the exact thing it says it fixes unfixed.

I also don't want to entertain this game of "if it's just this small
thing, why didn't you say so". I would be setting myself up for an
endless time waste if I were to micromanage your commit message writing.

I am looking forward to a productive conversation with you, but if your
next reply is going to have the same strategy of avoiding my key message
and focusing on some other random thing, then I'm very sorry, but I'll
just have to focus my attention somewhere else.

Powered by blists - more mailing lists