On Thu, Mar 10, 2022 at 07:47:16PM +0100, Mark Kettenis wrote:
> > In general, atomic_* functions have not provided implicit memory
> > barriers on OpenBSD.
> >
> > I am not sure if the data dependency barrier is needed where
> > atomic_load_int() and atomic_load_long() are used. The memory ordering
> > guarantee is very weak and does not seem useful in any of the use cases
> > in the patch. However, the barrier does not appear to make things worse
> > in terms of correctness. Except maybe in assertions where they cause
> > subtle side effects.
> >
> > However, the patch looks good.
> >
> > OK visa@
>
> I have some doubts about using atomic_load_xxx() in KASSERTs. A
> KASSERT really shoudn't have any side-effects. But atomic_load_xxx()
> has a memory barrier in it, which does have the side-effect of
> ordering memory accesses.
The memory barrier is #ifdef __alpha__. Without it the CPU might
read the wrong value. But you are right, a barrier should not
depend on DIAGNOSTIC. I guess not many Alpha MP users have Intel
wireless devices.
ok?
bluhm
Index: dev/pci/if_iwm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.392
diff -u -p -r1.392 if_iwm.c
--- dev/pci/if_iwm.c 10 Mar 2022 15:21:08 -0000 1.392
+++ dev/pci/if_iwm.c 10 Mar 2022 20:34:18 -0000
@@ -9975,7 +9975,7 @@ iwm_init(struct ifnet *ifp)
generation = ++sc->sc_generation;
- KASSERT(atomic_load_int(&sc->task_refs.r_refs) == 0);
+ KASSERT(sc->task_refs.r_refs == 0);
refcnt_init(&sc->task_refs);
err = iwm_preinit(sc);
@@ -10116,7 +10116,7 @@ iwm_stop(struct ifnet *ifp)
iwm_del_task(sc, systq, &sc->mac_ctxt_task);
iwm_del_task(sc, systq, &sc->phy_ctxt_task);
iwm_del_task(sc, systq, &sc->bgscan_done_task);
- KASSERT(atomic_load_int(&sc->task_refs.r_refs) >= 1);
+ KASSERT(sc->task_refs.r_refs >= 1);
refcnt_finalize(&sc->task_refs, "iwmstop");
iwm_stop_device(sc);
Index: dev/pci/if_iwx.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_iwx.c,v
retrieving revision 1.135
diff -u -p -r1.135 if_iwx.c
--- dev/pci/if_iwx.c 10 Mar 2022 15:21:08 -0000 1.135
+++ dev/pci/if_iwx.c 10 Mar 2022 20:34:40 -0000
@@ -8017,7 +8017,7 @@ iwx_init(struct ifnet *ifp)
if (sc->sc_nvm.sku_cap_11n_enable)
iwx_setup_ht_rates(sc);
- KASSERT(atomic_load_int(&sc->task_refs.r_refs) == 0);
+ KASSERT(sc->task_refs.r_refs == 0);
refcnt_init(&sc->task_refs);
ifq_clr_oactive(&ifp->if_snd);
ifp->if_flags |= IFF_RUNNING;
@@ -8139,7 +8139,7 @@ iwx_stop(struct ifnet *ifp)
iwx_del_task(sc, systq, &sc->mac_ctxt_task);
iwx_del_task(sc, systq, &sc->phy_ctxt_task);
iwx_del_task(sc, systq, &sc->bgscan_done_task);
- KASSERT(atomic_load_int(&sc->task_refs.r_refs) >= 1);
+ KASSERT(sc->task_refs.r_refs >= 1);
refcnt_finalize(&sc->task_refs, "iwxstop");
iwx_stop_device(sc);
