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