Re: Fix sxie(4) tx

2016-09-11 Thread Marcus Glocker
On Sun, Sep 11, 2016 at 01:24:23PM +0200, Mark Kettenis wrote:

> Hardware has two transmit FIFOs, and the idea is to alternate between
> the two.  The code that decides which FIFO to use is busted.  It keeps
> a count of the number of FIFOs in use.  That means that if the count
> is one, we don't actually know which if the FIFOs is currently in use.
> If we pick the wrong FIFO for the next packet, the interface hangs and
> we get a watchdog timeout.
> 
> Diff below fixes this by using a bitmask to keep track of the FIFOs
> that are in use.
> 
> ok?

I can't test it myself, but makes sense to me.  ok mglocker
 
> Index: arch/armv7/sunxi/sxie.c
> ===
> RCS file: /cvs/src/sys/arch/armv7/sunxi/sxie.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 sxie.c
> --- arch/armv7/sunxi/sxie.c   22 Aug 2016 19:38:42 -  1.21
> +++ arch/armv7/sunxi/sxie.c   11 Sep 2016 11:18:24 -
> @@ -101,6 +101,9 @@
>  #define SXIE_INTR_DISABLE0x
>  #define SXIE_INTR_CLEAR  0x
>  
> +#define SXIE_TX_FIFO00x0001
> +#define SXIE_TX_FIFO10x0002
> +
>  #define  SXIE_RX_ENABLE  0x0004
>  #define  SXIE_TX_ENABLE  0x0003
>  #define  SXIE_RXTX_ENABLE0x0007
> @@ -436,16 +439,13 @@ sxie_intr(void *arg)
>   sxie_recv(sc);
>   }
>  
> - pending &= 3;
> -
> - if (pending) {
> + if (pending & (SXIE_TX_FIFO0 | SXIE_TX_FIFO1)) {
>   ifq_clr_oactive(>if_snd);
> - sc->txf_inuse--;
> - ifp->if_opackets++;
> - if (pending == 3) { /* 2 packets got sent */
> - sc->txf_inuse--;
> + if (pending & SXIE_TX_FIFO0)
> + ifp->if_opackets++;
> + if (pending & SXIE_TX_FIFO1)
>   ifp->if_opackets++;
> - }
> + sc->txf_inuse &= ~pending;
>   if (sc->txf_inuse == 0)
>   ifp->if_timer = 0;
>   else
> @@ -473,7 +473,7 @@ sxie_start(struct ifnet *ifp)
>   uint32_t fifo;
>   uint32_t txbuf[SXIE_MAX_PKT_SIZE / sizeof(uint32_t)]; /* XXX !!! */
>  
> - if (sc->txf_inuse > 1)
> + if (sc->txf_inuse == (SXIE_TX_FIFO0 | SXIE_TX_FIFO1))
>   ifq_set_oactive(>if_snd);
>  
>   if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(>if_snd))
> @@ -494,7 +494,7 @@ trynext:
>   return;
>   }
>  
> - if (sc->txf_inuse > 1) {
> + if (sc->txf_inuse == (SXIE_TX_FIFO0 | SXIE_TX_FIFO1)) {
>   ifq_deq_rollback(>if_snd, m);
>   printf("sxie_start: tx fifos in use.\n");
>   ifq_set_oactive(>if_snd);
> @@ -502,10 +502,14 @@ trynext:
>   }
>  
>   /* select fifo */
> - fifo = sc->txf_inuse;
> + if (sc->txf_inuse & SXIE_TX_FIFO0) {
> + sc->txf_inuse |= SXIE_TX_FIFO1;
> + fifo = 1;
> + } else {
> + sc->txf_inuse |= SXIE_TX_FIFO0;
> + fifo = 0;
> + }
>   SXIWRITE4(sc, SXIE_TXINS, fifo);
> -
> - sc->txf_inuse++;
>  
>   /* set packet length */
>   SXIWRITE4(sc, SXIE_TXPKTLEN0 + (fifo * 4), m->m_pkthdr.len);



Fix sxie(4) tx

2016-09-11 Thread Mark Kettenis
Hardware has two transmit FIFOs, and the idea is to alternate between
the two.  The code that decides which FIFO to use is busted.  It keeps
a count of the number of FIFOs in use.  That means that if the count
is one, we don't actually know which if the FIFOs is currently in use.
If we pick the wrong FIFO for the next packet, the interface hangs and
we get a watchdog timeout.

Diff below fixes this by using a bitmask to keep track of the FIFOs
that are in use.

ok?


Index: arch/armv7/sunxi/sxie.c
===
RCS file: /cvs/src/sys/arch/armv7/sunxi/sxie.c,v
retrieving revision 1.21
diff -u -p -r1.21 sxie.c
--- arch/armv7/sunxi/sxie.c 22 Aug 2016 19:38:42 -  1.21
+++ arch/armv7/sunxi/sxie.c 11 Sep 2016 11:18:24 -
@@ -101,6 +101,9 @@
 #define SXIE_INTR_DISABLE  0x
 #define SXIE_INTR_CLEAR0x
 
+#define SXIE_TX_FIFO0  0x0001
+#define SXIE_TX_FIFO1  0x0002
+
 #defineSXIE_RX_ENABLE  0x0004
 #defineSXIE_TX_ENABLE  0x0003
 #defineSXIE_RXTX_ENABLE0x0007
@@ -436,16 +439,13 @@ sxie_intr(void *arg)
sxie_recv(sc);
}
 
-   pending &= 3;
-
-   if (pending) {
+   if (pending & (SXIE_TX_FIFO0 | SXIE_TX_FIFO1)) {
ifq_clr_oactive(>if_snd);
-   sc->txf_inuse--;
-   ifp->if_opackets++;
-   if (pending == 3) { /* 2 packets got sent */
-   sc->txf_inuse--;
+   if (pending & SXIE_TX_FIFO0)
+   ifp->if_opackets++;
+   if (pending & SXIE_TX_FIFO1)
ifp->if_opackets++;
-   }
+   sc->txf_inuse &= ~pending;
if (sc->txf_inuse == 0)
ifp->if_timer = 0;
else
@@ -473,7 +473,7 @@ sxie_start(struct ifnet *ifp)
uint32_t fifo;
uint32_t txbuf[SXIE_MAX_PKT_SIZE / sizeof(uint32_t)]; /* XXX !!! */
 
-   if (sc->txf_inuse > 1)
+   if (sc->txf_inuse == (SXIE_TX_FIFO0 | SXIE_TX_FIFO1))
ifq_set_oactive(>if_snd);
 
if (!(ifp->if_flags & IFF_RUNNING) || ifq_is_oactive(>if_snd))
@@ -494,7 +494,7 @@ trynext:
return;
}
 
-   if (sc->txf_inuse > 1) {
+   if (sc->txf_inuse == (SXIE_TX_FIFO0 | SXIE_TX_FIFO1)) {
ifq_deq_rollback(>if_snd, m);
printf("sxie_start: tx fifos in use.\n");
ifq_set_oactive(>if_snd);
@@ -502,10 +502,14 @@ trynext:
}
 
/* select fifo */
-   fifo = sc->txf_inuse;
+   if (sc->txf_inuse & SXIE_TX_FIFO0) {
+   sc->txf_inuse |= SXIE_TX_FIFO1;
+   fifo = 1;
+   } else {
+   sc->txf_inuse |= SXIE_TX_FIFO0;
+   fifo = 0;
+   }
SXIWRITE4(sc, SXIE_TXINS, fifo);
-
-   sc->txf_inuse++;
 
/* set packet length */
SXIWRITE4(sc, SXIE_TXPKTLEN0 + (fifo * 4), m->m_pkthdr.len);