[PATCH 2/2] netdev/fec: fix performance impact from mdio poll operation
Bryan Wu
bryan.wu at canonical.com
Tue Apr 13 21:37:12 UTC 2010
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
More information about the kernel-team
mailing list