[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert(
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-arm] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert() |
Date: |
Fri, 23 Nov 2018 19:22:12 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
On 23/11/18 18:06, Edgar E. Iglesias wrote:
> On Fri, Nov 23, 2018 at 06:02:25PM +0100, Edgar E. Iglesias wrote:
>> On Fri, Nov 23, 2018 at 05:59:45PM +0100, Edgar E. Iglesias wrote:
>>> On Fri, Nov 23, 2018 at 05:46:17PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Hi Edgar,
>>>
>>> Hi Philippe,
>>>
>>>>
>>>> On 23/11/18 14:54, Edgar E. Iglesias wrote:
>>>>> From: "Edgar E. Iglesias" <address@hidden>
>>>>>
>>>>> Don't assert on RX descriptor settings when the receiver is
>>>>> disabled. This fixes an issue with incoming packets on an
>>>>> unused GEM.
>>>>>
>>>>> Reported-by: mbilal <address@hidden>
>>>>> Signed-off-by: Edgar E. Iglesias <address@hidden>
>>>>> ---
>>>>> hw/net/cadence_gem.c | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>>>> index d95cc27f58..7f63411430 100644
>>>>> --- a/hw/net/cadence_gem.c
>>>>> +++ b/hw/net/cadence_gem.c
>>>>> @@ -979,7 +979,6 @@ static ssize_t gem_receive(NetClientState *nc, const
>>>>> uint8_t *buf, size_t size)
>>>>>
>>>>> /* Do nothing if receive is not enabled. */
>>>>> if (!gem_can_receive(nc)) {
>>>>> - assert(!first_desc);
>>>>
>>>> Maybe worth:
>>>>
>>>> trace_gem_receive_packet_drop(size);
>>>
>>> Or perhaps a generic tracepoint on packet drops for any device.
>>> Anyway this is probably something for after the release.
>>>
>>> Not sure if it's too late to even get the removal of the assert into this
>>> release? Peter?
>>>
>>>>
>>>>> return -1;
>>>>
>>>> Shouldn't this be 'return 0'?
>>>>
>>>> The "net/net.h" doc is scarce...
>>>
>>> If we return 0 my understanding is that we later need to actively
>>> call qemu_flush_or_purge_queued_packets() to renable the rx
>>> path which the GEM model doesn't do. So that would mean
>>> refactoring the model a bit.
>>
>> Actually, the GEM model does do that, my bad, so yes return 0 seems to be
>> the right thing to do here.
>
> I take that back, the GEM model only handles some of the !can_receive cases
> with qemu_flush_queued_packets(). Not all, so return -1 is correct I think.
OK, thanks for checking this.
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Regards,
Phil.