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 <jasow...@redhat.com>
> > >> Signed-off-by: Amos Kong <ak...@redhat.com>
> > >> ---
> > >> 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
