From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXOEI-0000lQ-8s for qemu-devel@nongnu.org; Thu, 16 Jun 2011 21:52:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QXOEG-0005P5-7y for qemu-devel@nongnu.org; Thu, 16 Jun 2011 21:52:14 -0400 Received: from [222.73.24.84] (port=49947 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXOEF-0005Nv-CR for qemu-devel@nongnu.org; Thu, 16 Jun 2011 21:52:12 -0400 Message-ID: <4DFAB2B7.8090202@cn.fujitsu.com> Date: Fri, 17 Jun 2011 09:49:43 +0800 From: Wen Congyang MIME-Version: 1.0 References: <4DF9BD9C.1060307@cn.fujitsu.com> <4DF9DD77.5040401@redhat.com> In-Reply-To: <4DF9DD77.5040401@redhat.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1 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 , Aurelien Jarno , qemu-devel , "Michael S. Tsirkin" , Anthony Liguori At 06/16/2011 06:39 PM, Kevin Wolf Write: > 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(). Thanks for your detailed explanation. I know why can_receive() returns 1 now. > > 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 This check is introduced in commit 4105de67 by Anthony Liguori. He may know the reason. > there is, we may as well change rtl8139, but it definitely needs a > better justification. > > Kevin >