From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsCJ9-0001pw-7c for qemu-devel@nongnu.org; Mon, 07 Jan 2013 08:00:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsCJ4-0005zF-Jg for qemu-devel@nongnu.org; Mon, 07 Jan 2013 08:00:02 -0500 Received: from mail-bk0-f52.google.com ([209.85.214.52]:51067) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsCJ4-0005z9-Dq for qemu-devel@nongnu.org; Mon, 07 Jan 2013 07:59:58 -0500 Received: by mail-bk0-f52.google.com with SMTP id w5so8357716bku.39 for ; Mon, 07 Jan 2013 04:59:57 -0800 (PST) Date: Mon, 7 Jan 2013 13:59:54 +0100 From: Stefan Hajnoczi Message-ID: <20130107125954.GC17997@stefanha-thinkpad.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> <20130106051149.GA31337@t430s.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130106051149.GA31337@t430s.nay.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: Amos Kong Cc: jan.kiszka@siemens.com, Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi On Sun, Jan 06, 2013 at 01:11:49PM +0800, Amos Kong wrote: > 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. Then we have to raise the irq in a timer callback just like the existing code already does. I'm worried that a guest driver could depend on the LSC interrupt. > > 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. It's okay for the specific guest driver that you're thinking of. But emulation code should reflect how a real device behaves. That way it can work with other guest drivers too. The question is: does a real device raise LSC when setting the MII_SR_AUTONEG_COMPLETE bit in the PHY_STATUS register? I found no definite answer in the datasheet but I suspect it does. If you have a real e1000 could you test it? Stefan