* Re: [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY.
2014-12-09 17:31 [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY Andrey Volkov
@ 2014-12-09 18:37 ` Florian Fainelli
2014-12-09 19:45 ` Sergei Shtylyov
2014-12-10 3:23 ` Florian Fainelli
2 siblings, 0 replies; 5+ messages in thread
From: Florian Fainelli @ 2014-12-09 18:37 UTC (permalink / raw)
To: Andrey Volkov, netdev
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) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY.
2014-12-09 17:31 [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY Andrey Volkov
2014-12-09 18:37 ` Florian Fainelli
@ 2014-12-09 19:45 ` Sergei Shtylyov
2014-12-10 3:23 ` Florian Fainelli
2 siblings, 0 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2014-12-09 19:45 UTC (permalink / raw)
To: Andrey Volkov, netdev; +Cc: Florian Fainelli
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY.
2014-12-09 17:31 [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY Andrey Volkov
2014-12-09 18:37 ` Florian Fainelli
2014-12-09 19:45 ` Sergei Shtylyov
@ 2014-12-10 3:23 ` Florian Fainelli
2014-12-10 10:06 ` Andrey Volkov
2 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2014-12-10 3:23 UTC (permalink / raw)
To: Andrey Volkov, netdev, Brian Norris
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) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY.
2014-12-10 3:23 ` Florian Fainelli
@ 2014-12-10 10:06 ` Andrey Volkov
0 siblings, 0 replies; 5+ messages in thread
From: Andrey Volkov @ 2014-12-10 10:06 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, Brian Norris
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) {
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread