[PATCH 2/2] netdev/fec: fix performance impact from mdio poll operation

[Bryan Wu]

On 04/13/2010 12:09 PM, Andy Whitcroft wrote:
> On Thu, Apr 08, 2010 at 02:44:59PM +0800, Bryan Wu wrote:
>> BugLink: http://bugs.launchpad.net/bugs/546649
>> BugLink: http://bugs.launchpad.net/bugs/457878
>>
>> After introducing phylib supporting, users experienced performace drop. That is
>> because of the mdio polling operation of phylib. Use msleep to replace the busy
>> waiting cpu_relax() and remove the warning message.
>>
>> Signed-off-by: Bryan Wu<bryan.wu at canonical.com>
>> ---
>>   drivers/net/fec.c |   45 +++++++++++++++++++++------------------------
>>   1 files changed, 21 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>> index 53240d3..2280373 100644
>> --- a/drivers/net/fec.c
>> +++ b/drivers/net/fec.c
>> @@ -216,7 +216,7 @@ static void fec_stop(struct net_device *dev);
>>   #define FEC_MMFR_TA		(2<<  16)
>>   #define FEC_MMFR_DATA(v)	(v&  0xffff)
>>
>> -#define FEC_MII_TIMEOUT		10000
>> +#define FEC_MII_TIMEOUT		10
>>
>>   /* Transmitter timeout */
>>   #define TX_TIMEOUT (2 * HZ)
>> @@ -624,13 +624,29 @@ spin_unlock:
>>   /*
>>    * NOTE: a MII transaction is during around 25 us, so polling it...
>>    */
>> -static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>> +static int fec_enet_mdio_poll(struct fec_enet_private *fep)
>>   {
>> -	struct fec_enet_private *fep = bus->priv;
>>   	int timeout = FEC_MII_TIMEOUT;
>>
>>   	fep->mii_timeout = 0;
>>
>> +	/* wait for end of transfer */
>> +	while (!(readl(fep->hwp + FEC_IEVENT)&  FEC_ENET_MII)) {
>> +		msleep(1);
>> +		if (timeout--<  0) {
>> +			fep->mii_timeout = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>> +{
>> +	struct fec_enet_private *fep = bus->priv;
>> +
>> +
>>   	/* clear MII end of transfer bit*/
>>   	writel(FEC_ENET_MII, fep->hwp + FEC_IEVENT);
>>
>> @@ -639,15 +655,7 @@ static int fec_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>>   		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
>>   		FEC_MMFR_TA, fep->hwp + FEC_MII_DATA);
>>
>> -	/* wait for end of transfer */
>> -	while (!(readl(fep->hwp + FEC_IEVENT)&  FEC_ENET_MII)) {
>> -		cpu_relax();
>> -		if (timeout--<  0) {
>> -			fep->mii_timeout = 1;
>> -			printk(KERN_ERR "FEC: MDIO read timeout\n");
>> -			return -ETIMEDOUT;
>> -		}
>> -	}
>> +	fec_enet_mdio_poll(fep);
>
> You have pulled out this timeout loop, but in the replacement there is
> not indication that it has timed-out.  In the old code we would have
> returned -ETIMEDOUT and not touched MII_DATA (below).  In the new we
> will timeout, mark mii_timeout but not return an indication of that, not
> would we note it here, and so we would drop through and touch MII_DATA.
> Is this an intentional change of semantics?
>

Since the timeout thing is not an error just a warning, the caller in phylib 
timer will call this function shortly and normally ignore the negative return 
value. I just looked up this busy timeout things in drivers/net/.

drivers/net/davinci_emac.c:
it uses endless loop:
/* Wait until mdio is ready for next command */
#define MDIO_WAIT_FOR_USER_ACCESS\
                 while ((emac_mdio_read((MDIO_USERACCESS(0))) &\
                         MDIO_USERACCESS_GO) != 0)


drivers/net/au1000_eth.c
it returns -1 when timeout

drivers/net/arm/at91_ether.c
it is the same as us, don't return any thing if timeout.

So actually, I am not sure about the return value requirement for this function. 
And I found several timeout warning when we do large file transfer. The hardware 
is not perfect.

But I will try to check the return value of the poll function, and let you guys 
know the result.

Thanks,
-Bryan