| Message ID | 54873205.30401@nexvision.fr |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/12/14 09:31, Andrey Volkov wrote: > Fix of kernel panic in case of missing PHY. Humm, I can trust you, but I would need more context on how you could trigger a kernel panic here, the only code path that I could imagine is problematic is the end of dsa_slave_phy_setup(): if (!p->phy) { p->phy = ds->slave_mii_bus->phy_map[p->port]; phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, p->phy_interface); } else { netdev_info(slave_dev, "attached PHY at address %d [%s]\n", p->phy->addr, p->phy->drv->name); } Did you observe this with a pure platform_devices/data only setup, or was that with Device Tree? Thanks! > > Signed-off-by: Andrey Volkov <andrey.volkov@nexvision.fr> > --- > net/dsa/slave.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 528380a..6f89caa 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -512,7 +512,7 @@ static int dsa_slave_fixed_link_update(struct net_device *dev, > } > > /* slave device setup *******************************************************/ > -static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > +static int dsa_slave_phy_setup(struct dsa_slave_priv *p, > struct net_device *slave_dev) > { > struct dsa_switch *ds = p->parent; > @@ -533,7 +533,7 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > ret = of_phy_register_fixed_link(port_dn); > if (ret) { > netdev_err(slave_dev, "failed to register fixed PHY\n"); > - return; > + return ret; > } > phy_is_fixed = true; > phy_dn = port_dn; > @@ -555,12 +555,17 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > */ > if (!p->phy) { > p->phy = ds->slave_mii_bus->phy_map[p->port]; > - phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > + if(p->phy) > + phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > p->phy_interface); > + else > + return -ENODEV; > + > } else { > netdev_info(slave_dev, "attached PHY at address %d [%s]\n", > p->phy->addr, p->phy->drv->name); > } > + return 0; > } > > int dsa_slave_suspend(struct net_device *slave_dev) > @@ -653,7 +658,13 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent, > p->old_link = -1; > p->old_duplex = -1; > > - dsa_slave_phy_setup(p, slave_dev); > + ret = dsa_slave_phy_setup(p, slave_dev); > + if (ret) { > + netdev_err(master, "error %d registering interface %s\n", > + ret, slave_dev->name); > + free_netdev(slave_dev); > + return NULL; > + } > > ret = register_netdev(slave_dev); > if (ret) { > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/09/2014 08:31 PM, Andrey Volkov wrote: > Fix of kernel panic in case of missing PHY. > Signed-off-by: Andrey Volkov <andrey.volkov@nexvision.fr> > --- > net/dsa/slave.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 528380a..6f89caa 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c [...] > @@ -555,12 +555,17 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > */ > if (!p->phy) { > p->phy = ds->slave_mii_bus->phy_map[p->port]; > - phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > + if(p->phy) Space is needed after *if*. Run your patches thru scripts/checkpatch.pl, it should detect such coding style issues. > + phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > p->phy_interface); This continuation line should be realigned now, to start right under 'slave_dev' on the previous line. [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/12/14 09:31, Andrey Volkov wrote: > Fix of kernel panic in case of missing PHY. > > Signed-off-by: Andrey Volkov <andrey.volkov@nexvision.fr> Brian has actually been able to reproduce such a crash in this code-path today: if (!p->phy) { p->phy = ds->slave_mii_bus->phy_map[p->port]; phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, p->phy_interface); } we basically assume here that we have a valid phy pointer out of ds->slave_mii_bus->phy_map[p->port] which is not true in all cases, especially not if the device is not there. I will come up with a fix for that, as for propagating the error code down to the caller, this can be a separate patch. Thanks! > --- > net/dsa/slave.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 528380a..6f89caa 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -512,7 +512,7 @@ static int dsa_slave_fixed_link_update(struct net_device *dev, > } > > /* slave device setup *******************************************************/ > -static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > +static int dsa_slave_phy_setup(struct dsa_slave_priv *p, > struct net_device *slave_dev) > { > struct dsa_switch *ds = p->parent; > @@ -533,7 +533,7 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > ret = of_phy_register_fixed_link(port_dn); > if (ret) { > netdev_err(slave_dev, "failed to register fixed PHY\n"); > - return; > + return ret; > } > phy_is_fixed = true; > phy_dn = port_dn; > @@ -555,12 +555,17 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, > */ > if (!p->phy) { > p->phy = ds->slave_mii_bus->phy_map[p->port]; > - phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > + if(p->phy) > + phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, > p->phy_interface); > + else > + return -ENODEV; > + > } else { > netdev_info(slave_dev, "attached PHY at address %d [%s]\n", > p->phy->addr, p->phy->drv->name); > } > + return 0; > } > > int dsa_slave_suspend(struct net_device *slave_dev) > @@ -653,7 +658,13 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent, > p->old_link = -1; > p->old_duplex = -1; > > - dsa_slave_phy_setup(p, slave_dev); > + ret = dsa_slave_phy_setup(p, slave_dev); > + if (ret) { > + netdev_err(master, "error %d registering interface %s\n", > + ret, slave_dev->name); > + free_netdev(slave_dev); > + return NULL; > + } > > ret = register_netdev(slave_dev); > if (ret) { > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 10/12/2014 04:23, Florian Fainelli wrote : > On 09/12/14 09:31, Andrey Volkov wrote: >> Fix of kernel panic in case of missing PHY. >> >> Signed-off-by: Andrey Volkov <andrey.volkov@nexvision.fr> > > Brian has actually been able to reproduce such a crash in this code-path > today: > > if (!p->phy) { > p->phy = ds->slave_mii_bus->phy_map[p->port]; > phy_connect_direct(slave_dev, p->phy, > dsa_slave_adjust_link, > p->phy_interface); > } > > we basically assume here that we have a valid phy pointer out of > ds->slave_mii_bus->phy_map[p->port] which is not true in all cases, > especially not if the device is not there. Yes it's what really happened in my case: some PHYs were not soldered (development version of the board). But in real life, they may be missed for various reasons, so that the assumption was wrong. > > I will come up with a fix for that, as for propagating the error code > down to the caller, this can be a separate patch. Ok, I'll wait for it > > Thanks! ²You are welcome! > >> --- >> net/dsa/slave.c | 19 +++++++++++++++---- >> 1 file changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 528380a..6f89caa 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -512,7 +512,7 @@ static int dsa_slave_fixed_link_update(struct net_device *dev, >> } >> >> /* slave device setup *******************************************************/ >> -static void dsa_slave_phy_setup(struct dsa_slave_priv *p, >> +static int dsa_slave_phy_setup(struct dsa_slave_priv *p, >> struct net_device *slave_dev) >> { >> struct dsa_switch *ds = p->parent; >> @@ -533,7 +533,7 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, >> ret = of_phy_register_fixed_link(port_dn); >> if (ret) { >> netdev_err(slave_dev, "failed to register fixed PHY\n"); >> - return; >> + return ret; >> } >> phy_is_fixed = true; >> phy_dn = port_dn; >> @@ -555,12 +555,17 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, >> */ >> if (!p->phy) { >> p->phy = ds->slave_mii_bus->phy_map[p->port]; >> - phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, >> + if(p->phy) >> + phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, >> p->phy_interface); >> + else >> + return -ENODEV; >> + >> } else { >> netdev_info(slave_dev, "attached PHY at address %d [%s]\n", >> p->phy->addr, p->phy->drv->name); >> } >> + return 0; >> } >> >> int dsa_slave_suspend(struct net_device *slave_dev) >> @@ -653,7 +658,13 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent, >> p->old_link = -1; >> p->old_duplex = -1; >> >> - dsa_slave_phy_setup(p, slave_dev); >> + ret = dsa_slave_phy_setup(p, slave_dev); >> + if (ret) { >> + netdev_err(master, "error %d registering interface %s\n", >> + ret, slave_dev->name); >> + free_netdev(slave_dev); >> + return NULL; >> + } >> >> ret = register_netdev(slave_dev); >> if (ret) { >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 528380a..6f89caa 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -512,7 +512,7 @@ static int dsa_slave_fixed_link_update(struct net_device *dev, } /* slave device setup *******************************************************/ -static void dsa_slave_phy_setup(struct dsa_slave_priv *p, +static int dsa_slave_phy_setup(struct dsa_slave_priv *p, struct net_device *slave_dev) { struct dsa_switch *ds = p->parent; @@ -533,7 +533,7 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, ret = of_phy_register_fixed_link(port_dn); if (ret) { netdev_err(slave_dev, "failed to register fixed PHY\n"); - return; + return ret; } phy_is_fixed = true; phy_dn = port_dn; @@ -555,12 +555,17 @@ static void dsa_slave_phy_setup(struct dsa_slave_priv *p, */ if (!p->phy) { p->phy = ds->slave_mii_bus->phy_map[p->port]; - phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, + if(p->phy) + phy_connect_direct(slave_dev, p->phy, dsa_slave_adjust_link, p->phy_interface); + else + return -ENODEV; + } else { netdev_info(slave_dev, "attached PHY at address %d [%s]\n", p->phy->addr, p->phy->drv->name); } + return 0; } int dsa_slave_suspend(struct net_device *slave_dev) @@ -653,7 +658,13 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent, p->old_link = -1; p->old_duplex = -1; - dsa_slave_phy_setup(p, slave_dev); + ret = dsa_slave_phy_setup(p, slave_dev); + if (ret) { + netdev_err(master, "error %d registering interface %s\n", + ret, slave_dev->name); + free_netdev(slave_dev); + return NULL; + } ret = register_netdev(slave_dev); if (ret) {
Fix of kernel panic in case of missing PHY. Signed-off-by: Andrey Volkov <andrey.volkov@nexvision.fr> --- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html