Patch for async. packet corruption in polling mode.

Hidetoshi Shimokawa simokawa at FreeBSD.ORG
Mon Jul 9 02:30:34 UTC 2007


It looks that your patch fixes the case where the number of packets processed
is limited. As far as I tested, -current does not have this problem because
it doesn't limit the number of packets processed for polling.

I assume you are using polling mode for performance, right?
I think you should gain performance if fwohci shares IRQ with
other devices. Which devices does your machine share IRQ with?

We have two choices now,
1. Merge MPSAFE firewire stack from current to RELENG_6.
We need some modification because CAM in -stable needs the Giant.

2. Fix RELENG_6 independently.
Just removing packet limitation is the easiest way.

I'm not sure about effectiveness of limiting the number of packets
in preemptive kernel. In my opinion, separating rx and tx taskq seems
a good way to go.

On 6/29/07, Hidetoshi Shimokawa <simokawa at freebsd.org> wrote:
> I remember that there is a PR related this problem.
> I'll check your patch next week.
>
> Thank you for your patch.
>
> On 6/28/07, Kobayashi Katsushi <ikob at ni.aist.go.jp> wrote:
> > Hi,
> >
> > I met async. packet corruption when using fwip device with
> > polling mode. My patch for R6.2 is attached.
> >
> > Thanks,
> >
> > --
> > Katsushi Kobayashi
> >
> > Index: sys/dev/firewire/fwohci.c
> > ===================================================================
> > RCS file: /grid/home/ikob/develop/FreeBSD/sys/dev/firewire/fwohci.c,v
> > retrieving revision 1.1.1.1
> > retrieving revision 1.1.1.1.6.3
> > diff -c -r1.1.1.1 -r1.1.1.1.6.3
> > *** sys/dev/firewire/fwohci.c   22 May 2007 06:25:52 -0000      1.1.1.1
> > --- sys/dev/firewire/fwohci.c   28 Jun 2007 09:09:11 -0000
> > 1.1.1.1.6.3
> > ***************
> > *** 125,131 ****
> >    static void fwohci_ibr (struct firewire_comm *);
> >    static void fwohci_db_init (struct fwohci_softc *, struct
> > fwohci_dbch *);
> >    static void fwohci_db_free (struct fwohci_dbch *);
> > ! static void fwohci_arcv (struct fwohci_softc *, struct fwohci_dbch
> > *, int);
> >    static void fwohci_txd (struct fwohci_softc *, struct fwohci_dbch *);
> >    static void fwohci_start_atq (struct firewire_comm *);
> >    static void fwohci_start_ats (struct firewire_comm *);
> > --- 125,131 ----
> >    static void fwohci_ibr (struct firewire_comm *);
> >    static void fwohci_db_init (struct fwohci_softc *, struct
> > fwohci_dbch *);
> >    static void fwohci_db_free (struct fwohci_dbch *);
> > ! static int fwohci_arcv (struct fwohci_softc *, struct fwohci_dbch
> > *, int);
> >    static void fwohci_txd (struct fwohci_softc *, struct fwohci_dbch *);
> >    static void fwohci_start_atq (struct firewire_comm *);
> >    static void fwohci_start_ats (struct firewire_comm *);
> > ***************
> > *** 580,585 ****
> > --- 580,586 ----
> >          }
> >
> >
> > +       sc->pollstat = 0;
> >          /* Enable interrupts */
> >          OWRITE(sc, FWOHCI_INTMASK,
> >                          OHCI_INT_ERR  | OHCI_INT_PHY_SID
> > ***************
> > *** 671,676 ****
> > --- 672,678 ----
> >
> >          sc->fc.tcode = tinfo;
> >          sc->fc.dev = dev;
> > +       sc->pollstat = 0;
> >
> >          sc->fc.config_rom = fwdma_malloc(&sc->fc, CROMSIZE, CROMSIZE,
> >                                                  &sc->crom_dma,
> > BUS_DMA_WAITOK);
> > ***************
> > *** 1867,1873 ****
> >                  dump_dma(sc, ARRS_CH);
> >                  dump_db(sc, ARRS_CH);
> >    #endif
> > !               fwohci_arcv(sc, &sc->arrs, count);
> >          }
> >          if((stat & OHCI_INT_DMA_PRRQ )){
> >    #ifndef ACK_ALL
> > --- 1869,1879 ----
> >                  dump_dma(sc, ARRS_CH);
> >                  dump_db(sc, ARRS_CH);
> >    #endif
> > !               if(fwohci_arcv(sc, &sc->arrs, count) == 0) {
> > !                       sc->pollstat &= ~OHCI_INT_DMA_PRRS;
> > !               }else{
> > !                       sc->pollstat |= OHCI_INT_DMA_PRRS;
> > !               }
> >          }
> >          if((stat & OHCI_INT_DMA_PRRQ )){
> >    #ifndef ACK_ALL
> > ***************
> > *** 1877,1883 ****
> >                  dump_dma(sc, ARRQ_CH);
> >                  dump_db(sc, ARRQ_CH);
> >    #endif
> > !               fwohci_arcv(sc, &sc->arrq, count);
> >          }
> >          if(stat & OHCI_INT_PHY_SID){
> >                  uint32_t *buf, node_id;
> > --- 1883,1893 ----
> >                  dump_dma(sc, ARRQ_CH);
> >                  dump_db(sc, ARRQ_CH);
> >    #endif
> > !               if(fwohci_arcv(sc, &sc->arrq, count) == 0){
> > !                       sc->pollstat &= ~OHCI_INT_DMA_PRRQ;
> > !               }else{
> > !                       sc->pollstat |= OHCI_INT_DMA_PRRQ;
> > !               }
> >          }
> >          if(stat & OHCI_INT_PHY_SID){
> >                  uint32_t *buf, node_id;
> > ***************
> > *** 2081,2086 ****
> > --- 2091,2097 ----
> >          if (1) {
> >    #endif
> >                  stat = fwochi_check_stat(sc);
> > +               stat |= sc->pollstat;
> >                  if (stat == 0 || stat == 0xffffffff)
> >                          return;
> >          }
> > ***************
> > *** 2601,2607 ****
> >          return 0;
> >    }
> >
> > -
> >    static int
> >    fwohci_arcv_swap(struct fw_pkt *fp, int len)
> >    {
> > --- 2612,2617 ----
> > ***************
> > *** 2686,2692 ****
> >          dbch->bottom = db_tr;
> >    }
> >
> > ! static void
> >    fwohci_arcv(struct fwohci_softc *sc, struct fwohci_dbch *dbch, int
> > count)
> >    {
> >          struct fwohcidb_tr *db_tr;
> > --- 2696,2702 ----
> >          dbch->bottom = db_tr;
> >    }
> >
> > ! static int
> >    fwohci_arcv(struct fwohci_softc *sc, struct fwohci_dbch *dbch, int
> > count)
> >    {
> >          struct fwohcidb_tr *db_tr;
> > ***************
> > *** 2701,2713 ****
> >          int s;
> >          caddr_t buf;
> >          int resCount;
> >
> >          if(&sc->arrq == dbch){
> >                  off = OHCI_ARQOFF;
> >          }else if(&sc->arrs == dbch){
> >                  off = OHCI_ARSOFF;
> >          }else{
> > !               return;
> >          }
> >
> >          s = splfw();
> > --- 2711,2724 ----
> >          int s;
> >          caddr_t buf;
> >          int resCount;
> > +       int ret = 0;
> >
> >          if(&sc->arrq == dbch){
> >                  off = OHCI_ARQOFF;
> >          }else if(&sc->arrs == dbch){
> >                  off = OHCI_ARSOFF;
> >          }else{
> > !               return 0;
> >          }
> >
> >          s = splfw();
> > ***************
> > *** 2719,2725 ****
> >          status = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) >>
> > OHCI_STATUS_SHIFT;
> >          resCount = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) &
> > OHCI_COUNT_MASK;
> >    #if 0
> > !       printf("status 0x%04x, resCount 0x%04x\n", status, resCount);
> >    #endif
> >          while (status & OHCI_CNTL_DMA_ACTIVE) {
> >                  len = dbch->xferq.psize - resCount;
> > --- 2730,2736 ----
> >          status = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) >>
> > OHCI_STATUS_SHIFT;
> >          resCount = FWOHCI_DMA_READ(db_tr->db[0].db.desc.res) &
> > OHCI_COUNT_MASK;
> >    #if 0
> > !       printf("status 0x%04x, resCount 0x%04x count %d\n", status,
> > resCount, count);
> >    #endif
> >          while (status & OHCI_CNTL_DMA_ACTIVE) {
> >                  len = dbch->xferq.psize - resCount;
> > ***************
> > *** 2733,2739 ****
> >                                          BUS_DMASYNC_POSTREAD);
> >                  while (len > 0 ) {
> >                          if (count >= 0 && count-- == 0)
> > !                               goto out;
> >                          if(dbch->pdb_tr != NULL){
> >                                  /* we have a fragment in previous
> > buffer */
> >                                  int rlen;
> > --- 2744,2753 ----
> >                                          BUS_DMASYNC_POSTREAD);
> >                  while (len > 0 ) {
> >                          if (count >= 0 && count-- == 0)
> > !                       {
> > !                               ret = 1;
> > !                               goto pollout;
> > !                       }
> >                          if(dbch->pdb_tr != NULL){
> >                                  /* we have a fragment in previous
> > buffer */
> >                                  int rlen;
> > ***************
> > *** 2898,2906 ****
> > --- 2912,2922 ----
> >                  }
> >                  /* XXX make sure DMA is not dead */
> >          }
> > + pollout:
> >    #if 0
> >          if (pcnt < 1)
> >                  printf("fwohci_arcv: no packets\n");
> >    #endif
> >          splx(s);
> > +       return ret;
> >    }
> > Index: sys/dev/firewire/fwohcivar.h
> > ===================================================================
> > RCS file: /grid/home/ikob/develop/FreeBSD/sys/dev/firewire/fwohcivar.h,v
> > retrieving revision 1.1.1.1
> > retrieving revision 1.1.1.1.6.1
> > diff -c -r1.1.1.1 -r1.1.1.1.6.1
> > *** sys/dev/firewire/fwohcivar.h        22 May 2007 06:25:52
> > -0000      1.1.1.1
> > --- sys/dev/firewire/fwohcivar.h        28 Jun 2007 04:17:22
> > -0000      1.1.1.1.6.1
> > ***************
> > *** 81,86 ****
> > --- 81,87 ----
> >          uint32_t intstat;
> >          struct task fwohci_task_complete;
> >    #endif
> > +       uint32_t pollstat;
> >    } fwohci_softc_t;
> >
> >    void fwohci_intr (void *arg);
> >
> >
> >
> >
> >
> >
> > --
> > Katsushi Kobayashi
> >
> >
> >
> > _______________________________________________
> > freebsd-firewire at freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-firewire
> > To unsubscribe, send any mail to "freebsd-firewire-unsubscribe at freebsd.org"
> >
>
>
> --
> /\ Hidetoshi Shimokawa
> \/  simokawa at FreeBSD.ORG
>


-- 
/\ Hidetoshi Shimokawa
\/  simokawa at FreeBSD.ORG


More information about the freebsd-firewire mailing list