From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43583) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TriWV-0008C1-Nu for qemu-devel@nongnu.org; Sun, 06 Jan 2013 00:11:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TriWU-0007f3-FW for qemu-devel@nongnu.org; Sun, 06 Jan 2013 00:11:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:18508) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TriWU-0007eu-7S for qemu-devel@nongnu.org; Sun, 06 Jan 2013 00:11:50 -0500 Date: Sun, 6 Jan 2013 13:11:49 +0800 From: Amos Kong Message-ID: <20130106051149.GA31337@t430s.nay.redhat.com> References: <1356686951-20305-1-git-send-email-akong@redhat.com> <1356686951-20305-2-git-send-email-akong@redhat.com> <20130103122033.GC6976@stefanha-thinkpad.muc.redhat.com> <50E7E81A.8050203@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50E7E81A.8050203@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] e1000: no need auto-negotiation if link was down List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org, Stefan Hajnoczi On Sat, Jan 05, 2013 at 04:45:14PM +0800, Jason Wang wrote: > On 01/03/2013 08:20 PM, Stefan Hajnoczi wrote: > > On Fri, Dec 28, 2012 at 05:29:10PM +0800, Amos Kong wrote: > >> Commit b9d03e352cb6b31a66545763f6a1e20c9abf0c2c added link > >> auto-negotiation emulation, it would always set link up by > >> callback function. Problem exists if original link status > >> was down, link status should not be changed in auto-negotiation. > >> > >> Signed-off-by: Jason Wang > >> Signed-off-by: Amos Kong > >> --- > >> hw/e1000.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/hw/e1000.c b/hw/e1000.c > >> index 92fb00a..eebcd1d 100644 > >> --- a/hw/e1000.c > >> +++ b/hw/e1000.c > >> @@ -164,6 +164,11 @@ 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; > > Do we need set_ics(s, 0, E1000_ICR_LSC) when autonegotiation completes? > > The code doesn't but I wonder if we should. > > Not in this case I think. The hack of the auto-negotiation was used to > prevent the irq to be injected before the handler is registered in > windows guest. So an irq would be raised here if we do this which breaks > the hack. In e1000_open(), after enable irq of adapter, driver will fire a link status change interrupt to start a watchdog, which will update the link status in system. After auto-nego complete, the irq of adapter is still not enabled, the early interrupt will not work. So current code is ok. Thanks, Amos > > Stefan