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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Sep 2016 13:42:59 -0500
From:   Jeremy Linton <jeremy.linton@....com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, steve.glendinning@...well.net,
        sergei.shtylyov@...entembedded.com
Subject: Re: [PATCH 1/2] net: smsc911x: Fix register_netdev, phy startup,
 driver unload ordering

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...