[PATCH v3 1/5] net: Introduce DSA class for Ethernet switches

[Vladimir Oltean]

On Fri, Jan 22, 2021 at 10:30:48PM +0100, Michael Walle wrote:
> > +	if (ops->port_enable) {
> > +		struct dsa_port_pdata *port_pdata;
> > +
> > +		port_pdata = dev_get_parent_plat(pdev);
> > +		err = ops->port_enable(dev, port_pdata->index,
> > +				       port_pdata->phy);
> > +		if (err)
> > +			return err;
> > +
> > +		err = ops->port_enable(dev, priv->cpu_port, NULL);
>
> This will lead to a NULL pointer dereference in felix_port_enable()
> when accessing the phy. Although somehow (due to caching?) u-boot
> won't panic until reaching board_phy_config() (the weak one in
> drivers/net/phy/phy.c).

Weird that this never panicked for me. Also, it must be annoying to
debug a panic in board_phy_config() due to a bad dereference in
felix_start_pcs...

Also, I didn't think we'd reach such fundamental questions about DSA so
soon, but nonetheless, here we are. We don't have a struct phy_device
for the CPU port because we can't call phy_connect, because we don't
have an udevice for the CPU port, because we don't want to present it as
an UCLASS_ETH to anybody, because the user won't actually be able to use
it for traffic. That has been beaten to death already, the question is
what we can do here.

I would hate to inflict upon drivers the pain of checking for a NULL
pointer for the phy_device. It's also not only the checking that's
missing. It's actual information that's missing - we don't know if we
need to set up a 10G port, or 1G, or 100M port. So keeping the phy
argument as NULL is really off the table.

The DSA master should always have a fixed-link stanza at the very least,
no? And phy_connect_fixed fabricates an actual phy_device for that
fixed-link, that we could harvest for the speed, duplex and
phy_interface_t (well, _almost_ for the latter, we'd have to do some
direction reversal for PHY_INTERFACE_MODE_RGMII*, like TXID becomes
RXID, RGMII becomes RGMII_ID...). And this could only ever work for
the fixed-link species of a phy_device. There have been reports from the
people at Bootlin that they were working with some mv88e6xxx switch that
needed an actual struct phy_device in Linux, and phylink/phylib gained
support for working without an attached net device. But that's fairly
odd, so in practice I think it wouldn't be a huge loss if fixed-link is
all we had for the CPU port.

The crux of the problem is that phy_connect needs an udevice to parse
the device tree information from. So it can't just be any udevice, it
must be the udevice bound to the OF node holding the phy-handle and/or
fixed-link stanza. If it could have been any udevice I would have just
passed it the UCLASS_DSA udevice corresponding to the switch itself in a
heartbeat. So if we decided to treat the actual problem, we could:
- Register yet another uclass, something along the lines of
  UCLASS_DSA_HIDDEN_PORT, which will bind to the OF node of the CPU port
  so that we can pass it to dm_eth_phy_connect. I looked at the code
  path and it looks like nobody really cares if it's UCLASS_ETH or not.
  That would inflict some pain to the maintainers of the networking
  code, nonetheless.
- We could simply fabricate a struct phy_device by hand-parsing the
  fixed-link stanza of the CPU port, right where we're skipping over it
  in dsa_post_bind, where we left that damn TODO. We could then keep
  this struct phy_device cpu_phy in struct dsa_priv, not connected to
  any udevice, just fabricated, so that we could pass it to port_enable.
  We'd still be limiting ourselves to just fixed-links though.
- Bite the bullet and register a UCLASS_ETH for the CPU port. It won't
  do anything useful for the user, but it would keep the code simple...