Re: [Qemu-stable] [Qemu-devel] [PATCH for-1.4] Revert "e1000: no need auto-negotiation if link was down"

[Anthony Liguori]

Michael Roth <address@hidden> writes:

> This reverts commit 84dd2120247a7d25ff1bb337de21c0e76816ad2d.
>
> I'm not sure what issue the original commit was meant to fix, or if
> the logic is actually wrong, but it causes e1000 to stop working
> after a guest issues a reset.
>
>>From what I can tell a guest with an e1000 nic has no way of changing
> the link status, as far as it's NetClient peer is concerned, except
> in the auto-negotiation path, so with this patch in place there's no
> recovery after a reset, since the link goes down and stays that way.
>
> Revert this patch now to fix the bigger problem, and handle any
> lingering issues with a follow-up.
>
> Reproduced/tested with qemu-jeos and Ubuntu 12.10.
>
> Signed-off-by: Michael Roth <address@hidden>
> ---
>  hw/e1000.c |    5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/hw/e1000.c b/hw/e1000.c
> index ef06ca1..563a58f 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -166,11 +166,6 @@ static void
>  set_phy_ctrl(E1000State *s, int index, uint16_t val)
>  {
>      if ((val & MII_CR_AUTO_NEG_EN) && (val & MII_CR_RESTART_AUTO_NEG)) {
> -        /* no need auto-negotiation if link was down */
> -        if (s->nic->nc.link_down) {
> -            s->phy_reg[PHY_STATUS] |= MII_SR_AUTONEG_COMPLETE;
> -            return;
> -        }

The problem with this patch is that it skips autonegotiate if the link
is down however it doesn't take reset into account.

Consider if you reset the guest during autonegotiation.  The link is down
but it's not really down--the guest is in the process of bringing it
back up.

But since it was down before reset, we won't let autonegotiation happen
again.

We shouldn't use nc.link_down state to indicate this.  We should use
another variable that we can clear during reset.

I'm going to take this revert since it fixes a serious problem
(networking doesn't work after a reboot) at the cost of a less serious
problem (bring the link up if it was previously set to be down).  But I
hope we can get a proper fix during the -rc cycle.

Regards,

Anthony Liguori

>          s->nic->nc.link_down = true;
>          e1000_link_down(s);
>          s->phy_reg[PHY_STATUS] &= ~MII_SR_AUTONEG_COMPLETE;
> -- 
> 1.7.9.5