diff mbox

[2/2] hub: change hub can_receive() strategy

Message ID 1345794984-10337-2-git-send-email-rongqing.li@windriver.com
State New
Headers show

Commit Message

li qongqing Aug. 24, 2012, 7:56 a.m. UTC 
From: Roy.Li <rongqing.li@windriver.com>

Only one hub port's peer can_receive() returns 1, the source
hub port .can_receive should return 1, to fix the below bug:

    The up state NIC can not receive any packets if guest has
    more than two NICs and only one NIC is in down state.
    http://lists.nongnu.org/archive/html/qemu-discuss/2012-08/msg00036.html

This bug is introduced by 52a3cb8(add the support for hub
own flow control) and 60c07d93 (fix qemu_can_send_packet logic),
they are tried to fix that usb NIC lost packets by blocking hub
receive until all port attached this hub can receive since usb
NIC only can accept one packet at one time, their logic is wrong,
we should fix it by creating a queue for usb NIC.

Signed-off-by: Roy.Li <rongqing.li@windriver.com>
---
 net/hub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini Aug. 24, 2012, 8:20 a.m. UTC | #1 
Il 24/08/2012 09:56, rongqing.li@windriver.com ha scritto:
> Only one hub port's peer can_receive() returns 1, the source
> hub port .can_receive should return 1, to fix the below bug:
> 
>     The up state NIC can not receive any packets if guest has
>     more than two NICs and only one NIC is in down state.
>     http://lists.nongnu.org/archive/html/qemu-discuss/2012-08/msg00036.html
> 
> This bug is introduced by 52a3cb8(add the support for hub
> own flow control) and 60c07d93 (fix qemu_can_send_packet logic),
> they are tried to fix that usb NIC lost packets by blocking hub
> receive until all port attached this hub can receive since usb
> NIC only can accept one packet at one time, their logic is wrong,
> we should fix it by creating a queue for usb NIC.

A link-down NIC should always return 1 from can_receive (and will drop
the packet).  Is that the real bug here?

Paolo
li qongqing Aug. 24, 2012, 8:33 a.m. UTC | #2 
On 2012年08月24日 16:20, Paolo Bonzini wrote:
> A link-down NIC should always return 1 from can_receive (and will drop
> the packet).  Is that the real bug here?
>

A link-down NIC always return 0 from can_receive.

Yes, it is a bug.

-Roy


> Paolo
>
>
Stefan Hajnoczi Aug. 24, 2012, 10:11 a.m. UTC | #3 
On Fri, Aug 24, 2012 at 9:33 AM, Rongqing Li <rongqing.li@windriver.com> wrote:
>
>
> On 2012年08月24日 16:20, Paolo Bonzini wrote:
>>
>> A link-down NIC should always return 1 from can_receive (and will drop
>> the packet).  Is that the real bug here?
>>
>
> A link-down NIC always return 0 from can_receive.
>
> Yes, it is a bug.

The code says something different.  link_down isn't taken into account
by netc.:qemu_can_send_packet():

int qemu_can_send_packet(NetClientState *sender)
{
    if (!sender->peer) {
        return 1;
    }

    if (sender->peer->receive_disabled) {
        return 0;
    } else if (sender->peer->info->can_receive &&
               !sender->peer->info->can_receive(sender->peer)) {
        return 0;
    }
    return 1;
}

If the net client has no ->can_receive then we return 1.  Otherwise we
return ->can_receive().

Were you thinking of a specific NIC where .can_receive() returns 0
when link_down=1?

Stefan
diff mbox

Patch

diff --git a/net/hub.c b/net/hub.c
index ac157e3..650a8b4 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -97,12 +97,12 @@  static int net_hub_port_can_receive(NetClientState *nc)
             continue;
         }
 
-        if (!qemu_can_send_packet(&port->nc)) {
-            return 0;
+        if (qemu_can_send_packet(&port->nc)) {
+            return 1;
         }
     }
 
-    return 1;
+    return 0;
 }
 
 static ssize_t net_hub_port_receive(NetClientState *nc,