| Message ID | 20210223161906.9177-2-michael@walle.cc |
|---|---|
| State | Superseded |
| Delegated to: | Priyanka Jain |
| Headers | show |
| Series | net: dsa: various fixes | expand |
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
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);
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(-)