From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXCfZ-0004dQ-84 for qemu-devel@nongnu.org; Thu, 16 Jun 2011 09:31:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QXCfQ-00051k-8J for qemu-devel@nongnu.org; Thu, 16 Jun 2011 09:31:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55962) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXCfP-00051L-Ge for qemu-devel@nongnu.org; Thu, 16 Jun 2011 09:31:28 -0400 Date: Thu, 16 Jun 2011 15:46:51 +0300 From: "Michael S. Tsirkin" Message-ID: <20110616124651.GD14583@redhat.com> References: <4DF9BD9C.1060307@cn.fujitsu.com> <4DF9DD77.5040401@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4DF9DD77.5040401@redhat.com> Subject: Re: [Qemu-devel] [PATCH] fix the return value of rtl8139_can_receive() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Aurelien Jarno , qemu-devel On Thu, Jun 16, 2011 at 12:39:51PM +0200, Kevin Wolf wrote: > Am 16.06.2011 10:23, schrieb Wen Congyang: > > If rtl8139_can_receive() returns 1, it means that the nic can receive packet, > > otherwise, it means the nic can not receive packet. > > > > If !s->clock_enabled or !rtl8139_receiver_enabled(s), it means that the nic > > can not receive packet. So the return value should be 0, not 1. > > > > Signed-off-by: Wen Congyang > > > > --- > > hw/rtl8139.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/rtl8139.c b/hw/rtl8139.c > > index 2f8db58..9084678 100644 > > --- a/hw/rtl8139.c > > +++ b/hw/rtl8139.c > > @@ -810,9 +810,9 @@ static int rtl8139_can_receive(VLANClientState *nc) > > > > /* Receive (drop) packets if card is disabled. */ > > if (!s->clock_enabled) > > - return 1; > > + return 0; > > if (!rtl8139_receiver_enabled(s)) > > - return 1; > > + return 0; > > > > if (rtl8139_cp_receiver_enabled(s)) { > > /* ??? Flow control not implemented in c+ mode. > > NACK. > > The old behaviour is clearly intentional. IIRC, can_receive() returning > 0 means that the packet is kept in a queue and qemu tries to deliver it > later. For a disabled receiver, what I would expect is that it should > just drop the packets. This is what this code does by returning 1 in > can_receive() and then return -1 without processing the packet in receive(). > > That said, e1000 has a check for (s->mac_reg[RCTL] & E1000_RCTL_EN) in > can_receive. Should it be changed or is there a reason behind it? If > there is, we may as well change rtl8139, but it definitely needs a > better justification. > > Kevin I doubt it matters much. In practice returning 1 has the effect that qemu will keep getting packets from host and wasting CPU on dropping packets. This seems worse than packets that should be dropped but aren't. You do want to fix the comment above if you tweak this though.` -- MST