Should probably note that this would require miibus users (and other consumers of this) to correctly init the status and change callbacks to iflib_media_change and iflib_media_status.

I think a check/assert for that (and maybe a comment on isc_media) would be good idea

I can just skip the iflib_media_init if we can just trust that the phy is correctly inited. Those two routines are just wrappers to drive user callbacks:

static int
iflib_media_change(if_t ifp)
{
	if_ctx_t ctx = if_getsoftc(ifp);
	int err;

	CTX_LOCK(ctx);
	if ((err = IFDI_MEDIA_CHANGE(ctx)) == 0)
		iflib_init_locked(ctx);
	CTX_UNLOCK(ctx);
	return (err);
}

static void
iflib_media_status(if_t ifp, struct ifmediareq *ifmr)
{
	if_ctx_t ctx = if_getsoftc(ifp);

	CTX_LOCK(ctx);
	IFDI_UPDATE_ADMIN_STATUS(ctx);
	IFDI_MEDIA_STATUS(ctx, ifmr);
	CTX_UNLOCK(ctx);
}

@aryeeteygerald_rogers.com can you test with this patch and let me know? It should be a no-op for current drivers so can probably go in quickly.

the custom media would have to call the wrappers, especially if restart is necessary

As is, the patch works.

I would still suggest exposing the iflib_media_* because init_locked is unavailable in the case of reset and I think the locks in the wrapper are useful?

In D19946#428672, @mmacy wrote:

can you test with this patch and let me know? It should be a no-op for current drivers so can probably go in quickly.

In D19946#428687, @aryeeteygerald_rogers.com wrote:

As is, the patch works.

I would still suggest exposing the iflib_media_* because init_locked is unavailable in the case of reset and I think the locks in the wrapper are useful?

Call iflib_init. You don't need the iflib lock held across your PHY routines - the wrapper you see here is so that normal drivers don't have to maintain their own locks. Any dedicated PHY driver is going to already be using its own locks. Exposing these would only further complicate the API with no benefit.

In D19946#428672, @mmacy wrote:

can you test with this patch and let me know? It should be a no-op for current drivers so can probably go in quickly.

In D19946#428673, @aryeeteygerald_rogers.com wrote:

the custom media would have to call the wrappers, especially if restart is necessary

No. It should just call iflib_init.

In D19946#428690, @mmacy wrote:

In D19946#428687, @aryeeteygerald_rogers.com wrote:

As is, the patch works.

I would still suggest exposing the iflib_media_* because init_locked is unavailable in the case of reset and I think the locks in the wrapper are useful?

Call iflib_init. You don't need the iflib lock held across your PHY routines - the wrapper you see here is so that normal drivers don't have to maintain their own locks. Any dedicated PHY driver is going to already be using its own locks. Exposing these would only further complicate the API with no benefit.

In D19946#428672, @mmacy wrote:

can you test with this patch and let me know? It should be a no-op for current drivers so can probably go in quickly.

Makes sense ... you mean call the ifdi_init method directly? If so, it looks like init_locked does a bunch of queue/iflib management ... is all of that unnecessary on reset?

Makes sense ... you mean call the ifdi_init method directly? If so, it looks like init_locked does a bunch of queue/iflib management ... is all of that unnecessary on reset?

Actually I mean iflib_if_init

static void
iflib_if_init(void *arg)
{
	if_ctx_t ctx = arg;

	CTX_LOCK(ctx);
	iflib_if_init_locked(ctx);
	CTX_UNLOCK(ctx);
}

which you can call by way of

struct ifnet *ifp = iflib_ifp_get(); 
ifp->if_init()

Your driver may not need to have the queues reset, but on most hardware where the PHY and MAC are managed together it avoids issues. In general, if the overhead of reset is an issue you're calling reset far far too often.

nvm, that makes sense.

Testing with this patch shows a panic attaching igb (on Ampere eMAG), in:

ifmedia_add() at em_if_attach_post+0xe4
         pc = 0xffff0000004be41c  lr = 0xffff00000014607c
         sp = 0xffff000000010390  fp = 0xffff000000010430

em_if_attach_post() at iflib_device_register+0xdcc
         pc = 0xffff00000014607c  lr = 0xffff0000004c5544
         sp = 0xffff000000010440  fp = 0xffff0000000104d0

iflib_device_register() at iflib_device_attach+0xd0
         pc = 0xffff0000004c5544  lr = 0xffff0000004c8ed0
         sp = 0xffff0000000104e0  fp = 0xffff000000010500

iflib_device_attach() at device_attach+0x3ec
         pc = 0xffff0000004c8ed0  lr = 0xffff0000003f680c
         sp = 0xffff000000010510  fp = 0xffff000000010560

For more info on this bug, it occurs because all iflib drivers atm have iflib_get_media in IFDI_ATTACH_PRE. I don't think any of them actually use the media until later so you can either move the calls down into IFDI_ATTACH_POST or initialize the pointer and media before IFDI_ATTACH_PRE.

Updated patch works for me in the non-miibus (i.e., existing) case, tested with igb on Ampere eMAG. I have not yet updated Gerald's D20079 (to set the flag) or retested, but I'd be happy for this to go into HEAD now.

I merged this D19946 and D20142 into my local test tree, rebased D20079 on that and then passed the new IFLIB_DRIVER_MEDIA flag and basic functionality in the mgb driver works.