Re: [PATCH 1/1] net: dsa: Fix of kernel panic in case of missing PHY.

[Andrey Volkov <andrey.volkov@nexvision.fr>]

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) {
>>
> 
>