Re: [Qemu-devel] [PATCH v1 1/1] net: cadence_gem: Remove incorrect assert()

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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" <edgar.iglesias@xilinx.com>
> > 
> > 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 <muhammad_bilal@mentor.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  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.

Cheers,
Edgar


> 
> Regards,
> 
> Phil.
> 
> >          }
> >  
> >