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

[Michael S. Tsirkin]

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?

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

> ---
> 
> 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