| Message ID | 20181123135450.24829-2-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: cadence_gem: Remove incorrect assert() | expand |
Hi Edgar, 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); > return -1; Shouldn't this be 'return 0'? The "net/net.h" doc is scarce... Regards, Phil. > } > >
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. > > > } > > > >
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.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. Actually, the GEM model does do that, my bad, so yes return 0 seems to be the right thing to do here. Thanks, Edgar > > Cheers, > Edgar > > > > > > Regards, > > > > Phil. > > > > > } > > > > > >
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.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. > > 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
On Fri, 23 Nov 2018 at 16:59, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> Not sure if it's too late to even get the removal of the assert into this release? Peter?
If you're happy that removing the assert is the correct fix,
yes, this could go in before rc3 next week.
thanks
-- PMM
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" <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. >> >> 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é <philmd@redhat.com> Regards, Phil.
On Fri, Nov 23, 2018 at 05:09:35PM +0000, Peter Maydell wrote: > On Fri, 23 Nov 2018 at 16:59, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > Not sure if it's too late to even get the removal of the assert into this release? Peter? > > If you're happy that removing the assert is the correct fix, > yes, this could go in before rc3 next week. Yes, I think removing the assert is a suitable fix for the release. Thanks! Edgar
On Mon, 26 Nov 2018 at 12:52, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Fri, Nov 23, 2018 at 05:09:35PM +0000, Peter Maydell wrote: > > On Fri, 23 Nov 2018 at 16:59, Edgar E. Iglesias > > <edgar.iglesias@gmail.com> wrote: > > > Not sure if it's too late to even get the removal of the assert into this release? Peter? > > > > If you're happy that removing the assert is the correct fix, > > yes, this could go in before rc3 next week. > > > Yes, I think removing the assert is a suitable fix for the release. OK, I have applied this to target-arm.next for rc3. thanks -- PMM
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); return -1; }