Message-ID: <Zfq8TNrt0KxW/IWh@shell.armlinux.org.uk>
Date: Wed, 20 Mar 2024 10:37:00 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Yanteng Si <siyanteng@...ngson.cn>, andrew@...n.ch,
	hkallweit1@...il.com, peppe.cavallaro@...com,
	alexandre.torgue@...s.st.com, joabreu@...opsys.com,
	Jose.Abreu@...opsys.com, chenhuacai@...ngson.cn,
	guyinggang@...ngson.cn, netdev@...r.kernel.org,
	chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 09/11] net: stmmac: dwmac-loongson: Fix half
 duplex

Serge,

Again, please learn to trim your replies.

On Wed, Mar 20, 2024 at 01:10:04PM +0300, Serge Semin wrote:
> > > As to this approach, I don't think it's a good model to override the
> > > stmmac MAC operations. Instead, I would suggest that a better approach
> > > would be for the platform to provide its capabilities to the stmmac
> > > core code (maybe a new member in stmmac_priv) which, when set, is used
> > > to reduce the capabilities provided to phylink via
> > > priv->phylink_config.mac_capabilities.
> > > 
> > > Why? The driver has several components that are involved in the
> > > overall capabilities, and the capabilities of the system is the
> > > logical subset of all these capabilities. One component should not
> > > be setting capabilities that a different component doesn't support.
> 
> In general you are right for sure - it's better to avoid one part
> setting capabilities and another part unsetting them at least from the
> readability and maintainability point of view. But in this case we've
> already got implemented a ready-to-use internal interface
> stmmac_ops::phylink_get_caps() which can be used to extend/reduce the
> capabilities field based on the particular MAC abilities. Moreover
> it's called right from the component setting the capabilities. Are you
> saying that the callback is supposed to be utilized for extending the
> capabilities only?

What concerns me is that the proposed code _overwrites_ the
capabilities from the MAC layer, so from a maintanability point of
view it's a nightmare, because you will now have the situation where
MACs provide their capabilities, and then platform code may overwrite
it - which means it's like a spiders web trying to work out what
capabilities are provided.

The reality is surely that the MAC dictates what it can do, but there
may be further restrictions by other components in the platform, so
the capabilities provided to phylink should be:

	mac_capabilities & platform_capabilities

And what I'm proposing is that _that_ should be done in a way that
makes it _easy_ for the platform code to get right. Overriding
stmmac_ops::phylink_get_caps() doesn't do that - as can be seen in
the proposed patch.

Help your users write correct code by adopting a structure that makes
it easy for them to do the right thing.

> If you insist on not overriding the stmmac_ops::phylink_get_caps()
> anyway then please explain what is the principal difference
> between the next two code snippets:
> 	/* Get the MAC specific capabilities */
>         stmmac_mac_phylink_get_caps(priv);
> and
> 	priv->phylink_config.mac_capabilities &= ~priv->plat->mac_caps_mask;


I was thinking:

	stmmac_mac_phylink_get_caps(priv);

	if (priv->plat->mac_capabilities)
		priv->phylink_config.mac_capabilities &=
			priv->plat->mac_capabilities;

In other words, if a platform sets plat->mac_capabilities, then it is
providing the capabilities that it supports, and those need to reduce
the capabilities provided by the MAC.

This will _also_ allow stmmac_set_half_duplex() to do the right thing.
Consider something in the platform side that doesn't allow half-duplex,
but allows tx_queues_to_use == 1. That'll set the half-duplex modes
when stmmac_set_half_duplex() is called, overriding what the platform
supports.

Now that I look at the stmmac implementation, there's even more that
is wrong. Consider plat->max_speed = 100, like
arch/arc/boot/dts/axs10x_mb.dtsi sets. If stmmac_set_half_duplex()
is called as it can be from stmmac_reinit_queues(), it'll enable
1000 half-duplex, despite the plat->max_speed = 100.

> in the MAC-capabilities update implementation? Do you think the later
> approach would be more descriptive? If so then would the
> callback-based approach almost equally descriptive if the callback
> name was, suppose, stmmac_mac_phylink_set_caps() or similar?

>From what I can see of the existing stmmac MAC phylink_get_caps
implementations, there seem to be two - xgmac_phylink_get_caps()
and dwmac4_phylink_get_caps(). Both of these merely set additional
modes in priv->phylink_config.mac_capabilities. Is there a reason
to have this as an instruction stream, rather than providing data
to the core stmmac code from the MAC about its capabilities? Is
there a reason why it would be necessary for the code in a MAC backend
to make a decision about what capabilities to enable based on some
condition?

> In anyway I am sure the approach suggested in the initial patch of
> this thread isn't good since it motivates the developers to implement
> more-and-more DW MAC-specific platform capabilities flags fixing
> another flags, which makes the generic code even more complicated
> than it already is with endless if-else-plat-flags statements.

Yes, I do agree with that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists