[3/3] dp8393x: fix receiving buffer exhaustion
diff mbox series

Message ID 20191102171511.31881-4-laurent@vivier.eu
State New
Headers show
Series
  • dp8393x: fix problems detected with Quadra 800 machine
Related show

Commit Message

Laurent Vivier Nov. 2, 2019, 5:15 p.m. UTC
The card is not able to exit from exhaustion state, because
while the drive consumes the buffers, the RRP is incremented
(when the driver clears the ISR RBE bit), so it stays equal
to RWP, and while RRP == RWP, the card thinks it is always
in exhaustion state. So the driver consumes all the buffers,
but the card cannot receive new ones.

This patch fixes the problem by not incrementing RRP when
the driver clears the ISR RBE bit.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/net/dp8393x.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

Comments

Laurent Vivier Nov. 4, 2019, 10:14 a.m. UTC | #1
Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> The card is not able to exit from exhaustion state, because
> while the drive consumes the buffers, the RRP is incremented
> (when the driver clears the ISR RBE bit), so it stays equal
> to RWP, and while RRP == RWP, the card thinks it is always
> in exhaustion state. So the driver consumes all the buffers,
> but the card cannot receive new ones.
> 
> This patch fixes the problem by not incrementing RRP when
> the driver clears the ISR RBE bit.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)

For reviewers, this patch main changes (without indentation changes) can
be simplified to:

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b8c4473f99..123d110f16 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -304,7 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     dp8393x_update_irq(s);
 }

-static void dp8393x_do_read_rra(dp8393xState *s)
+static void dp8393x_do_read_rra(dp8393xState *s, int next)
 {
     int width, size;

@@ -323,6 +323,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
         s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);

+    if (next) {
         /* Go to next entry */
         s->regs[SONIC_RRP] += size;

@@ -337,6 +338,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
             s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
             dp8393x_update_irq(s);
         }
+    }

     /* Done */
     s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
@@ -549,7 +551,7 @@ static void dp8393x_do_command(dp8393xState *s,
uint16_t command)
     if (command & SONIC_CR_RST)
         dp8393x_do_software_reset(s);
     if (command & SONIC_CR_RRRA)
-        dp8393x_do_read_rra(s);
+        dp8393x_do_read_rra(s, 1);
     if (command & SONIC_CR_LCAM)
         dp8393x_do_load_cam(s);
 }
@@ -640,7 +642,7 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
             data &= s->regs[reg];
             s->regs[reg] &= ~data;
             if (data & SONIC_ISR_RBE) {
-                dp8393x_do_read_rra(s);
+                dp8393x_do_read_rra(s, 0);
             }
             dp8393x_update_irq(s);
             if (dp8393x_can_receive(s->nic->ncs)) {
@@ -840,7 +842,7 @@ static ssize_t dp8393x_receive(NetClientState *nc,
const uint8_t * buf,

         if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
             /* Read next RRA */
-            dp8393x_do_read_rra(s);
+            dp8393x_do_read_rra(s, 1);
         }
     }
Hervé Poussineau Nov. 5, 2019, 8:45 p.m. UTC | #2
Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
> The card is not able to exit from exhaustion state, because
> while the drive consumes the buffers, the RRP is incremented
> (when the driver clears the ISR RBE bit), so it stays equal
> to RWP, and while RRP == RWP, the card thinks it is always
> in exhaustion state. So the driver consumes all the buffers,
> but the card cannot receive new ones.
> 
> This patch fixes the problem by not incrementing RRP when
> the driver clears the ISR RBE bit.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)

I checked the DP83932C specification, available at
https://www.eit.lth.se/fileadmin/eit/courses/datablad/Periphery/Communication/DP83932C.pdf

In the Buffer Resources Exhausted section (page 20):
"To continue reception after the last RBA is used, the system must supply
additional RRA descriptor(s), update the RWP register, and clear the RBE
bit in the ISR. The SONIC rereads the RRA after this bit is cleared."

If I understand correctly, if the OS updates first the RWP and then clear the RBE bit,
then RRP should be different of RWP in dp8393x_do_read_rra() ? Or did I miss something?

> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index b8c4473f99..21deb32456 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -304,7 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>       dp8393x_update_irq(s);
>   }
>   
> -static void dp8393x_do_read_rra(dp8393xState *s)
> +static void dp8393x_do_read_rra(dp8393xState *s, int next)
>   {
>       int width, size;
>   
> @@ -323,19 +323,20 @@ static void dp8393x_do_read_rra(dp8393xState *s)
>           s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
>           s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
>   
> -    /* Go to next entry */
> -    s->regs[SONIC_RRP] += size;
> +    if (next) {
> +        /* Go to next entry */
> +        s->regs[SONIC_RRP] += size;
>   
> -    /* Handle wrap */
> -    if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
> -        s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
> -    }
> +        /* Handle wrap */
> +        if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
> +            s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
> +        }
>   
> -    /* Check resource exhaustion */
> -    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
> -    {
> -        s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
> -        dp8393x_update_irq(s);
> +        /* Check resource exhaustion */
> +        if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
> +            s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
> +            dp8393x_update_irq(s);
> +        }
>       }
>   
>       /* Done */
> @@ -549,7 +550,7 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t command)
>       if (command & SONIC_CR_RST)
>           dp8393x_do_software_reset(s);
>       if (command & SONIC_CR_RRRA)
> -        dp8393x_do_read_rra(s);
> +        dp8393x_do_read_rra(s, 1);
>       if (command & SONIC_CR_LCAM)
>           dp8393x_do_load_cam(s);
>   }
> @@ -640,7 +641,7 @@ static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
>               data &= s->regs[reg];
>               s->regs[reg] &= ~data;
>               if (data & SONIC_ISR_RBE) {
> -                dp8393x_do_read_rra(s);
> +                dp8393x_do_read_rra(s, 0);
>               }
>               dp8393x_update_irq(s);
>               if (dp8393x_can_receive(s->nic->ncs)) {
> @@ -840,7 +841,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
>   
>           if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
>               /* Read next RRA */
> -            dp8393x_do_read_rra(s);
> +            dp8393x_do_read_rra(s, 1);
>           }
>       }
>   
>
Laurent Vivier Nov. 5, 2019, 8:51 p.m. UTC | #3
Le 05/11/2019 à 21:45, Hervé Poussineau a écrit :
> Le 02/11/2019 à 18:15, Laurent Vivier a écrit :
>> The card is not able to exit from exhaustion state, because
>> while the drive consumes the buffers, the RRP is incremented
>> (when the driver clears the ISR RBE bit), so it stays equal
>> to RWP, and while RRP == RWP, the card thinks it is always
>> in exhaustion state. So the driver consumes all the buffers,
>> but the card cannot receive new ones.
>>
>> This patch fixes the problem by not incrementing RRP when
>> the driver clears the ISR RBE bit.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/net/dp8393x.c | 31 ++++++++++++++++---------------
>>   1 file changed, 16 insertions(+), 15 deletions(-)
> 
> I checked the DP83932C specification, available at
> https://www.eit.lth.se/fileadmin/eit/courses/datablad/Periphery/Communication/DP83932C.pdf
> 
> 
> In the Buffer Resources Exhausted section (page 20):
> "To continue reception after the last RBA is used, the system must supply
> additional RRA descriptor(s), update the RWP register, and clear the RBE
> bit in the ISR. The SONIC rereads the RRA after this bit is cleared."
> 
> If I understand correctly, if the OS updates first the RWP and then
> clear the RBE bit,
> then RRP should be different of RWP in dp8393x_do_read_rra() ? Or did I
> miss something?

No, I found that by debugging the problem, I didn't have the
documentation. I'll rework this patch, relying on the doc to better
understand the problem.

Thanks,
Laurent

Patch
diff mbox series

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b8c4473f99..21deb32456 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -304,7 +304,7 @@  static void dp8393x_do_load_cam(dp8393xState *s)
     dp8393x_update_irq(s);
 }
 
-static void dp8393x_do_read_rra(dp8393xState *s)
+static void dp8393x_do_read_rra(dp8393xState *s, int next)
 {
     int width, size;
 
@@ -323,19 +323,20 @@  static void dp8393x_do_read_rra(dp8393xState *s)
         s->regs[SONIC_CRBA0], s->regs[SONIC_CRBA1],
         s->regs[SONIC_RBWC0], s->regs[SONIC_RBWC1]);
 
-    /* Go to next entry */
-    s->regs[SONIC_RRP] += size;
+    if (next) {
+        /* Go to next entry */
+        s->regs[SONIC_RRP] += size;
 
-    /* Handle wrap */
-    if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
-        s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
-    }
+        /* Handle wrap */
+        if (s->regs[SONIC_RRP] == s->regs[SONIC_REA]) {
+            s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
+        }
 
-    /* Check resource exhaustion */
-    if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
-    {
-        s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
-        dp8393x_update_irq(s);
+        /* Check resource exhaustion */
+        if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
+            s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
+            dp8393x_update_irq(s);
+        }
     }
 
     /* Done */
@@ -549,7 +550,7 @@  static void dp8393x_do_command(dp8393xState *s, uint16_t command)
     if (command & SONIC_CR_RST)
         dp8393x_do_software_reset(s);
     if (command & SONIC_CR_RRRA)
-        dp8393x_do_read_rra(s);
+        dp8393x_do_read_rra(s, 1);
     if (command & SONIC_CR_LCAM)
         dp8393x_do_load_cam(s);
 }
@@ -640,7 +641,7 @@  static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
             data &= s->regs[reg];
             s->regs[reg] &= ~data;
             if (data & SONIC_ISR_RBE) {
-                dp8393x_do_read_rra(s);
+                dp8393x_do_read_rra(s, 0);
             }
             dp8393x_update_irq(s);
             if (dp8393x_can_receive(s->nic->ncs)) {
@@ -840,7 +841,7 @@  static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
 
         if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
             /* Read next RRA */
-            dp8393x_do_read_rra(s);
+            dp8393x_do_read_rra(s, 1);
         }
     }