sys/dev/al_eth/al_eth.c
341–342	I dont think this should be changed. This `LINK_STATE_DOWN` is the initial state of `ifp`, and the `ifp` should not be exposed until fully constructed.
3516	I think this is correct usage of `if_link_state_change()`.

This revision now requires changes to proceed.Mar 8 2023, 3:41 AM

sys/dev/al_eth/al_eth.c
341–342	There's currently no way to just set the link state. I could add the API, but it would be used only by this driver. The initial state is LINK_STATE_UNKNOWN. @wma would it be possible for you to test the driver with the if_link_state setting removed? No other driver explicitly sets the link state.

Drop the initial link state. Nothing else sets initial link state.

I do not have al_eth devices. It is ideally someone can test this.

Add mw@ for review as well.

Generally looks good to me, except the concurrency of device_detach and miibus_statchg event.

sys/dev/al_eth/al_eth.c
341–342	I do not see any logic checking `adapter->netdev->if_link_state`. So an initial state `LINK_STATE_UNKNOWN` should be OK.
3519	I'm a little worried about this `LINK_STATE_DOWN` event. if_link_state_change(struct ifnet ifp, int link_state) { / Return if state hasn't changed. / if (ifp->if_link_state == link_state) return; ifp->if_link_state = link_state; / XXXGL: reference ifp? */ taskqueue_enqueue(taskqueue_swi, &ifp->if_linktask); } I'm not familiar with MII bus. Is it possible that we get concurrent `device_detach / al_detach()` and `miibus_statchg / al_miibus_statchg()` ? If yes then the above `LINK_STATE_DOWN` may lead use-after-free. See also D39614 .

This revision is now accepted and ready to land.Apr 25 2023, 3:31 AM

sys/dev/al_eth/al_eth.c
3519	I think al_detach() probably needs a bus_generic_detach() to detach its child miibus. This seems common across at least several drivers I've looked at.