> Date: Wed, 27 Jul 2022 00:11:19 +0200
> From: Alexander Bluhm <[email protected]>
>
> On Tue, Jul 26, 2022 at 11:19:27PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 26 Jul 2022 18:11:01 +0200
> > > From: Alexander Bluhm <[email protected]>
> > >
> > > On Fri, Jul 22, 2022 at 06:13:04PM +0200, Alexander Bluhm wrote:
> > > > But I am fine with committing ifmedia, gem(4) and bge(4) now. Then
> > > > we can decide on a per driver basis. As long kernel lock is not
> > > > removed from the ifmedia layer, this diff is not strictly necessary.
> > > > I want to commit it anyway to show how MP in ifmedia should work.
> > >
> > > This is the part for gem(4) and bge(4). I have tested them.
> > > ifmedia_current() is MP safe and provides the data which all the
> > > drivers need.
> > >
> > > After commiting this, I can look for more hardware to test.
> > >
> > > ok?
> >
> > I still don't understand what this fixes. Most drivers create a list
> > of media when the PHY attaches during autoconf. That list never
> > changes, which means that the pointer to the ifmedia struct remains
> > valid until the driver detaches.
> >
> > There is of course a race when you use ifconfig to change the media
> > type. But your new interface doesn't really fix that race. Code that
> > uses ifmedia_current() may still end up with the old information and
> > use that at a point where that information is no longer accurate.
>
> It does not fix a real bug. The diff tries to keep internal data
> structures within a file, instead of spreading them over all drivers.
> MP work is easier if each struct field has a clear lock that is
> responsible for it. 70 drivers that should change it only during
> attach is not that clear.
But making data structures "mpsafe" is not a goal in itself. The way
the data is used must be safe as well. If you don't think that
through, you run the risk of adding mutexes that don't actually help.
And this is starting to smell like one of those to me.
> The ifmedia_list is protected by mutex. ifm_cur is a pointer to
> an element of the list. It would be natural to protect it with the
> same mutex. ifm_cur is changed by ifmedia_set() and ifmedia_ioctl()
> and ifmedia_delete_instance().
>
> Look at bnxt_hwrm_port_phy_qcfg(). There the list is changed by
> bnxt_media_status() which is called from ifmedia_ioctl().
> ixgbe_handle_msf() changes the list during SPF interrupt. But I
> have to admit that these drivers do not access ifm_cur.
>
> Why do you refuse to make the ifmedia layer MP safe by itself?
> It is just one function and a mutex. I cannot see harm.
I looked a bit at what NetBSD does. I don't think what they do is a
good idea (in particular having pointers to locks). But they actually
hold a lock across the callbacks. Which made me realize that if you
don't do that you don't really fix the race between changes detected
by the PHY and manual changes through ioctl. And I think the
callbacks can sleep. So using a mutex may simply be wrong.
> > > Index: dev/ic/gem.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/ic/gem.c,v
> > > retrieving revision 1.127
> > > diff -u -p -r1.127 gem.c
> > > --- dev/ic/gem.c 12 Jul 2022 22:08:17 -0000 1.127
> > > +++ dev/ic/gem.c 26 Jul 2022 09:14:10 -0000
> > > @@ -1348,16 +1348,18 @@ void
> > > gem_mii_statchg(struct device *dev)
> > > {
> > > struct gem_softc *sc = (void *)dev;
> > > -#ifdef GEM_DEBUG
> > > - uint64_t instance = IFM_INST(sc->sc_mii.mii_media.ifm_cur->ifm_media);
> > > -#endif
> > > bus_space_tag_t t = sc->sc_bustag;
> > > bus_space_handle_t mac = sc->sc_h1;
> > > u_int32_t v;
> > >
> > > #ifdef GEM_DEBUG
> > > - if (sc->sc_debug)
> > > - printf("gem_mii_statchg: status change: phy = %lld\n",
> > > instance);
> > > + if (sc->sc_debug) {
> > > + uint64_t media;
> > > +
> > > + ifmedia_current(&sc->sc_mii.mii_media, &media, NULL);
> > > + printf("%s: status change: phy = %lld\n",
> > > + __func__, IFM_INST(media));
> > > + }
> > > #endif
> > >
> > > /* Set tx full duplex options */
> > > Index: dev/pci/if_bge.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_bge.c,v
> > > retrieving revision 1.398
> > > diff -u -p -r1.398 if_bge.c
> > > --- dev/pci/if_bge.c 11 Mar 2022 18:00:45 -0000 1.398
> > > +++ dev/pci/if_bge.c 26 Jul 2022 09:14:10 -0000
> > > @@ -1054,12 +1054,15 @@ bge_miibus_statchg(struct device *dev)
> > > {
> > > struct bge_softc *sc = (struct bge_softc *)dev;
> > > struct mii_data *mii = &sc->bge_mii;
> > > + uint64_t media;
> > > u_int32_t mac_mode, rx_mode, tx_mode;
> > >
> > > + ifmedia_current(&mii->mii_media, &media, NULL);
> > > +
> > > /*
> > > * Get flow control negotiation result.
> > > */
> > > - if (IFM_SUBTYPE(mii->mii_media.ifm_cur->ifm_media) == IFM_AUTO &&
> > > + if (IFM_SUBTYPE(media) == IFM_AUTO &&
> > > (mii->mii_media_active & IFM_ETH_FMASK) != sc->bge_flowflags)
> > > sc->bge_flowflags = mii->mii_media_active & IFM_ETH_FMASK;
> > >
> > > @@ -3095,7 +3098,8 @@ bge_attach(struct device *parent, struct
> > > 0, NULL);
> > > ifmedia_add(&sc->bge_ifmedia, IFM_ETHER|IFM_AUTO, 0, NULL);
> > > ifmedia_set(&sc->bge_ifmedia, IFM_ETHER|IFM_AUTO);
> > > - sc->bge_ifmedia.ifm_media = sc->bge_ifmedia.ifm_cur->ifm_media;
> > > + ifmedia_current(&sc->bge_ifmedia, &sc->bge_ifmedia.ifm_media,
> > > + NULL);
> > > } else {
> > > int mii_flags;
> > >
> > > Index: net/if_media.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.c,v
> > > retrieving revision 1.36
> > > diff -u -p -r1.36 if_media.c
> > > --- net/if_media.c 14 Jul 2022 13:46:25 -0000 1.36
> > > +++ net/if_media.c 26 Jul 2022 09:14:10 -0000
> > > @@ -430,6 +430,21 @@ ifmedia_get(struct ifmedia *ifm, uint64_
> > > return (match);
> > > }
> > >
> > > +void
> > > +ifmedia_current(struct ifmedia *ifm, uint64_t *media, u_int *data)
> > > +{
> > > + struct ifmedia_entry *ife;
> > > +
> > > + mtx_enter(&ifmedia_mtx);
> > > + ife = ifm->ifm_cur;
> > > + KASSERT(ife != NULL);
> > > + if (media != NULL)
> > > + *media = ife->ifm_media;
> > > + if (data != NULL)
> > > + *data = ife->ifm_data;
> > > + mtx_leave(&ifmedia_mtx);
> > > +}
> > > +
> > > /*
> > > * Delete all media for a given instance.
> > > */
> > > Index: net/if_media.h
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.h,v
> > > retrieving revision 1.45
> > > diff -u -p -r1.45 if_media.h
> > > --- net/if_media.h 14 Jul 2022 13:46:25 -0000 1.45
> > > +++ net/if_media.h 26 Jul 2022 09:14:10 -0000
> > > @@ -131,6 +131,9 @@ int ifmedia_ioctl(struct ifnet *, struct
> > > /* Locate a media entry */
> > > int ifmedia_match(struct ifmedia *, uint64_t, uint64_t);
> > >
> > > +/* Get current media and data */
> > > +void ifmedia_current(struct ifmedia *, uint64_t *, u_int *);
> > > +
> > > /* Delete all media for a given media instance */
> > > void ifmedia_delete_instance(struct ifmedia *, uint64_t);
> > >
> > >
>
