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 (4): net: dsa: return early if there is no master net: dsa: probe master device net: dsa: remove NULL check for priv and platform data net: dsa: remove master santiy check
net/dsa-uclass.c | 63 ++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 39 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 Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com --- 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 Wed, Feb 24, 2021 at 6:40 PM Michael Walle michael@walle.cc 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
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);-- 2.20.1
Reviewed-By: Ramon Fried rfried.dev@gmail.com
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 --- net/dsa-uclass.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 88a8ea9352..7898f30e15 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -272,6 +272,7 @@ static int dsa_port_probe(struct udevice *pdev) struct dsa_port_pdata *port_pdata; struct dsa_priv *dsa_priv; struct udevice *master; + int ret;
port_pdata = dev_get_parent_plat(pdev); dsa_priv = dev_get_uclass_priv(dev); @@ -284,6 +285,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 Wed, Feb 24, 2021 at 6:40 PM Michael Walle michael@walle.cc 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
net/dsa-uclass.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 88a8ea9352..7898f30e15 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -272,6 +272,7 @@ static int dsa_port_probe(struct udevice *pdev) struct dsa_port_pdata *port_pdata; struct dsa_priv *dsa_priv; struct udevice *master;
int ret; port_pdata = dev_get_parent_plat(pdev); dsa_priv = dev_get_uclass_priv(dev);@@ -284,6 +285,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.-- 2.20.1
Reviewed-By: Ramon Fried rfried.dev@gmail.com
Because the uclass has the "*_auto" properties set, the driver model will take care of allocating the private structures for us and they can't be NULL. Drop the checks.
Signed-off-by: Michael Walle michael@walle.cc --- net/dsa-uclass.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 7898f30e15..d453cc6930 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -28,8 +28,8 @@ int dsa_set_tagging(struct udevice *dev, ushort headroom, ushort tailroom) { struct dsa_priv *priv;
- if (!dev || !dev_get_uclass_priv(dev)) - return -ENODEV; + if (!dev) + return -EINVAL;
if (headroom + tailroom > DSA_MAX_OVR) return -EINVAL; @@ -47,11 +47,13 @@ int dsa_set_tagging(struct udevice *dev, ushort headroom, ushort tailroom) /* returns the DSA master Ethernet device */ struct udevice *dsa_get_master(struct udevice *dev) { - struct dsa_priv *priv = dev_get_uclass_priv(dev); + struct dsa_priv *priv;
- if (!priv) + if (!dev) return NULL;
+ priv = dev_get_uclass_priv(dev); + return priv->master_dev; }
@@ -67,9 +69,6 @@ static int dsa_port_start(struct udevice *pdev) struct dsa_ops *ops = dsa_get_ops(dev); int err;
- if (!priv) - return -ENODEV; - if (!master) { dev_err(pdev, "DSA master Ethernet device not found!\n"); return -EINVAL; @@ -101,9 +100,6 @@ static void dsa_port_stop(struct udevice *pdev) struct udevice *master = dsa_get_master(dev); struct dsa_ops *ops = dsa_get_ops(dev);
- if (!priv) - return; - if (ops->port_disable) { struct dsa_port_pdata *port_pdata;
@@ -347,7 +343,7 @@ static int dsa_post_bind(struct udevice *dev) ofnode node = dev_ofnode(dev), pnode; int i, err, first_err = 0;
- if (!pdata || !ofnode_valid(node)) + if (!ofnode_valid(node)) return -ENODEV;
pdata->master_node = ofnode_null(); @@ -459,9 +455,6 @@ static int dsa_pre_probe(struct udevice *dev) struct dsa_pdata *pdata = dev_get_uclass_plat(dev); struct dsa_priv *priv = dev_get_uclass_priv(dev);
- if (!pdata || !priv) - return -ENODEV; - priv->num_ports = pdata->num_ports; priv->cpu_port = pdata->cpu_port; priv->cpu_port_fixed_phy = fixed_phy_create(pdata->cpu_port_node);
On Wed, Feb 24, 2021 at 05:40:41PM +0100, Michael Walle wrote:
Because the uclass has the "*_auto" properties set, the driver model will take care of allocating the private structures for us and they can't be NULL. Drop the checks.
Signed-off-by: Michael Walle michael@walle.cc
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
On Thu, Feb 25, 2021 at 12:23 AM Vladimir Oltean olteanv@gmail.com wrote:
On Wed, Feb 24, 2021 at 05:40:41PM +0100, Michael Walle wrote:
Because the uclass has the "*_auto" properties set, the driver model will take care of allocating the private structures for us and they can't be NULL. Drop the checks.
Signed-off-by: Michael Walle michael@walle.cc
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
Reviewed-By: Ramon Fried rfried.dev@gmail.com
Because we probe the master ourselves (and fail if there is no master), it is not possible that we don't have a master device.
There is one catch though: device removal. We don't support that. It wasn't supported neither before this patch. Because the master device was only set in .pre_probe(), if a device was removed master_dev was a dangling pointer and transmitting a frame cause a panic. I don't see a good solution without having some sort of notify machanism when a udevice is removed.
Signed-off-by: Michael Walle michael@walle.cc --- net/dsa-uclass.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index d453cc6930..7ea1cb6949 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -69,11 +69,6 @@ static int dsa_port_start(struct udevice *pdev) struct dsa_ops *ops = dsa_get_ops(dev); int err;
- if (!master) { - dev_err(pdev, "DSA master Ethernet device not found!\n"); - return -EINVAL; - } - if (ops->port_enable) { struct dsa_port_pdata *port_pdata;
@@ -108,13 +103,7 @@ static void dsa_port_stop(struct udevice *pdev) ops->port_disable(dev, priv->cpu_port, NULL); }
- /* - * 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); + eth_get_ops(master)->stop(master); }
/* @@ -133,9 +122,6 @@ static int dsa_port_send(struct udevice *pdev, void *packet, int length) struct dsa_port_pdata *port_pdata; int err;
- if (!master) - return -EINVAL; - if (length + head + tail > PKTSIZE_ALIGN) return -EINVAL;
@@ -165,9 +151,6 @@ static int dsa_port_recv(struct udevice *pdev, int flags, uchar **packetp) struct dsa_port_pdata *port_pdata; int length, port_index, err;
- if (!master) - return -EINVAL; - length = eth_get_ops(master)->recv(master, flags, packetp); if (length <= 0) return length; @@ -201,9 +184,6 @@ static int dsa_port_free_pkt(struct udevice *pdev, uchar *packet, int length) struct udevice *master = dsa_get_master(dev); struct dsa_priv *priv;
- if (!master) - return -EINVAL; - priv = dev_get_uclass_priv(dev); if (eth_get_ops(master)->free_pkt) { /* return the original pointer and length to master Eth */ @@ -284,6 +264,9 @@ static int dsa_port_probe(struct udevice *pdev) /* * Probe the master device. We depend on the master device for proper * operation and we also need it for MAC inheritance below. + * + * TODO: we assume the master device is always there and doesn't get + * removed during runtime. */ ret = device_probe(master); if (ret)
On Wed, Feb 24, 2021 at 05:40:42PM +0100, Michael Walle wrote:
Because we probe the master ourselves (and fail if there is no master), it is not possible that we don't have a master device.
There is one catch though: device removal. We don't support that. It wasn't supported neither before this patch. Because the master device was only set in .pre_probe(), if a device was removed master_dev was a dangling pointer and transmitting a frame cause a panic. I don't see a good solution without having some sort of notify machanism when a udevice is removed.
Signed-off-by: Michael Walle michael@walle.cc
Ha, this is a tough one. So the DSA master is not a parent of the DSA switch, instead the SPI bus/whatever is. Additionally, the checks for NULL are pointless, because the master is not NULL, just deallocated.
In Linux we solved this by using device links: https://github.com/torvalds/linux/commit/07b90056cb15ff9877dca0d8f1b6583d105...
However it doesn't seem like we have such a generic dependency graph between udevices in U-Boot, do we? I could just find device_reparent, which is definitely not what we want.
My best guess as to how to solve the DSA master removal scenario would be to: (a) scatter all DSA callbacks with device_probe(master), so even if it was unbound, it will be rebound. (b) introduce DSA specific code in eth_pre_remove and a DSA-specific pointer in eth_pdata to manually call device_remove for an attached DSA switch, if that exists.
Other ideas are welcome, of course.
Am 2021-02-24 18:11, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 05:40:42PM +0100, Michael Walle wrote:
Because we probe the master ourselves (and fail if there is no master), it is not possible that we don't have a master device.
There is one catch though: device removal. We don't support that. It wasn't supported neither before this patch. Because the master device was only set in .pre_probe(), if a device was removed master_dev was a dangling pointer and transmitting a frame cause a panic. I don't see a good solution without having some sort of notify machanism when a udevice is removed.
Signed-off-by: Michael Walle michael@walle.cc
Ha, this is a tough one. So the DSA master is not a parent of the DSA switch, instead the SPI bus/whatever is. Additionally, the checks for NULL are pointless, because the master is not NULL, just deallocated.
In Linux we solved this by using device links: https://github.com/torvalds/linux/commit/07b90056cb15ff9877dca0d8f1b6583d105...
However it doesn't seem like we have such a generic dependency graph between udevices in U-Boot, do we? I could just find device_reparent, which is definitely not what we want.
I didn't find anything else either. Thus I just ignored it for the moment.
My best guess as to how to solve the DSA master removal scenario would be to: (a) scatter all DSA callbacks with device_probe(master), so even if it was unbound, it will be rebound. (b) introduce DSA specific code in eth_pre_remove and a DSA-specific pointer in eth_pdata to manually call device_remove for an attached DSA switch, if that exists.
Other ideas are welcome, of course.
What is the reason to remove that device in the first place? Like is this really a valid scenario? I really don't know when a device is removed and if its remove, will it still be there or is it rather a hot-plug type and rebinding it won't work anyways.
I also had (b) in mind, but again. Does it really apply? After all, this is a bootloader ;)
On Wed, Feb 24, 2021 at 06:29:39PM +0100, Michael Walle wrote:
What is the reason to remove that device in the first place? Like is this really a valid scenario? I really don't know when a device is removed and if its remove, will it still be there or is it rather a hot-plug type and rebinding it won't work anyways.
Did you get this to crash under any circumstance other than using the 'unbind' command?
Am 2021-02-24 18:45, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 06:29:39PM +0100, Michael Walle wrote:
What is the reason to remove that device in the first place? Like is this really a valid scenario? I really don't know when a device is removed and if its remove, will it still be there or is it rather a hot-plug type and rebinding it won't work anyways.
Did you get this to crash under any circumstance other than using the 'unbind' command?
Nope, thus I was curious about that comment in dsa_port_stop(). Someone (Alex, Claudiu maybe?) must have something in mind when writing about it. But I couldn't figure out in which case a device is removed.
-michael
On Wed, Feb 24, 2021 at 07:08:51PM +0100, Michael Walle wrote:
Am 2021-02-24 18:45, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 06:29:39PM +0100, Michael Walle wrote:
What is the reason to remove that device in the first place? Like is this really a valid scenario? I really don't know when a device is removed and if its remove, will it still be there or is it rather a hot-plug type and rebinding it won't work anyways.
Did you get this to crash under any circumstance other than using the 'unbind' command?
Nope, thus I was curious about that comment in dsa_port_stop(). Someone (Alex, Claudiu maybe?) must have something in mind when writing about it. But I couldn't figure out in which case a device is removed.
I'm pretty sure that the checks that are in place now were once written so that the sandbox tests would pass. If they still do, we should be fine.
You can run the sandbox tests using:
make sandbox_defconfig NO_SDL=1 make -j 8 NO_SDL=1 ./u-boot -d ./arch/sandbox/dts/test.dtb setenv ethact swp0 ping 1.2.3.5 ut dm dsa_probe ut dm dsa ut dm ut dm net_retry
Am 2021-02-24 19:19, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 07:08:51PM +0100, Michael Walle wrote:
Am 2021-02-24 18:45, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 06:29:39PM +0100, Michael Walle wrote:
What is the reason to remove that device in the first place? Like is this really a valid scenario? I really don't know when a device is removed and if its remove, will it still be there or is it rather a hot-plug type and rebinding it won't work anyways.
Did you get this to crash under any circumstance other than using the 'unbind' command?
Nope, thus I was curious about that comment in dsa_port_stop(). Someone (Alex, Claudiu maybe?) must have something in mind when writing about it. But I couldn't figure out in which case a device is removed.
I'm pretty sure that the checks that are in place now were once written so that the sandbox tests would pass. If they still do, we should be fine.
Ah.
You can run the sandbox tests using:
Just if one is trying to follow this thread: you'll also need to have the following series applied: https://patchwork.ozlabs.org/project/uboot/list/?series=229778
make sandbox_defconfig NO_SDL=1 make -j 8 NO_SDL=1 ./u-boot -d ./arch/sandbox/dts/test.dtb
btw theres a shortcut for this "u-boot -T"
setenv ethact swp0
"setenv ethact lan0" I guess
ping 1.2.3.5 ut dm dsa_probe ut dm dsa ut dm ut dm net_retry
Not more failures than without my patch.
-michael
On Wed, Feb 24, 2021 at 05:40:42PM +0100, Michael Walle wrote:
Because we probe the master ourselves (and fail if there is no master), it is not possible that we don't have a master device.
There is one catch though: device removal. We don't support that. It wasn't supported neither before this patch. Because the master device was only set in .pre_probe(), if a device was removed master_dev was a dangling pointer and transmitting a frame cause a panic. I don't see a good solution without having some sort of notify machanism when a udevice is removed.
Signed-off-by: Michael Walle michael@walle.cc
U-Boot DM maintainers might want to comment on this as well, but with the knowledge I have this seems good enough to me:
Reviewed-by: Vladimir Oltean vladimir.oltean@nxp.com
Am 2021-02-24 17:40, schrieb Michael Walle:
Because we probe the master ourselves (and fail if there is no master), it is not possible that we don't have a master device.
There is one catch though: device removal. We don't support that. It wasn't supported neither before this patch. Because the master device was only set in .pre_probe(), if a device was removed master_dev was a dangling pointer and transmitting a frame cause a panic. I don't see a good solution without having some sort of notify machanism when a udevice is removed.
Signed-off-by: Michael Walle michael@walle.cc
FWIW
Tested-by: Michael Walle michael@walle.cc [DSA unit tests]
participants (3)
-
Michael Walle -
Ramon Fried -
Vladimir Oltean