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" <edgar.igles...@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_bi...@mentor.com>
> > > > Signed-off-by: Edgar E. Iglesias <edgar.igles...@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.
>
> 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.
Cheers,
Edgar
