[RFC PATCH 0/2] net: dsa: various fixes
Before a DSA port is probed, the master port needs to be probed first. For now this worked, because the probing order was correct. But it already falls short if you use the enetc6 port on the LS1028A SoC:
Device tree snippet:
&enetc6 { status = "okay"; };
&mscc_felix_port5 { ethernet = <&enetc6>; status = "okay"; };
NB. keep enetc2 enabled, otherwise you will trigger an access violation.
Michael Walle (2): net: dsa: return early if there is no master net: dsa: probe master device
net/dsa-uclass.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
It doesn't make sense to have DSA without a master port. Error out early if there is no master port.
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc --- net/dsa-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 2ce9ddb90d..88a8ea9352 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -280,6 +280,10 @@ static int dsa_port_probe(struct udevice *pdev) if (!port_pdata->phy) return -ENODEV;
+ master = dsa_get_master(dev); + if (!master) + return -ENODEV; + /* * Inherit port's hwaddr from the DSA master, unless the port already * has a unique MAC address specified in the environment. @@ -288,10 +292,6 @@ static int dsa_port_probe(struct udevice *pdev) if (!is_zero_ethaddr(env_enetaddr)) return 0;
- master = dsa_get_master(dev); - if (!master) - return 0; - master_pdata = dev_get_plat(master); eth_pdata = dev_get_plat(pdev); memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
On Tue, Feb 23, 2021 at 05:19:05PM +0100, Michael Walle wrote:
It doesn't make sense to have DSA without a master port. Error out early if there is no master port.
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
I think you can also be more aggressive and remove the checks:
if (!master) return -EINVAL;
from dsa_port_send and dsa_port_recv. At least it sounds broken to me that this could ever happen.
Am 2021-02-23 17:32, schrieb Vladimir Oltean:
On Tue, Feb 23, 2021 at 05:19:05PM +0100, Michael Walle wrote:
It doesn't make sense to have DSA without a master port. Error out early if there is no master port.
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
I think you can also be more aggressive and remove the checks:
if (!master) return -EINVAL;
from dsa_port_send and dsa_port_recv. At least it sounds broken to me that this could ever happen.
Yep, I've seen these, too. I'll send a non-RFC version and remove these.
-michael
I think you can also be more aggressive and remove the checks:
if (!master) return -EINVAL;
from dsa_port_send and dsa_port_recv. At least it sounds broken to me that this could ever happen.
The following comment got me curious:
/* * stop master only if it's active, don't probe it otherwise. * Under normal usage it would be active because we're using it, but * during tear-down it may have been removed ahead of us. */ if (master && device_active(master)) eth_get_ops(master)->stop(master);
Do we actually care about device removal? I don't think it will work right now.
If you do "unbind eth 0" and then using a DSA port you'll get a panic. The check for master doesn't really help here because it will return "priv->master_dev" which is just set in .pre_probe(). Thus in the error case, it will contain a dangling pointer.
-michael
DSA needs to have the master device probed first for MAC inheritance. Until now, it only works by chance because the only user (LS1028A SoC) will probe the master device first. The probe order is given by the PCI device ordering, thus it works because the master device has a "smaller" BDF then the switch device.
Explicitly probe the master device in dsa_port_probe().
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc --- net/dsa-uclass.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 88a8ea9352..242e2be035 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -284,6 +284,14 @@ static int dsa_port_probe(struct udevice *pdev) if (!master) return -ENODEV;
+ /* + * Probe the master device. We depend on the master device for proper + * operation and we also need it for MAC inheritance below. + */ + ret = device_probe(master); + if (ret) + return ret; + /* * Inherit port's hwaddr from the DSA master, unless the port already * has a unique MAC address specified in the environment.
On Tue, Feb 23, 2021 at 05:19:06PM +0100, Michael Walle wrote:
DSA needs to have the master device probed first for MAC inheritance. Until now, it only works by chance because the only user (LS1028A SoC) will probe the master device first. The probe order is given by the PCI device ordering, thus it works because the master device has a "smaller" BDF then the switch device.
Explicitly probe the master device in dsa_port_probe().
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
By the way we had this in the old driver that marinated too much and never got merged, I am not sure why we removed it during the second submission process:
https://github.com/openil/u-boot/commit/2544ed8051d3dce55b12e13b6c2b476733d1...
Am 2021-02-23 17:19, schrieb Michael Walle:
DSA needs to have the master device probed first for MAC inheritance. Until now, it only works by chance because the only user (LS1028A SoC) will probe the master device first. The probe order is given by the PCI device ordering, thus it works because the master device has a "smaller" BDF then the switch device.
Explicitly probe the master device in dsa_port_probe().
Fixes: fc054d563bfb ("net: Introduce DSA class for Ethernet switches") Signed-off-by: Michael Walle michael@walle.cc
net/dsa-uclass.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 88a8ea9352..242e2be035 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -284,6 +284,14 @@ static int dsa_port_probe(struct udevice *pdev) if (!master) return -ENODEV;
- /*
* Probe the master device. We depend on the master device for proper* operation and we also need it for MAC inheritance below.*/- ret = device_probe(master);
btw, there is a "int ret" missing above. Will be fixed in the non-RFC version.
- if (ret)
return ret;- /*
- Inherit port's hwaddr from the DSA master, unless the port already
- has a unique MAC address specified in the environment.
participants (2)
-
Michael Walle -
Vladimir Oltean