[Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Peter Crosthwaite]

Hi Richard,

This should hopefully fix your bug, while addressing the extra concern
I raised.

There was also inconsistent behaviour with corking packets through a
soft reset which I notice and fixed.

Please let me know if this works for you.

Regards,
Peter


Peter Crosthwaite (3):
  net: smc91c111: guard flush_queued_packets() on can_rx()
  net: smc91c111: gate can_receive() on rx FIFO having a slot
  net: smc91c111: flush packets on RCR register changes

 hw/net/smc91c111.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

-- 
1.9.1

[Qemu-devel] [RFT PATCH v1 1/3] net: smc91c111: guard flush_queued_packets() on can_rx()

[Peter Crosthwaite]

Check that the core can once again receive packets before asking the
net layer to do a flush. This will make it more convenient to flush
packets when adding new conditions to can_receive.

Add missing if braces while moving the can_receive() core code.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

 hw/net/smc91c111.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 74e06e6..5774eff 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -124,6 +124,24 @@ static void smc91c111_update(smc91c111_state *s)
     qemu_set_irq(s->irq, level);
 }
 
+static int smc91c111_can_receive(smc91c111_state *s)
+{
+    if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST)) {
+        return 1;
+    }
+    if (s->allocated == (1 << NUM_PACKETS) - 1) {
+        return 0;
+    }
+    return 1;
+}
+
+static inline void smc91c111_flush_queued_packets(smc91c111_state *s)
+{
+    if (smc91c111_can_receive(s)) {
+        qemu_flush_queued_packets(qemu_get_queue(s->nic));
+    }
+}
+
 /* Try to allocate a packet.  Returns 0x80 on failure.  */
 static int smc91c111_allocate_packet(smc91c111_state *s)
 {
@@ -185,7 +203,7 @@ static void smc91c111_release_packet(smc91c111_state *s, int packet)
     s->allocated &= ~(1 << packet);
     if (s->tx_alloc == 0x80)
         smc91c111_tx_alloc(s);
-    qemu_flush_queued_packets(qemu_get_queue(s->nic));
+    smc91c111_flush_queued_packets(s);
 }
 
 /* Flush the TX FIFO.  */
@@ -636,15 +654,11 @@ static uint32_t smc91c111_readl(void *opaque, hwaddr offset)
     return val;
 }
 
-static int smc91c111_can_receive(NetClientState *nc)
+static int smc91c111_can_receive_nc(NetClientState *nc)
 {
     smc91c111_state *s = qemu_get_nic_opaque(nc);
 
-    if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST))
-        return 1;
-    if (s->allocated == (1 << NUM_PACKETS) - 1)
-        return 0;
-    return 1;
+    return smc91c111_can_receive(s);
 }
 
 static ssize_t smc91c111_receive(NetClientState *nc, const uint8_t *buf, size_t size)
@@ -739,7 +753,7 @@ static const MemoryRegionOps smc91c111_mem_ops = {
 static NetClientInfo net_smc91c111_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
-    .can_receive = smc91c111_can_receive,
+    .can_receive = smc91c111_can_receive_nc,
     .receive = smc91c111_receive,
 };
 
-- 
1.9.1

Re: [Qemu-devel] [RFT PATCH v1 1/3] net: smc91c111: guard flush_queued_packets() on can_rx()

[Fam Zheng]

On Thu, 09/10 21:23, Peter Crosthwaite wrote:
> Check that the core can once again receive packets before asking the
> net layer to do a flush. This will make it more convenient to flush
> packets when adding new conditions to can_receive.
> 
> Add missing if braces while moving the can_receive() core code.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 
>  hw/net/smc91c111.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 74e06e6..5774eff 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -124,6 +124,24 @@ static void smc91c111_update(smc91c111_state *s)
>      qemu_set_irq(s->irq, level);
>  }
>  
> +static int smc91c111_can_receive(smc91c111_state *s)
> +{
> +    if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST)) {
> +        return 1;
> +    }
> +    if (s->allocated == (1 << NUM_PACKETS) - 1) {
> +        return 0;
> +    }
> +    return 1;
> +}
> +
> +static inline void smc91c111_flush_queued_packets(smc91c111_state *s)
> +{
> +    if (smc91c111_can_receive(s)) {
> +        qemu_flush_queued_packets(qemu_get_queue(s->nic));
> +    }
> +}
> +
>  /* Try to allocate a packet.  Returns 0x80 on failure.  */
>  static int smc91c111_allocate_packet(smc91c111_state *s)
>  {
> @@ -185,7 +203,7 @@ static void smc91c111_release_packet(smc91c111_state *s, int packet)
>      s->allocated &= ~(1 << packet);
>      if (s->tx_alloc == 0x80)
>          smc91c111_tx_alloc(s);
> -    qemu_flush_queued_packets(qemu_get_queue(s->nic));
> +    smc91c111_flush_queued_packets(s);
>  }
>  
>  /* Flush the TX FIFO.  */
> @@ -636,15 +654,11 @@ static uint32_t smc91c111_readl(void *opaque, hwaddr offset)
>      return val;
>  }
>  
> -static int smc91c111_can_receive(NetClientState *nc)
> +static int smc91c111_can_receive_nc(NetClientState *nc)
>  {
>      smc91c111_state *s = qemu_get_nic_opaque(nc);
>  
> -    if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST))
> -        return 1;
> -    if (s->allocated == (1 << NUM_PACKETS) - 1)
> -        return 0;
> -    return 1;
> +    return smc91c111_can_receive(s);
>  }
>  
>  static ssize_t smc91c111_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> @@ -739,7 +753,7 @@ static const MemoryRegionOps smc91c111_mem_ops = {
>  static NetClientInfo net_smc91c111_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_NIC,
>      .size = sizeof(NICState),
> -    .can_receive = smc91c111_can_receive,
> +    .can_receive = smc91c111_can_receive_nc,
>      .receive = smc91c111_receive,
>  };
>  
> -- 
> 1.9.1
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [RFT PATCH v1 2/3] net: smc91c111: gate can_receive() on rx FIFO having a slot

[Peter Crosthwaite]

Return false from can_receive() when the FIFO doesn't have a free RX
slot. This fixes a bug in the current code where the allocated buffer
is freed before the fifo pop, triggering a premature flush of queued RX
packets. It also will handle a corner case, where the guest manually
frees the allocated buffer before popping the rx FIFO (hence it is not
enough to just delay the flush_queued_packets()).

Reported-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

 hw/net/smc91c111.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 5774eff..8fc3deb 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -129,7 +129,8 @@ static int smc91c111_can_receive(smc91c111_state *s)
     if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST)) {
         return 1;
     }
-    if (s->allocated == (1 << NUM_PACKETS) - 1) {
+    if (s->allocated == (1 << NUM_PACKETS) - 1 ||
+        s->rx_fifo_len == NUM_PACKETS) {
         return 0;
     }
     return 1;
@@ -182,6 +183,7 @@ static void smc91c111_pop_rx_fifo(smc91c111_state *s)
     } else {
         s->int_level &= ~INT_RCV;
     }
+    smc91c111_flush_queued_packets(s);
     smc91c111_update(s);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [RFT PATCH v1 2/3] net: smc91c111: gate can_receive() on rx FIFO having a slot

[Fam Zheng]

On Thu, 09/10 21:23, Peter Crosthwaite wrote:
> Return false from can_receive() when the FIFO doesn't have a free RX
> slot. This fixes a bug in the current code where the allocated buffer
> is freed before the fifo pop, triggering a premature flush of queued RX
> packets. It also will handle a corner case, where the guest manually
> frees the allocated buffer before popping the rx FIFO (hence it is not
> enough to just delay the flush_queued_packets()).
> 
> Reported-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 
>  hw/net/smc91c111.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 5774eff..8fc3deb 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -129,7 +129,8 @@ static int smc91c111_can_receive(smc91c111_state *s)
>      if ((s->rcr & RCR_RXEN) == 0 || (s->rcr & RCR_SOFT_RST)) {
>          return 1;
>      }
> -    if (s->allocated == (1 << NUM_PACKETS) - 1) {
> +    if (s->allocated == (1 << NUM_PACKETS) - 1 ||
> +        s->rx_fifo_len == NUM_PACKETS) {
>          return 0;
>      }
>      return 1;
> @@ -182,6 +183,7 @@ static void smc91c111_pop_rx_fifo(smc91c111_state *s)
>      } else {
>          s->int_level &= ~INT_RCV;
>      }
> +    smc91c111_flush_queued_packets(s);
>      smc91c111_update(s);
>  }
>  
> -- 
> 1.9.1
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [RFT PATCH v1 3/3] net: smc91c111: flush packets on RCR register changes

[Peter Crosthwaite]

The SOFT_RST or RXEN in the control register can be used as a condition
to unblock the net layer via can_receive(). So check for possible
flushes on RCR changes. This will drop all pending packets on soft
reset or disable which is the functional intent of the can_receive()
logic.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

 hw/net/smc91c111.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 8fc3deb..c19cdd1 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -331,6 +331,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
             if (s->rcr & RCR_SOFT_RST) {
                 smc91c111_reset(DEVICE(s));
             }
+            smc91c111_flush_queued_packets(s);
             return;
         case 10: case 11: /* RPCR */
             /* Ignored */
-- 
1.9.1

Re: [Qemu-devel] [RFT PATCH v1 3/3] net: smc91c111: flush packets on RCR register changes

[Fam Zheng]

On Thu, 09/10 21:24, Peter Crosthwaite wrote:
> The SOFT_RST or RXEN in the control register can be used as a condition
> to unblock the net layer via can_receive(). So check for possible
> flushes on RCR changes. This will drop all pending packets on soft
> reset or disable which is the functional intent of the can_receive()
> logic.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 
>  hw/net/smc91c111.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 8fc3deb..c19cdd1 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -331,6 +331,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
>              if (s->rcr & RCR_SOFT_RST) {
>                  smc91c111_reset(DEVICE(s));
>              }
> +            smc91c111_flush_queued_packets(s);
>              return;
>          case 10: case 11: /* RPCR */
>              /* Ignored */
> -- 
> 1.9.1
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

This should be useful for other NICs too.

Fam

Re: [Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Richard Purdie]

Hi Peter,

On Thu, 2015-09-10 at 21:23 -0700, Peter Crosthwaite wrote:
> This should hopefully fix your bug, while addressing the extra concern
> I raised.
> 
> There was also inconsistent behaviour with corking packets through a
> soft reset which I notice and fixed.
> 
> Please let me know if this works for you.

I've run it through a few builds/tests in place of my own patches and so
far it seems to be working, thanks!

Cheers,

Richard

Re: [Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Peter Crosthwaite]

On Mon, Sep 14, 2015 at 2:09 PM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> Hi Peter,
>
> On Thu, 2015-09-10 at 21:23 -0700, Peter Crosthwaite wrote:
>> This should hopefully fix your bug, while addressing the extra concern
>> I raised.
>>
>> There was also inconsistent behaviour with corking packets through a
>> soft reset which I notice and fixed.
>>
>> Please let me know if this works for you.
>
> I've run it through a few builds/tests in place of my own patches and so
> far it seems to be working, thanks!
>

Can we take that as a formal Tested-by? :)

Regards,
Peter

> Cheers,
>
> Richard
>

Re: [Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Richard Purdie]

On Tue, 2015-09-15 at 11:02 -0700, Peter Crosthwaite wrote:
> On Mon, Sep 14, 2015 at 2:09 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > Hi Peter,
> >
> > On Thu, 2015-09-10 at 21:23 -0700, Peter Crosthwaite wrote:
> >> This should hopefully fix your bug, while addressing the extra concern
> >> I raised.
> >>
> >> There was also inconsistent behaviour with corking packets through a
> >> soft reset which I notice and fixed.
> >>
> >> Please let me know if this works for you.
> >
> > I've run it through a few builds/tests in place of my own patches and so
> > far it seems to be working, thanks!
> >
> 
> Can we take that as a formal Tested-by? :)

Tested-by: Richard Purdie <richard.purdie@linuxfoundation.org>

if that helps :)

Cheers,

Richard

Re: [Qemu-devel] [RFT PATCH v1 0/3] net: smc91c111 can_receive fixes

[Stefan Hajnoczi]

On Thu, Sep 10, 2015 at 09:23:27PM -0700, Peter Crosthwaite wrote:
> Hi Richard,
> 
> This should hopefully fix your bug, while addressing the extra concern
> I raised.
> 
> There was also inconsistent behaviour with corking packets through a
> soft reset which I notice and fixed.
> 
> Please let me know if this works for you.
> 
> Regards,
> Peter
> 
> 
> Peter Crosthwaite (3):
>   net: smc91c111: guard flush_queued_packets() on can_rx()
>   net: smc91c111: gate can_receive() on rx FIFO having a slot
>   net: smc91c111: flush packets on RCR register changes
> 
>  hw/net/smc91c111.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> -- 
> 1.9.1
> 
> 

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan