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

[Amos Kong <akong@redhat.com>]

On Thu, Jan 31, 2013 at 05:43:51PM -0600, Michael Roth wrote:
> 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.

Hi Michael,

What's your test scenario?

I tried this test with current qemu code, link status is not reseted
to 'up' after step 3. Is it the problem you said?
This problem also exists with current virtio (existed in the past) /
rtl8139 (introduced in 83f58e570f21c3e7227e7fbef1fc0e18b5ed7ea9)

1) boot a guest with e1000 nic
2) set link down in monitor
   (hmp) set_link e1000.0 down
3) reset guest by 'system_reset' in monitor
   (hmp) system_reset


My original patch is used to restore the link status after guest
reboot(execute 'reboot' insider guest system).
The link status should always be up after virtual 'hardware' reset
(execute 'system_reset' in monitor).

Thanks, Amos
 
> >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 <mdroth@linux.vnet.ibm.com>
> ---
>  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;
> -        }
>          s->nic->nc.link_down = true;
>          e1000_link_down(s);
>          s->phy_reg[PHY_STATUS] &= ~MII_SR_AUTONEG_COMPLETE;
> -- 
> 1.7.9.5