From owner-freebsd-firewire@FreeBSD.ORG Mon Jul 9 02:30:34 2007 Return-Path: X-Original-To: freebsd-firewire@freebsd.org Delivered-To: freebsd-firewire@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 5D3B016A421 for ; Mon, 9 Jul 2007 02:30:34 +0000 (UTC) (envelope-from freebsd@gm.nunu.org) Received: from py-out-1112.google.com (py-out-1112.google.com [64.233.166.176]) by mx1.freebsd.org (Postfix) with ESMTP id 118B113C44B for ; Mon, 9 Jul 2007 02:30:33 +0000 (UTC) (envelope-from freebsd@gm.nunu.org) Received: by py-out-1112.google.com with SMTP id a73so1529875pye for ; Sun, 08 Jul 2007 19:30:33 -0700 (PDT) Received: by 10.35.98.3 with SMTP id a3mr5815651pym.1183948232846; Sun, 08 Jul 2007 19:30:32 -0700 (PDT) Received: by 10.35.71.5 with HTTP; Sun, 8 Jul 2007 19:30:32 -0700 (PDT) Message-ID: <626eb4530707081930x6ec4ad60q54df347be1aba727@mail.gmail.com> Date: Mon, 9 Jul 2007 11:30:32 +0900 From: "Hidetoshi Shimokawa" Sender: freebsd@gm.nunu.org To: "Kobayashi Katsushi" In-Reply-To: <626eb4530706290218n66064fferfe04a6a146fb69f9@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <9E3627A8-0036-408A-B604-11A514B0CFD2@ni.aist.go.jp> <626eb4530706290218n66064fferfe04a6a146fb69f9@mail.gmail.com> X-Google-Sender-Auth: 2278a808e262ee2d Cc: freebsd-firewire@freebsd.org Subject: Re: Patch for async. packet corruption in polling mode. X-BeenThere: freebsd-firewire@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Firewire support in FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2007 02:30:34 -0000 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 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 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 From owner-freebsd-firewire@FreeBSD.ORG Mon Jul 9 03:14:25 2007 Return-Path: X-Original-To: freebsd-firewire@FreeBSD.ORG Delivered-To: freebsd-firewire@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id A9BDD16A4D2 for ; Mon, 9 Jul 2007 03:14:25 +0000 (UTC) (envelope-from ikob@ni.aist.go.jp) Received: from mail.asahi-net.or.jp (mail2.asahi-net.or.jp [202.224.39.198]) by mx1.freebsd.org (Postfix) with ESMTP id 6112A13C489 for ; Mon, 9 Jul 2007 03:14:25 +0000 (UTC) (envelope-from ikob@ni.aist.go.jp) Received: from [150.82.175.93] (unknown [150.82.175.93]) by mail.asahi-net.or.jp (Postfix) with ESMTP id DE7DA34DED; Mon, 9 Jul 2007 12:14:23 +0900 (JST) 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> Mime-Version: 1.0 (Apple Message framework v752.3) Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed Message-Id: <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp> Content-Transfer-Encoding: 7bit From: Kobayashi Katsushi Date: Mon, 9 Jul 2007 12:14:24 +0900 To: Hidetoshi Shimokawa X-Mailer: Apple Mail (2.752.3) Cc: freebsd-firewire@FreeBSD.ORG Subject: Re: Patch for async. packet corruption in polling mode. X-BeenThere: freebsd-firewire@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Firewire support in FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2007 03:14:25 -0000 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 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 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 From owner-freebsd-firewire@FreeBSD.ORG Mon Jul 9 04:02:20 2007 Return-Path: X-Original-To: freebsd-firewire@freebsd.org Delivered-To: freebsd-firewire@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 2E1AF16A41F for ; Mon, 9 Jul 2007 04:02:20 +0000 (UTC) (envelope-from freebsd@gm.nunu.org) Received: from py-out-1112.google.com (py-out-1112.google.com [64.233.166.177]) by mx1.freebsd.org (Postfix) with ESMTP id D3D9B13C447 for ; Mon, 9 Jul 2007 04:02:19 +0000 (UTC) (envelope-from freebsd@gm.nunu.org) Received: by py-out-1112.google.com with SMTP id a73so1564468pye for ; Sun, 08 Jul 2007 21:02:19 -0700 (PDT) Received: by 10.35.62.19 with SMTP id p19mr5934927pyk.1183953739025; Sun, 08 Jul 2007 21:02:19 -0700 (PDT) Received: by 10.35.71.5 with HTTP; Sun, 8 Jul 2007 21:02:18 -0700 (PDT) Message-ID: <626eb4530707082102g3363c14br21555d8f3b1c8385@mail.gmail.com> Date: Mon, 9 Jul 2007 13:02:18 +0900 From: "Hidetoshi Shimokawa" Sender: freebsd@gm.nunu.org To: "Kobayashi Katsushi" In-Reply-To: <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <9E3627A8-0036-408A-B604-11A514B0CFD2@ni.aist.go.jp> <626eb4530706290218n66064fferfe04a6a146fb69f9@mail.gmail.com> <626eb4530707081930x6ec4ad60q54df347be1aba727@mail.gmail.com> <06F66E24-16E7-4FBD-BF60-C9F5C31064F6@ni.aist.go.jp> X-Google-Sender-Auth: 848109d58d64ec80 Cc: freebsd-firewire@freebsd.org Subject: Re: Patch for async. packet corruption in polling mode. X-BeenThere: freebsd-firewire@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Firewire support in FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2007 04:02:20 -0000 I need some more experiments to see whether limiting packets is useful or not. Actually, we need some improvements on TX scheduling to saturate S400 for my laptop. If you have any insight abount the effect of interrupt coalescing and device polling, please let me know. I'll fix the problem on RELENG_6 before 6.3 anyway. Thanks for your input. On 7/9/07, Kobayashi Katsushi wrote: > 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 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 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 > > _______________________________________________ > 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 From owner-freebsd-firewire@FreeBSD.ORG Mon Jul 9 11:08:28 2007 Return-Path: X-Original-To: freebsd-firewire@FreeBSD.org Delivered-To: freebsd-firewire@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 0BA1016A474 for ; Mon, 9 Jul 2007 11:08:28 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [69.147.83.40]) by mx1.freebsd.org (Postfix) with ESMTP id D422B13C457 for ; Mon, 9 Jul 2007 11:08:27 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id l69B8RUp044767 for ; Mon, 9 Jul 2007 11:08:27 GMT (envelope-from owner-bugmaster@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id l69B8QXs044763 for freebsd-firewire@FreeBSD.org; Mon, 9 Jul 2007 11:08:26 GMT (envelope-from owner-bugmaster@FreeBSD.org) Date: Mon, 9 Jul 2007 11:08:26 GMT Message-Id: <200707091108.l69B8QXs044763@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: gnats set sender to owner-bugmaster@FreeBSD.org using -f From: FreeBSD bugmaster To: freebsd-firewire@FreeBSD.org Cc: Subject: Current problem reports assigned to you X-BeenThere: freebsd-firewire@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Firewire support in FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2007 11:08:28 -0000 Current FreeBSD problem reports Critical problems Serious problems S Tracker Resp. Description -------------------------------------------------------------------------------- o kern/74238 firewire [firewire] fw_rcv: unknown response; firewire ad-hoc w o kern/85434 firewire [fwip] fwip (IP over firewire) doesn't work with polli 2 problems total. Non-critical problems