On Wed, Jul 27, 2022 at 08:53:38PM +0200, Mark Kettenis wrote: > > Date: Wed, 27 Jul 2022 00:11:19 +0200 > > From: Alexander Bluhm <alexander.bl...@gmx.net> > > > > 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 <alexander.bl...@gmx.net> > > > > > > > > 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. > >
NetBSD uses spin mutex by default for ifmedia protection. But it could use driver's mutex for that purpose. That's why they have pointer to lock. I don't like to keep lock while callback called. It introduces lock inconsistency like we have with (*if_qstart)() callbacks. May be we should pass the new media to the media change callback directly, like below: if (ifm->new_ifm_change_cb) error = (*ifm->new_ifm_change_cb)(ifm, ifp, match, newmedia); else { oldentry = ifm->ifm_cur; oldmedia = ifm->ifm_media; /* ... */ error = (*ifm->ifm_change_cb)(ifp); if (error && error != ENETRESET) { mtx_enter(&ifmedia_mtx); if (ifm->ifm_cur == match) { ifm->ifm_cur = oldentry; ifm->ifm_cur = oldentry; } mtx_leave(&ifmedia_mtx); } } This keeps the media change atomicy, and leaves `ifm' protection to the driver, but without pointers to locks and callback calls with lock held.