Re: msk(4) require to sync status buffer

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

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

> I think require.
> Do we tell to have cleared bit SK_Y2_STOPC_OWN to LSI?

Ah, okay, it's my bad. I thought cur_rx is a structure because
bus_dmamap_sync(9) was called just before it was assigned,
but actually it's a pointer. Umm.

Anyway, I don't think it's a good idea to use a pointer
against DMA descriptor in that way because we should reduce
accesses against DMA descs to avoid cache issue.

I'd suggest to rewrite the while() loop in msk_intr(),
how about this one?
---
Izumi Tsutsui


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    26 Feb 2008 14:34:59 -0000
@@ -893,6 +893,8 @@
        /* Reset status ring. */
        bzero((char *)sc->sk_status_ring,
            MSK_STATUS_RING_CNT * sizeof(struct msk_status_desc));
+       bus_dmamap_sync(sc->sc_dmatag, sc->sk_status_map, 0,
+           sc->sk_status_map->dm_mapsize, BUS_DMASYNC_PREREAD);
        sc->sk_status_idx = 0;
 
        sk_win_write_4(sc, SK_STAT_BMU_CSR, SK_STAT_BMU_RESET);
@@ -1256,8 +1258,6 @@
                goto fail_5;
        }
        sc->sk_status_ring = (struct msk_status_desc *)kva;
-       bzero(sc->sk_status_ring,
-           MSK_STATUS_RING_CNT * sizeof(struct msk_status_desc));
 
        /* Reset the adapter. */
        msk_reset(sc);
@@ -1741,11 +1741,12 @@
                if (sc_if->sk_cdata.sk_tx_cnt <= 0)
                        break;
                prog++;
+               cur_tx = &sc_if->sk_rdata->sk_tx_ring[cons];
+
                MSK_CDTXSYNC(sc_if, cons, 1,
                    BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
-
-               cur_tx = &sc_if->sk_rdata->sk_tx_ring[cons];
                sk_ctl = cur_tx->sk_ctl;
+               MSK_CDTXSYNC(sc_if, cons, 1, BUS_DMASYNC_PREREAD);
 #ifdef MSK_DEBUG
                if (mskdebug >= 2)
                        msk_dump_txdesc(cur_tx, cons);
@@ -1816,6 +1817,9 @@
        struct ifnet            *ifp0 = NULL, *ifp1 = NULL;
        int                     claimed = 0;
        u_int32_t               status;
+       uint32_t                st_status;
+       uint16_t                st_len;
+       uint8_t                 st_opcode, st_link;
        struct msk_status_desc  *cur_st;
 
        status = CSR_READ_4(sc, SK_Y2_ISSR2);
@@ -1841,42 +1845,45 @@
                msk_intr_yukon(sc_if1);
        }
 
-       MSK_CDSTSYNC(sc, sc->sk_status_idx,
-           BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
-       cur_st = &sc->sk_status_ring[sc->sk_status_idx];
-
-       while (cur_st->sk_opcode & SK_Y2_STOPC_OWN) {
-               cur_st->sk_opcode &= ~SK_Y2_STOPC_OWN;
+       for (;;) {
+               cur_st = &sc->sk_status_ring[sc->sk_status_idx];
+               MSK_CDSTSYNC(sc, sc->sk_status_idx,
+                   BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
+               st_opcode = cur_st->sk_opcode;
+               if ((st_opcode & SK_Y2_STOPC_OWN) == 0) {
+                       MSK_CDSTSYNC(sc, sc->sk_status_idx,
+                           BUS_DMASYNC_PREREAD);
+                       break;
+               }
+               st_status = le32toh(cur_st->sk_status);
+               st_len = le16toh(cur_st->sk_len);
+               st_link = cur_st->sk_link;
+               cur_st->sk_opcode = st_opcode & ~SK_Y2_STOPC_OWN;
+               MSK_CDSTSYNC(sc, sc->sk_status_idx,
+                   BUS_DMASYNC_PREREAD|BUS_DMASYNC_PREWRITE);
+       
                switch (cur_st->sk_opcode) {
                case SK_Y2_STOPC_RXSTAT:
-                       msk_rxeof(sc->sk_if[cur_st->sk_link],
-                           letoh16(cur_st->sk_len),
-                           letoh32(cur_st->sk_status));
-                       SK_IF_WRITE_2(sc->sk_if[cur_st->sk_link], 0,
+                       msk_rxeof(sc->sk_if[st_link], st_len, st_status);
+                       SK_IF_WRITE_2(sc->sk_if[st_link], 0,
                            SK_RXQ1_Y2_PREF_PUTIDX,
-                           sc->sk_if[cur_st->sk_link]->sk_cdata.sk_rx_prod);
+                           sc->sk_if[st_link]->sk_cdata.sk_rx_prod);
                        break;
                case SK_Y2_STOPC_TXSTAT:
                        if (sc_if0)
-                               msk_txeof(sc_if0,
-                                   letoh32(cur_st->sk_status)
+                               msk_txeof(sc_if0, st_status
                                    & SK_Y2_ST_TXA1_MSKL);
                        if (sc_if1)
                                msk_txeof(sc_if1,
-                                   ((letoh32(cur_st->sk_status)
-                                       & SK_Y2_ST_TXA2_MSKL)
+                                   ((st_status & SK_Y2_ST_TXA2_MSKL)
                                        >> SK_Y2_ST_TXA2_SHIFTL)
-                                   | ((letoh16(cur_st->sk_len) & 
SK_Y2_ST_TXA2_MSKH) << SK_Y2_ST_TXA2_SHIFTH));
+                                   | ((st_len & SK_Y2_ST_TXA2_MSKH) << 
SK_Y2_ST_TXA2_SHIFTH));
                        break;
                default:
-                       aprint_error("opcode=0x%x\n", cur_st->sk_opcode);
+                       aprint_error("opcode=0x%x\n", st_opcode);
                        break;
                }
                SK_INC(sc->sk_status_idx, MSK_STATUS_RING_CNT);
-
-               MSK_CDSTSYNC(sc, sc->sk_status_idx,
-                   BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
-               cur_st = &sc->sk_status_ring[sc->sk_status_idx];
        }
 
        if (status & SK_Y2_IMR_BMU) {