lkml.org 
[lkml]   [2007]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [patch 1/1] network: add the missing phy_device speed information to phy_mii_ioctl
Date

On Mar 2, 2007, at 11:42, Shan Lu wrote:

> Changelog:
> Function `phy_mii_ioctl' returns physical device's information
> based on
> user requests. When requested to return the basic mode control
> register
> information (BMCR), the original implementation only returns the
> physical device's duplex information and forgets to return speed
> information, which should not be because BMCR register is used to hold
> both duplex and speed information.
>
> The patch checks the BMCR value against speed-related flags and fills
> the return structure's speed field accordingly.


I think you've misunderstood what this code does, though I agree, in
principle, with your change. You have modified the SIOCSMIIREG
command, which takes in a value, and sets the given register to that
value. The structure which is being filled in is not being
returned. It is the PHY's software representation at runtime.

So this patch probably fixes a bug (I'd have to think harder about it
to see if phydev->speed doesn't get updated automatically), but
you've modified the *setting* command, not the *getting* command.

Is there a reason you are using this function to get and set the
BMCR? The PHY Lib provides ethtool functions to do this, and they
fit better with the design of the PHY Lib. The mii IOCTL allows you
to change the registers any way you want, which potentially breaks
the state machine the PHY Lib sets up for tracking such changes.

All that said, I think the change below needs to go in. See below
for inline commentary:


>
> Signed-off-by: Shan<shanlu@cs.uiuc.edu>
>
> ---
> --- drivers/net/phy/phy.c 2007-03-02 10:40:26.000000000 -0600
> 2.6.20
> +++ drivers/net/phy/phy.c 2007-03-02 10:41:39.000000000 -0600
> @@ -337,6 +337,10 @@ int phy_mii_ioctl(struct phy_device *phy
> phydev->duplex = DUPLEX_FULL;
> else
> phydev->duplex = DUPLEX_HALF;
> + if ((!phydev->autoneg) && (val
> &BMCR_SPEED1000))
> + phydev->speed = SPEED_1000;
> + else if ((!phydev->autoneg) && (val &
> BMCR_SPEED100))
> + phydev->speed = SPEED_100;


Why not also support SPEED_10 if neither bits are set?

Andy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-03-06 18:39    [W:0.851 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site