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

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

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This fixes an issue with the GEM models reported by Bilal.
If a GEM's receiver is disabled, we shouldn't be asserting on
descriptor processing.

Cheers,
Edgar

Edgar E. Iglesias (1):
  net: cadence_gem: Remove incorrect assert()

 hw/net/cadence_gem.c | 1 -
 1 file changed, 1 deletion(-)

-- 
2.17.1

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

[Edgar E. Iglesias]

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);
             return -1;
         }
 
-- 
2.17.1

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

[Philippe Mathieu-Daudé]

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.

>          }
>  
> 

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

[Edgar E. Iglesias]

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.
> 
> >          }
> >  
> > 

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

[Edgar E. Iglesias]

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.
> > 
> > >          }
> > >  
> > > 

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

[Edgar E. Iglesias]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Peter Maydell]

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

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

[Edgar E. Iglesias]

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

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

[Peter Maydell]

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