netdev - Re: [PATCH 3/3] mv643xx_eth: convert to phylib

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] [day] [month] [year] [list]

Message-ID: <20080919175256.GB23288@xi.wantstofly.org>
Date:	Fri, 19 Sep 2008 19:52:56 +0200
From:	Lennert Buytenhek <buytenh@...tstofly.org>
To:	Andy Fleming <afleming@...il.com>
Cc:	Andy Fleming <afleming@...escale.com>, netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] mv643xx_eth: convert to phylib

On Thu, Sep 18, 2008 at 05:47:03PM -0500, Andy Fleming wrote:

> > I'll make this function:
> >
> >        static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)
> >        {
> >                struct phy_device *phy = mp->phy;
> >
> >                phy_reset(mp);
> >
> >                phy_attach(mp->dev, phy->dev.bus_id, 0, PHY_INTERFACE_MODE_GMII);
> >
> >                phy->state = PHY_HALTED;
> >
> >                if (speed == 0) {
> >                        phy->autoneg = AUTONEG_ENABLE;
> >                        phy->speed = 0;
> >                        phy->duplex = 0;
> >                        phy->advertising = phy->supported | ADVERTISED_Autoneg;
> >                } else {
> >                        phy->autoneg = AUTONEG_DISABLE;
> >                        phy->advertising = 0;
> >                        phy->speed = speed;
> >                        phy->duplex = duplex;
> >                }
> >                phy_start_aneg(phy);
> >        }
> >
> > OK?  (PHY_HALTED because that seems to be the way to stop the state
> > machine from running at all.  Or should I call phy_stop() once?)
> 
> You don't need to disable the state machine.  With phy_attach, it
> won't start unless you call phy_start_machine().  So the state changes
> won't cause the state machine to run.  If the state information is
> interfering with things, let me know.  We might have to create a state
> for "state machine is not actually running".

OK.  From a look at the state handling, trying to make sure that it's
in HALTED seemed like the safe option, but as you say, just the one
call to phy_start_aneg() shouldn't mess things up -- so I'll get rid
of that.


> >> You will, however, need to
> >> figure out how best to read out the status for the PHY, and how to
> >> know when autonegotiation is done.  I'm assuming that your controller
> >> tracks that information, and that you can manually pull that
> >> information from local register space (rather than the SMI bus).
> >
> > I don't really need to access it all during normal operation -- the
> > controller just reads the link up/down and speed and duplex bits from
> > the PHY automatically and then configures itself accordingly.  The
> > only time I need to access this information is when someone asks for
> > it via ethtool (or if I want to print a link status message).  I.e. I
> > don't need any state machine, be it phylib's or my own.
> 
> Right, but doesn't your driver react to the link going up and down by
> changing the carrier state?  If so, then it costs nothing to cache
> that state in the phydev->link, and then a request for link status can
> just return phydev->link.  Those are decisions you can make, though,
> as I'm not familiar with your device and your driver.

Or, I can just return !!netif_carrier_ok(dev), since that'll always
be the same thing in this case.


> If you could check those things, you can submit the patch with:
> 
> Acked-by: Andy Fleming <afleming@...escale.com>

Thanks for the review!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html