On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote:

On Thu, Jul 07, 2022 at 06:15:46PM +0100, Russell King (Oracle) wrote:

quoted

quoted

This is why dsa_port_phylink_register() calls phylink_of_phy_connect()
without checking whether it has a fixed-link or a PHY, because it
doesn't fail even if it doesn't do anything.

In fact I've wanted to make a correction to my previous phrasing that
"this function shouldn't be called if phylink_{,fwnode_}connect_phy() is
going to be called later". The correction is "... with a phy-handle".

I'm not sure that clarification makes sense when talking about
phylink_connect_phy(), so I think if you're clarifying it with a
firmware property, you're only talking about
phylink_fwnode_connect_phy() now?

Yes, it's super hard to verbalize, and this is the reason why I didn't
add "... with a phy-handle" in the first place.

I wanted to say: phylink_connect_phy(), OR phylink_fwnode_connect_phy()
WITH a phy-handle. I shouldn't have conflated them in the first place.

Ah, right, because I interpreted it quite differently!

quoted

quoted

quoted

quoted

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link()
based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error
    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

My question in response would be - why should this DSA specific behaviour
be handled completely internally within phylink, when it's a DSA
specific behaviour? Why do we need boolean flags for this?

Because the end result will be simpler if we respect the separation of
concerns that continues to exist, and it's still phylink's business to
say what is and isn't valid. DSA still isn't aware of the bindings
required by phylink, it just passes its fwnode to it. Practically
speaking, I wouldn't be scratching my head as to why we're checking for
half the prerequisites of phylink_set_max_fixed_link() in one place and
for the other half in another.

True, through this patch set DSA is creating its own context specific
extension of phylink bindings, but arguably those existed before DSA was
even integrated with phylink, and we're just fixing something now we
didn't realize at the time we'd need to do.

I can reverse the question, why would phylink even want to be involved
in how the max fixed link parameters are deduced, and it doesn't just
require that a fixed-link software node is constructed somehow
(irrelevant to phylink how), and phylink is just modified to find and
work with that if it exists? Isn't it for the exact same reason,
separation of concerns, that it's easiest for phylink to figure out what
is the most appropriate maximum fixed-link configuration?

If that could be done, I'd love it, because then we don't have this in
phylink at all, and it can all be a DSA problem to solve. It also means
that others won't be tempted to use the interface incorrectly.

I'm not sure how practical that is when we have both DT and ACPI to deal
with, and ACPI is certainly out of my knowledge area to be able to
construct a software node to specify a fixed-link. Maybe it can be done
at the fwnode layer? I don't know.

I don't want to be misunderstood. I brought up software nodes because
I'm sure you must have thought about this too, before proposing what you
did here. And unless there's a technical reason against software nodes
(which there doesn't appear to be, but I don't want to get ahead of
myself), I figured you must be OK with phylink absorbing the logic, case
in which I just don't understand why you are pushing back on a proposal
how to make phylink absorb the logic completely.

The reason I hadn't is because switching DSA to fwnode brings with it
issues for ACPI, and Andrew wants to be very careful about ACPI in
networking - and I think quite rightly. As soon as one switches from
DT APIs to fwnode APIs, you basically permit people an easy path to
re-use DT properties in ACPI-land without the ACPI issues being first
considered.

So, I think if we did go this route, we need Andrew's input.

quoted

Do you have a handy example of what you're suggesting?

No, I didn't, but I thought, how hard can it be, and here's a hacked up
attempt on one of my boards:

Thanks - that looks like something that should be possible to do, and
way better than trying to shoe-horn this into phylink.

My only comment would be that Andrew would disagree with you about this
being "fixing up broken DT" - he has actively encouraged some drivers
to adopt this "default" mode, which means it's anything but "broken"
but it really is part of the DSA firmware description.

[    4.315754] sja1105 spi0.1: configuring for fixed/rgmii link mode
[    4.322653] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL)
[    4.334796] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL)
[    4.345853] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL)
[    4.356859] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL)
[    4.367245] device eth2 entered promiscuous mode
[    4.371864] DSA: tree 0 setup
[    4.376971] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off
(...)
root@black:~# ip link set swp2 up && dhclient -i swp2 && ip addr show swp2
[   64.762756] fsl-gianfar soc:ethernet@2d90000 eth2: Link is Up - 1Gbps/Full - flow control off
[   64.771530] sja1105 spi0.1 swp2: configuring for phy/rgmii-id link mode
[   68.955048] sja1105 spi0.1 swp2: Link is Up - 1Gbps/Full - flow control off
12: swp2@eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 00:1f:7b:63:02:48 brd ff:ff:ff:ff:ff:ff
    inet 10.0.0.68/24 brd 10.0.0.255 scope global dynamic swp2
       valid_lft 600sec preferred_lft 600sec

It's by far the messiest patch I've posted to the list (in the interest
of responding quickly), but if you study the code you can obviously see
what's missing, basically I've hardcoded the speed to 1000 and I'm
copying the phy-mode from the real DT node.

Yep - there's at least one other property we need to carry over from the
DT node, which is the "ethernet" property.

Unfortunately I don't have the time (and most importantly the interest)
in pushing this any further than that. If you want to take this from
here and integrate it with phylink_get_caps() I'd be glad to review
the result. Otherwise, feel free to continue with phylink_set_max_fixed_link().

I think this could be a much better solution to this problem, quite
simply because we then don't end up with phylink_set_max_fixed_link()
which could be abused - and this keeps the complexity where it should
be, in the DSA code.

As I say, though, I think we need Andrew's input on this. Andrew?

I'll look at turning this into a proper solution tomorrow if Andrew is
okay with the fwnode change.

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