From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U1I3T-0006wb-Eb for qemu-devel@nongnu.org; Fri, 01 Feb 2013 09:57:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U1I3R-0000PE-JN for qemu-devel@nongnu.org; Fri, 01 Feb 2013 09:57:27 -0500 Received: from e7.ny.us.ibm.com ([32.97.182.137]:34866) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U1I3R-0000Mf-Fc for qemu-devel@nongnu.org; Fri, 01 Feb 2013 09:57:25 -0500 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 1 Feb 2013 09:57:07 -0500 From: Anthony Liguori In-Reply-To: <1359675832-11999-1-git-send-email-mdroth@linux.vnet.ibm.com> References: <1359675832-11999-1-git-send-email-mdroth@linux.vnet.ibm.com> Date: Fri, 01 Feb 2013 08:56:53 -0600 Message-ID: <871ud0uloq.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH for-1.4] Revert "e1000: no need auto-negotiation if link was down" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth , qemu-devel@nongnu.org Cc: jasowang@redhat.com, akong@redhat.com, qemu-stable@nongnu.org Michael Roth 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 > --- > 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