diff mbox series

[net,4/7] net: dsa: mt7530: set both CPU port interfaces to PHY_INTERFACE_MODE_NA

Message ID 20230326140818.246575-5-arinc.unal@arinc9.com (mailing list archive)
State New, archived
Headers show
Series net: dsa: mt7530: fix port 5 phylink, phy muxing, and port 6 | expand

Commit Message

Arınç ÜNAL March 26, 2023, 2:08 p.m. UTC 
From: Arınç ÜNAL <arinc.unal@arinc9.com>

Set interfaces of both CPU ports to PHY_INTERFACE_MODE_NA. Either phylink
or mt7530_setup_port5() on mt7530_setup() will handle the rest.

This is already being done for port 6, do it for port 5 as well.

Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Vladimir Oltean March 27, 2023, 7:12 p.m. UTC | #1 
On Sun, Mar 26, 2023 at 05:08:15PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Set interfaces of both CPU ports to PHY_INTERFACE_MODE_NA. Either phylink
> or mt7530_setup_port5() on mt7530_setup() will handle the rest.
> 
> This is already being done for port 6, do it for port 5 as well.
> 
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")

This is getting comical.. I think I'm putting too much energy in
trying to understand the hidden meaning of this patch set.

In include/linux/phy.h we have:

typedef enum {
	PHY_INTERFACE_MODE_NA,

In lack of other initializer, the first element of an enum gets the
value 0 in C.

Then, "priv" is allocated by this driver with devm_kzalloc(), which
means that its entire memory is zero-filled. So priv->p5_interface and
priv->p6_interface are already set to 0, or PHY_INTERFACE_MODE_NA.

There is no code path between the devm_kzalloc() and the position in
mt7530_setup() that would change the value of priv->p5_interface or
priv->p6_interface from their value of 0 (PHY_INTERFACE_MODE_NA).
For example, mt753x_phylink_mac_config() can only be called from
phylink, after dsa_port_phylink_create() was called. But
dsa_port_phylink_create() comes later than ds->ops->setup() - one comes
from dsa_tree_setup_ports(), and the other from dsa_tree_setup_switches().

The movement of the priv->p6_interface assignment with a few lines
earlier does not change anything relative to the other call sites which
assign different values to priv->p6_interface, so there isn't any
functional change there, either.

So this patch is putting 0 into a variable containing 0, and claiming,
through the presence of the Fixes: tag and the submission to the "net"
tree, that it is a bug fix which should be backported to "stable".

Can it be that you are abusing the meaning of a "bug fix", and that I'm
trying too hard to take this patch set seriously?

> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 6d33c1050458..3deebdcfeedf 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2203,14 +2203,18 @@ mt7530_setup(struct dsa_switch *ds)
>  		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
>  			   RD_TAP_MASK, RD_TAP(16));
>  
> +	/* Let phylink decide the interface later. If port 5 is used for phy
> +	 * muxing, its interface will be handled without involving phylink.
> +	 */
> +	priv->p5_interface = PHY_INTERFACE_MODE_NA;
> +	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> +
>  	/* Enable port 6 */
>  	val = mt7530_read(priv, MT7530_MHWTRAP);
>  	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>  	val |= MHWTRAP_MANUAL;
>  	mt7530_write(priv, MT7530_MHWTRAP, val);
>  
> -	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> -
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> -- 
> 2.37.2
>
Arınç ÜNAL March 27, 2023, 9:57 p.m. UTC | #2 
On 27.03.2023 22:12, Vladimir Oltean wrote:
> On Sun, Mar 26, 2023 at 05:08:15PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Set interfaces of both CPU ports to PHY_INTERFACE_MODE_NA. Either phylink
>> or mt7530_setup_port5() on mt7530_setup() will handle the rest.
>>
>> This is already being done for port 6, do it for port 5 as well.
>>
>> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> 
> This is getting comical.. I think I'm putting too much energy in
> trying to understand the hidden meaning of this patch set.
> 
> In include/linux/phy.h we have:
> 
> typedef enum {
> 	PHY_INTERFACE_MODE_NA,
> 
> In lack of other initializer, the first element of an enum gets the
> value 0 in C.
> 
> Then, "priv" is allocated by this driver with devm_kzalloc(), which
> means that its entire memory is zero-filled. So priv->p5_interface and
> priv->p6_interface are already set to 0, or PHY_INTERFACE_MODE_NA.
> 
> There is no code path between the devm_kzalloc() and the position in
> mt7530_setup() that would change the value of priv->p5_interface or
> priv->p6_interface from their value of 0 (PHY_INTERFACE_MODE_NA).
> For example, mt753x_phylink_mac_config() can only be called from
> phylink, after dsa_port_phylink_create() was called. But
> dsa_port_phylink_create() comes later than ds->ops->setup() - one comes
> from dsa_tree_setup_ports(), and the other from dsa_tree_setup_switches().
> 
> The movement of the priv->p6_interface assignment with a few lines
> earlier does not change anything relative to the other call sites which
> assign different values to priv->p6_interface, so there isn't any
> functional change there, either.
> 
> So this patch is putting 0 into a variable containing 0, and claiming,
> through the presence of the Fixes: tag and the submission to the "net"
> tree, that it is a bug fix which should be backported to "stable".
> 
> Can it be that you are abusing the meaning of a "bug fix", and that I'm
> trying too hard to take this patch set seriously?

I don't appreciate your consistent use of the word "abuse" on my 
patches. I'm by no means a senior C programmer. I'm doing my best to 
correct the driver.

Thank you for explaining the process of phylink with DSA, I will adjust 
my patches accordingly.

I suggest you don't take my patches seriously for a while, until I know 
better.

Arınç
Jakub Kicinski March 28, 2023, 2:03 a.m. UTC | #3 
On Tue, 28 Mar 2023 00:57:57 +0300 Arınç ÜNAL wrote:
> I don't appreciate your consistent use of the word "abuse" on my 
> patches. I'm by no means a senior C programmer. I'm doing my best to 
> correct the driver.
> 
> Thank you for explaining the process of phylink with DSA, I will adjust 
> my patches accordingly.
> 
> I suggest you don't take my patches seriously for a while, until I know 
> better.

Maybe my bad, I should have sent you a note on your previous series
already. The patches may be fine, but the commit messages need to do
a better job of describing what the goal of the change is, functionally.

For fixes the bar is even higher because, as Vladimir points out,
commit messages for fixes need to explain what user visible problem 
the patch is resolving. Think of it as a letter to a person using the
switch who hits a problem and wants to look thru the upstream commits
to see if it's already fixed.
Vladimir Oltean March 28, 2023, 11:20 a.m. UTC | #4 
On Tue, Mar 28, 2023 at 12:57:57AM +0300, Arınç ÜNAL wrote:
> I don't appreciate your consistent use of the word "abuse" on my patches.

Consistent would mean that, when given the same kind of input, I respond
with the same kind of output. I'm thinking you'd want a reviewer to do that?

Last time I said: "It's best not to abuse the net.git tree with non-bugfix patches."
https://patchwork.kernel.org/project/netdevbpf/patch/20230307220328.11186-1-arinc.unal@arinc9.com/

If anything, Jakub was/is slightly inconsistent by accepting those previous
non-bugfix patches to the net.git tree, and then agreeing with me. He probably
did that thinking it wasn't a hill worth dying on, which I can agree with.
But I'm afraid that this didn't help you realize that yes, maybe you really
are abusing the process by submitting exclusively non-bugfix commits to the
net tree. There's a fine balance between trying to be nice and trying not to
transmit the wrong message.

It would be good if you could clarify your objection regarding my consistent
use of the word "abuse" on your patches.

There is a document at Documentation/process/stable-kernel-rules.rst
which I remember having shared with you before, where there are some
indications as to what constitutes a legitimate candidate for "stable"
and what does not.

> I'm by no means a senior C programmer. I'm doing my best to correct the
> driver.
> 
> Thank you for explaining the process of phylink with DSA, I will adjust my
> patches accordingly.
> 
> I suggest you don't take my patches seriously for a while, until I know
> better.

Whether you're a junior or a senior C programmer is entirely irrelevant
here. I have no choice but to take your patches seriously unless otherwise
specified, in the commit message, cover letter, or by marking them as
RFC/RFT (but even then, their intention must be very clearly specified,
so that I know what to comment on, or test).

I don't think you really want what you're asking for, which is for
people to not take your patches seriously. I recommend forming a smaller
community of people which does preliminary patch review and discusses
issues around the hardware you're working on, prior to upstream submission.
That would, at least, be more productive.
Arınç ÜNAL March 28, 2023, 9:26 p.m. UTC | #5 
On 28/03/2023 14:20, Vladimir Oltean wrote:
> On Tue, Mar 28, 2023 at 12:57:57AM +0300, Arınç ÜNAL wrote:
>> I don't appreciate your consistent use of the word "abuse" on my patches.
> 
> Consistent would mean that, when given the same kind of input, I respond
> with the same kind of output. I'm thinking you'd want a reviewer to do that?

Of course.

> 
> Last time I said: "It's best not to abuse the net.git tree with non-bugfix patches."
> https://patchwork.kernel.org/project/netdevbpf/patch/20230307220328.11186-1-arinc.unal@arinc9.com/
> 
> If anything, Jakub was/is slightly inconsistent by accepting those previous
> non-bugfix patches to the net.git tree, and then agreeing with me. He probably
> did that thinking it wasn't a hill worth dying on, which I can agree with.
> But I'm afraid that this didn't help you realize that yes, maybe you really
> are abusing the process by submitting exclusively non-bugfix commits to the
> net tree. There's a fine balance between trying to be nice and trying not to
> transmit the wrong message.
> 
> It would be good if you could clarify your objection regarding my consistent
> use of the word "abuse" on your patches.
> 
> There is a document at Documentation/process/stable-kernel-rules.rst
> which I remember having shared with you before, where there are some
> indications as to what constitutes a legitimate candidate for "stable"
> and what does not.

I forgot this existed, sorry about that. I had this thought left in my 
mind that "any changes that are not new features must go to the net 
tree", which clearly is not the case. I see what you mean now. None of 
my patches on the series satisfy all of the rules specified on the document.

I just think your response could've been less harsh considering I didn't 
intentionally do this. Anyway, it's all resolved now so let's not drag 
this further.

> 
>> I'm by no means a senior C programmer. I'm doing my best to correct the
>> driver.
>>
>> Thank you for explaining the process of phylink with DSA, I will adjust my
>> patches accordingly.
>>
>> I suggest you don't take my patches seriously for a while, until I know
>> better.
> 
> Whether you're a junior or a senior C programmer is entirely irrelevant
> here. I have no choice but to take your patches seriously unless otherwise
> specified, in the commit message, cover letter, or by marking them as
> RFC/RFT (but even then, their intention must be very clearly specified,
> so that I know what to comment on, or test).
> 
> I don't think you really want what you're asking for, which is for
> people to not take your patches seriously. I recommend forming a smaller
> community of people which does preliminary patch review and discusses
> issues around the hardware you're working on, prior to upstream submission.
> That would, at least, be more productive.

Yes, of course. I'm actually planning something similar that involves 
OpenWrt, thanks.

Arınç
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6d33c1050458..3deebdcfeedf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2203,14 +2203,18 @@  mt7530_setup(struct dsa_switch *ds)
 		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
 			   RD_TAP_MASK, RD_TAP(16));
 
+	/* Let phylink decide the interface later. If port 5 is used for phy
+	 * muxing, its interface will be handled without involving phylink.
+	 */
+	priv->p5_interface = PHY_INTERFACE_MODE_NA;
+	priv->p6_interface = PHY_INTERFACE_MODE_NA;
+
 	/* Enable port 6 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-	priv->p6_interface = PHY_INTERFACE_MODE_NA;
-
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);