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.
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.
bluhm
> > 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);
> >
> >
