[1/2] net: smsc911x: Fix register_netdev, phy startup, driver unload ordering

Move phy startup/shutdown into the smsc911x_open/stop routines. This
allows the module to be unloaded because phy_connect_direct is no longer
always holding the module use count. This one change also resolves a
number of other problems.

The link status of a downed interface no longer reflects a stale state.
Errors caused by the net device being opened before the mdio/phy was
configured. There is also a potential power savings as the phy's don't
remain powered when the interface isn't running.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 51 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

> @@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
>  	unsigned int timeout;
>  	unsigned int temp;
>  	unsigned int intcfg;
> +	int retval;
>  
> -	/* if the phy is not yet registered, retry later*/
> +	/* find and start the given phy */
>  	if (!dev->phydev) {
> -		SMSC_WARN(pdata, hw, "phy_dev is NULL");
> -		return -EAGAIN;
> +		if (smsc911x_mii_probe(dev) < 0) {
> +			SMSC_WARN(pdata, probe, "Error starting phy");
> +			retval = -EAGAIN;

smsc911x_mii_probe() returns an error code. It is better to use that,
than -EAGAIN, which is rather odd to start with.

> +			goto out;
> +		}
>  	}
>  
>  	/* Reset the LAN911x */
>  	if (smsc911x_soft_reset(pdata)) {
>  		SMSC_WARN(pdata, hw, "soft reset failed");
> -		return -EIO;
> +		retval = -EIO;
> +		goto mii_free_out;

smsc911x_soft_reset() also returns an error code you should use.

This patch also seems to do quite a few different things. Please can
you break it up into multiple patches.

Thanks
	Andrew

On 09/01/2016 11:58 AM, Andrew Lunn wrote:
>> @@ -1520,17 +1513,22 @@ static int smsc911x_open(struct net_device *dev)
>>  	unsigned int timeout;
>>  	unsigned int temp;
>>  	unsigned int intcfg;
>> +	int retval;
>>
>> -	/* if the phy is not yet registered, retry later*/
>> +	/* find and start the given phy */
>>  	if (!dev->phydev) {
>> -		SMSC_WARN(pdata, hw, "phy_dev is NULL");
>> -		return -EAGAIN;
>> +		if (smsc911x_mii_probe(dev) < 0) {
>> +			SMSC_WARN(pdata, probe, "Error starting phy");
>> +			retval = -EAGAIN;
>
> smsc911x_mii_probe() returns an error code. It is better to use that,
> than -EAGAIN, which is rather odd to start with.

	Thats a good point, I was just maintaining the existing behavior, but 
its definitely better to just return the actual error.

>
>> +			goto out;
>> +		}
>>  	}
>>
>>  	/* Reset the LAN911x */
>>  	if (smsc911x_soft_reset(pdata)) {
>>  		SMSC_WARN(pdata, hw, "soft reset failed");
>> -		return -EIO;
>> +		retval = -EIO;
>> +		goto mii_free_out;
>
> smsc911x_soft_reset() also returns an error code you should use.
>
> This patch also seems to do quite a few different things. Please can
> you break it up into multiple patches.
	

	That was the comment from the previous patch, but It confused me 
because it was only really moving the MDIO startup, the rest was a side 
effect of that and atomic to it (AKA it wasn't clear how to make it 
smaller). I read it as Steve misinterpreting the laundry list of fixes 
as things being individually fixed, rather than one thing fixing a bunch 
of stuff.

This patch does add additional code I overlooked to cleanup the phy if 
it fails, I guess in theory that portion could be a prereq patch, I will 
break that portion out. I'm still not sure how to partially move the 
MDIO startup...

> This patch does add additional code I overlooked to cleanup the phy
> if it fails, I guess in theory that portion could be a prereq patch,
> I will break that portion out. I'm still not sure how to partially
> move the MDIO startup...

Hi Jeremy

You can add a cleanup patch which replaces these hard coded return
values with the value returned by the function calls. Then do the
move.

In general, it is easier to review lots of small obvious patches, than
one big patch with lots of things going on.

    Andrew