Date: Mon, 9 Jul 2007 12:14:24 +0900 From: Kobayashi Katsushi <ikob@ni.aist.go.jp> To: Hidetoshi Shimokawa <simokawa@FreeBSD.ORG> Cc: freebsd-firewire@FreeBSD.ORG Subject: Re: Patch for async. packet corruption in polling mode. Message-ID: <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp> In-Reply-To: <626eb4530707081930x6ec4ad60q54df347be1aba727@mail.gmail.com> References: <9E3627A8-0036-408A-B604-11A514B0CFD2@ni.aist.go.jp> <626eb4530706290218n66064fferfe04a6a146fb69f9@mail.gmail.com> <626eb4530707081930x6ec4ad60q54df347be1aba727@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi, Our experience was done on -stable only. I am not sure how to work on the latest -current. I understood your suggestion that disabling limit number of packet processing works fine also in -stable. If the limit packet processing option has been removed from "device polling" in -current, my patch is an useless for at least -current. However, if the option continues to be supported in -current, the same corruption issue will be happen again. BTW, It is difficult to ask your question why turning on polling option. Simply say I would like to evaluate the effect of interrupt coalescing and/or device polling. P.S. I also understood the corruption mainly caused by a pretty strange spec. for 1394 OHCI (only support buffer fill mode in async. packet receive), and this spec. requires more little effort for device deriver implementor. -- Katsushi Kobayashi On 2007/07/09, at 11:30, Hidetoshi Shimokawa wrote: > 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@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@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@freebsd.org mailing list >> > http://lists.freebsd.org/mailman/listinfo/freebsd-firewire >> > To unsubscribe, send any mail to "freebsd-firewire- >> unsubscribe@freebsd.org" >> > >> >> >> -- >> /\ Hidetoshi Shimokawa >> \/ simokawa@FreeBSD.ORG >> > > > -- > /\ Hidetoshi Shimokawa > \/ simokawa@FreeBSD.ORG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?06F66E24-16E7-4FBD-BF60-C9F5C31064F6>