Re: msk(4) require to sync status buffer

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

kiyohara%kk.iij4u.or.jp@localhost wrote:

> > Index: if_msk.c
> > ===================================================================
> > RCS file: /cvsroot/src/sys/dev/pci/if_msk.c,v
> > retrieving revision 1.16
> > diff -u -r1.16 if_msk.c
> > --- if_msk.c        7 Feb 2008 01:21:56 -0000       1.16
> > +++ if_msk.c        22 Feb 2008 16:30:40 -0000
> > @@ -1745,6 +1745,8 @@
> >                 BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> >  
> >             cur_tx = &sc_if->sk_rdata->sk_tx_ring[cons];
> > +           MSK_CDTXSYNC(sc_if, cons, 1,
> > +               BUS_DMASYNC_PREREAD);
> >             sk_ctl = cur_tx->sk_ctl;
> >  #ifdef MSK_DEBUG
> >             if (mskdebug >= 2)
> 
> Why need it?
> msk seems not to update the value of this buffer to me.  For instance,
> SK_Y2_TXCTL_LASTFRAG is written with msk_encap() by the driver.

Hmm, I thought txeof ops checked SK_Y2_TXOPC_OWN, but appearently
it didn't. I'm not sure if it's okay, but anyway if CPU's cacheline
size is lager than struct msk_tx_desc, more than one descriptors
are fetched and stored into the cache. In that case the next
msk_tx_desc might have lingered data.

> > @@ -1844,6 +1846,7 @@
> >     MSK_CDSTSYNC(sc, sc->sk_status_idx,
> >         BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> >     cur_st = &sc->sk_status_ring[sc->sk_status_idx];
> > +   MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_PREREAD);
> >  
> >     while (cur_st->sk_opcode & SK_Y2_STOPC_OWN) {
> >             cur_st->sk_opcode &= ~SK_Y2_STOPC_OWN;
> > @@ -1877,6 +1880,8 @@
> >             MSK_CDSTSYNC(sc, sc->sk_status_idx,
> >                 BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> >             cur_st = &sc->sk_status_ring[sc->sk_status_idx];
> > +           MSK_CDSTSYNC(sc, sc->sk_status_idx,
> > +               BUS_DMASYNC_PREREAD);
> >     }
> >  
> >     if (status & SK_Y2_IMR_BMU) {
> 
> We think that we should do pre-read-sync before reading DMA.  It is not
> immediately before the driver's reading.

Why do you think it should be done "immediately before?"

PREREAD should be done before DMA is started, and
PREREAD/PREWRITE doesn't imply cache writeback or
invalidate even though most implementation does it.
(consider bounce buffer xfer case)

> @@ -893,6 +893,8 @@
>       /* Reset status ring. */
>       bzero((char *)sc->sk_status_ring,
>           MSK_STATUS_RING_CNT * sizeof(struct msk_status_desc));
> +     for (i = 0; i < MSK_STATUS_RING_CNT; i++)
> +             MSK_CDSTSYNC(sc, i, BUS_DMASYNC_PREREAD);
>       sc->sk_status_idx = 0;
>  
>       sk_win_write_4(sc, SK_STAT_BMU_CSR, SK_STAT_BMU_RESET);
>               goto fail_5;
>       }

This looks okay. (I wonder why FreeBSD guys didn't put it ;-p)

> @@ -1841,8 +1841,7 @@
>               msk_intr_yukon(sc_if1);
>       }
>  
> -     MSK_CDSTSYNC(sc, sc->sk_status_idx,
> -         BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> +     MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_POSTREAD);
>       cur_st = &sc->sk_status_ring[sc->sk_status_idx];
>  
>       while (cur_st->sk_opcode & SK_Y2_STOPC_OWN) {

Hmm, if msk_status_desc has only data transfered from device
to memory, we don't have to have POSTWRITE (or PREWRITE).

> @@ -1872,12 +1871,17 @@
>                       aprint_error("opcode=0x%x\n", cur_st->sk_opcode);
>                       break;
>               }
> +             /* Write back after clearing SK_Y2_STOPC_OWN of opcode */
> +             MSK_CDSTSYNC(sc, sc->sk_status_idx,
> +                 BUS_DMASYNC_PREREAD|BUS_DMASYNC_PREWRITE);
> +

Why do you say "write back" here?

If there is no date transfered from memory to the device,
no writeback is needed.

>               SK_INC(sc->sk_status_idx, MSK_STATUS_RING_CNT);
>  
> -             MSK_CDSTSYNC(sc, sc->sk_status_idx,
> -                 BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> +             MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_POSTREAD);

This seems okay as the first one in this function.

>               cur_st = &sc->sk_status_ring[sc->sk_status_idx];
>       }
> +     /* Invalidate the status buffer that has already been cached */
> +     MSK_CDSTSYNC(sc, sc->sk_status_idx, BUS_DMASYNC_PREREAD);

In your patch, only the last descriptor gets PREREAD op,
i.e. the rest DMA descriptors are not flushed from cache.
(then you had to add extra PREREAD op in the above?)
You have to call PREREAD op after sk_status_ring is fetched
in while loop.
---
Izumi Tsutsui