Re: [Qemu-devel] [RFC PATCH v1 1/2] e1000: clean up set_phy_ctrl function

[Gabriel L. Somlo]

On Mon, Jun 30, 2014 at 09:04:12PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> > set_phy_ctrl() runs the same autonegotiation availability checks
> > that were factored out into have_autoneg() in an earlier commit
> > (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> > on the value about to be written into the phy_ctrl register, and
> > then does not actually write the register.
> > 
> > This patch moves the have_autoneg() function in front of
> > set_phy_ctrl(), then updates the latter to actually write
> > the register with the given value, and use the factored-out
> > check to determine whether the link should be bounced.
> > 
> > Also, fix indentation/line width when setting up the timer.
> > 
> > Signed-off-by: Gabriel Somlo <address@hidden>
> 
> Weird.
> So does this mean have_autoneg is always false then?
> How does it work?

phy_reg[PHY_CTRL] is initialized (along with the rest of phy_reg)
by a memmove from phy_reg_init, during e1000_reset().

So the question would be rather "does it make sense to look at
anything besides (s->compat_flags & E1000_FLAG_AUTONEG), since
we never allow phy_reg[PHY_CTRL] to change after initialization,
so MII_CR_AUTO_NEG_EN and MII_CR_RESTART_AUTO_NEG bits will always
be set?"

> Wrt to patch itself, let's do it properly pls:
> - bits 0:5 are RO on some cards
> - bit 9 is SC

Will do, once we've sorted out the phy_reg[] mystery :)

Thanks,
--Gabriel

> 
> > ---
> > 
> > I'm not 100% sure whether "val" in set_phy_ctrl() is not written
> > to phy_reg[PHY_CTRL] by mistake, or whether there's an ulterior
> > motive (in which case we should add a comment as to why that is).
> > 
> > Assuming it's a mistake, and we *should* have been writing it (as
> > per my patch below), we can then use have_autoneg() to also simplify
> > the code significantly :)
> > 
> > Thanks,
> >   Gabriel
> > 
> >  hw/net/e1000.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 0fc29a0..2376910 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -186,6 +186,14 @@ e1000_link_up(E1000State *s)
> >      s->phy_reg[PHY_STATUS] |= MII_SR_LINK_STATUS;
> >  }
> >  
> > +static bool
> > +have_autoneg(E1000State *s)
> > +{
> > +    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > +           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > +           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > +}
> > +
> >  static void
> >  set_phy_ctrl(E1000State *s, int index, uint16_t val)
> >  {
> > @@ -194,13 +202,14 @@ set_phy_ctrl(E1000State *s, int index, uint16_t val)
> >       * migrate during auto negotiation, after migration the link will be
> >       * down.
> >       */
> > -    if (!(s->compat_flags & E1000_FLAG_AUTONEG)) {
> > -        return;
> > -    }
> > -    if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
> > +
> > +    s->phy_reg[PHY_CTRL] = val;
> > +
> > +    if (have_autoneg(s)) {
> >          e1000_link_down(s);
> >          DBGOUT(PHY, "Start link auto negotiation\n");
> > -        timer_mod(s->autoneg_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) 
> > + 500);
> > +        timer_mod(s->autoneg_timer,
> > +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 500);
> >      }
> >  }
> >  
> > @@ -848,14 +857,6 @@ receive_filter(E1000State *s, const uint8_t *buf, int 
> > size)
> >      return 0;
> >  }
> >  
> > -static bool
> > -have_autoneg(E1000State *s)
> > -{
> > -    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
> > -           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN) &&
> > -           (s->phy_reg[PHY_CTRL] & MII_CR_RESTART_AUTO_NEG);
> > -}
> > -
> >  static void
> >  e1000_set_link_status(NetClientState *nc)
> >  {
> > -- 
> > 1.9.3