allow iflib drivers to pass their own ifmedia

[mmacy]

Summary

this should make it easier for miibus consumers to use iflib

[mmacy]

missed change

[aryeeteygerald_rogers.com]

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.

[aryeeteygerald_rogers.com]

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

[aryeeteygerald_rogers.com]

sys/net/iflib.c

4451–4453

Note for the miibus case, I don't think you want to init/add/set anything because the phy does that already

[aryeeteygerald_rogers.com]

sys/net/iflib.c

4451–4453

Actually I forgot that mii_media is setup when the miibus is attached (sometime after iflib_device_register). So my comments shouldn't apply. iflib_media_change and iflib_media_status aren't exposed though.

[mmacy]

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

[mmacy]

don't call ifmedia_init for user provided ifmedia

[mmacy]

@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.

[aryeeteygerald_rogers.com]

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

[aryeeteygerald_rogers.com]

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.

[mmacy]

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.

[mmacy]

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.

[aryeeteygerald_rogers.com]

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?

[mmacy]

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.

[aryeeteygerald_rogers.com]

This comment was removed by aryeeteygerald_rogers.com.

[aryeeteygerald_rogers.com]

nvm, that makes sense.

[marius]

sys/net/iflib.c

4452

Second level indent are 4 spaces according to style(9). Apart from that, this change looks good to me.

[emaste]

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

[emaste]

Fatal data abort:
  x0: fffffd00101e8a80
  x1: fffffd00101e8a80
  x2: fffffd001007b600
  x3: ffff000040250000
  x4:               20
  x5:               54
  x6:              12c
  x7: ffff0000000103b0
  x8:                0
  x9:                0
 x10:  ffffffffff00000
 x11:                0
 x12:         92000000
 x13: ffff000000d59bb8
 x14:                2
 x15:            10000
 x16:                1
 x17:            10000
 x18: ffff000000010360
 x19:                0
 x20:                0
 x21:                0
 x22:               23
 x23: fffffd001186c000
 x24: ffff000161612648
 x25: fffffd0010e0f820
 x26:                4
 x27:                1
 x28:                0
 x29: ffff000000010380
  sp: ffff000000010360
  lr: ffff0000004be420
 elr: ffff0000004be42c
spsr:         a00001c5
 far:               10
 esr:         96000004
panic: vm_fault failed: ffff0000004be42c

[aryeeteygerald_rogers.com]

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.

[mmacy]

• initialize media pointer for non miibus drivers
• add flag for miibus drivers to set pass own media

[mmacy]

use right patch

[mmacy]

sigh

[emaste]

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.

[emaste]

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.