al_eth: Finish conversion to IfAPI

[jhibbits]

Summary

One gotcha with this is I don't know if the if_link_state_change() calls
are correct, so need further review.

[zlei]

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

[jhibbits]

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.

[jhibbits]

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

[zlei]

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

[jhibbits]

Add mw@ for review as well.

[zlei]

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 .

[jhibbits]

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.