| Message ID | 1356686951-20305-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jasowang@redhat.com> > Signed-off-by: Amos Kong <akong@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. Stefan
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 <jasowang@redhat.com> >> Signed-off-by: Amos Kong <akong@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. > > Stefan
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 <jasowang@redhat.com> > >> Signed-off-by: Amos Kong <akong@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. 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
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 <jasowang@redhat.com> > > >> Signed-off-by: Amos Kong <akong@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
On Mon, Jan 07, 2013 at 01:59:54PM +0100, Stefan Hajnoczi wrote: > 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 <jasowang@redhat.com> > > > >> Signed-off-by: Amos Kong <akong@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? Hi Stefan, I don't have e1000 (82540EM) in hand, and just tested with e1000e (82567LM-3) This is the debug message: | >>> setup autoneg: icr & E1000_ICR_LSC: 0 | >>> autoneg completed, icr & E1000_ICR_LSC: 0 | >>> setup autoneg: icr & E1000_ICR_LSC: 0 | >>> autoneg completed, icr & E1000_ICR_LSC: 0 No interrupt after auto-nego completed | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X irq is enabled | >>> e1000_open: before fire an interrupt, icr & E1000_ICR_LSC: 0 ^^^ ICR_LSC bit doesn't change by hardware Software driver changes ICR_LSC bit to fire a interrupt | >>> e1000_open: after fire an interrupt, icr & E1000_ICR_LSC: 4 | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4 ^^ handle this interrupt | IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4 ^^^ E1000_ICR_LSC is changed by hardware and caused an interrupt Our e1000 backend driver doesn't raise this interrupt. It seems a interrupt should be raise by backend driver, but we don't know what's the right time/point. | e1000e: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None | IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 | >>> set link up in watchdog task, icr & E1000_ICR_LSC: 0 In OpenSDM_8254x-37.pdf: | ++ PHY Initialization (10/100/1000 Mb/s Copper Media) | Once link is achieved by the PHY, software is notified when a Link | Status Change (LSC) interrupt is generated by the Ethernet controller. "link is achieved by the PHY" == "auto-nego completes" ? | + 8.6.5.2 Internal PHY Mode | While in internal PHY mode, an internal signal provides status of the | physical link as indicated by | the PHY. Indication that the link is not up disables MAC operation. | Upon determination of a valid | link, the assertion of the internal link signal asserts the LSC | interrupt (if enabled) to indicate to the software driver to check the link status. Is it lost in our backend driver? I will try to find a e1000 real nic to re-test. Thanks, Amos
On Tue, Jan 08, 2013 at 05:45:30PM +0800, Amos Kong wrote: > On Mon, Jan 07, 2013 at 01:59:54PM +0100, Stefan Hajnoczi wrote: > > 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 <jasowang@redhat.com> > > > > >> Signed-off-by: Amos Kong <akong@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? > > Hi Stefan, > > I don't have e1000 (82540EM) in hand, and just tested with e1000e (82567LM-3) > This is the debug message: > > | >>> setup autoneg: icr & E1000_ICR_LSC: 0 > | >>> autoneg completed, icr & E1000_ICR_LSC: 0 > | >>> setup autoneg: icr & E1000_ICR_LSC: 0 > | >>> autoneg completed, icr & E1000_ICR_LSC: 0 > > No interrupt after auto-nego completed > > | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X > | e1000e 0000:00:19.0: irq 49 for MSI/MSI-X > > irq is enabled > > | >>> e1000_open: before fire an interrupt, icr & E1000_ICR_LSC: 0 > ^^^ > ICR_LSC bit doesn't change by hardware > > Software driver changes ICR_LSC bit to fire a interrupt > > | >>> e1000_open: after fire an interrupt, icr & E1000_ICR_LSC: 4 > > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4 > ^^ handle this interrupt > > | IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 4 > > ^^^ > E1000_ICR_LSC is changed by hardware and caused an interrupt > Our e1000 backend driver doesn't raise this interrupt. > It seems a interrupt should be raise by backend driver, but we don't > know what's the right time/point. > > | e1000e: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None > | IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 > | >>> e1000_intr_msi: icr & E1000_ICR_LSC: 0 > | >>> set link up in watchdog task, icr & E1000_ICR_LSC: 0 > > > > In OpenSDM_8254x-37.pdf: > > | ++ PHY Initialization (10/100/1000 Mb/s Copper Media) > | Once link is achieved by the PHY, software is notified when a Link > | Status Change (LSC) interrupt is generated by the Ethernet controller. > > "link is achieved by the PHY" == "auto-nego completes" ? "Link is achieved" is more general than just auto-negotiation, I think it also occurs when you force a specific link speed (no autonegotiation). The host still wants to know if the network cable is plugging in or not :). > | + 8.6.5.2 Internal PHY Mode > | While in internal PHY mode, an internal signal provides status of the > | physical link as indicated by > | the PHY. Indication that the link is not up disables MAC operation. > | Upon determination of a valid > | link, the assertion of the internal link signal asserts the LSC > | interrupt (if enabled) to indicate to the software driver to check the link status. > > Is it lost in our backend driver? My interpretation is that hw/e1000.c should raise the LSC interrupt whenever the link state changes or when forced to restart auto-negotiation. > I will try to find a e1000 real nic to re-test. Great, thanks! Stefan
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;