net: dsa: Fix of kernel panic in case of missing PHY.
Subsystems (3)
PATCH 1/1 net: dsa: Fix of kernel panic in case of missing PHY.
Fix of kernel panic in case of missing PHY.
Signed-off-by Andrey Volkov <[email protected]>
--- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
net/dsa/slave.c +15 -4
@@ -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) {
Quoted 1 line · Andrey Volkov
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!
Quoted 63 lines
Signed-off-by Andrey Volkov <[email protected]>--- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)net/dsa/slave.c +15
-4@@ -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) {
Hello.
Quoted 11 lines · Andrey Volkov
Fix of kernel panic in case of missing PHY.
Signed-off-by Andrey Volkov <[email protected]>--- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)net/dsa/slave.c +0
-0
[...]
net/dsa/slave.c static void dsa_slave_phy_setup(struct dsa_slave_priv *p, · line 558
Quoted 6 lines
net/dsa/slave.c +1
-1@@ -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.
net/dsa/slave.c static void dsa_slave_phy_setup(struct dsa_slave_priv *p, · line 560
Quoted 2 lines
net/dsa/slave.c +1
-0+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
Quoted 3 lines · Andrey Volkov
Fix of kernel panic in case of missing PHY.
Signed-off-by Andrey Volkov <[email protected]>
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!
Quoted 61 lines
--- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)net/dsa/slave.c +15
-4@@ -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) {
Quoted 15 lines · Florian Fainelli
Quoted 3 lines · Andrey Volkov
Fix of kernel panic in case of missing PHY.
Signed-off-by Andrey Volkov <[email protected]>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); }
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 2 lines
Thanks!
²You are welcome!
Quoted 64 lines
Quoted 62 lines
--- net/dsa/slave.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)net/dsa/slave.c +15
-4@@ -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) {