Patch for async. packet corruption in polling mode.
Kobayashi Katsushi
ikob at ni.aist.go.jp
Mon Jul 9 03:14:25 UTC 2007
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 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