Page MenuHomeFreeBSD

allow iflib drivers to pass their own ifmedia
ClosedPublic

Authored by mmacy on Apr 17 2019, 8:31 PM.

Details

Summary

this should make it easier for miibus consumers to use iflib

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 23723

Event Timeline

mmacy created this revision.Apr 17 2019, 8:31 PM
mmacy updated this revision to Diff 56313.Apr 17 2019, 8:36 PM

missed change

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

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

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 added a comment.Apr 17 2019, 9:13 PM

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 updated this revision to Diff 56315.Apr 17 2019, 9:29 PM

don't call ifmedia_init for user provided ifmedia

mmacy added a comment.Apr 17 2019, 9:30 PM

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

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.

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.

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

No. It should just call iflib_init.

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.

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 added a comment.EditedApr 17 2019, 11:20 PM

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.

marius requested changes to this revision.Apr 25 2019, 3:08 PM
marius added a subscriber: marius.
marius added inline comments.
sys/net/iflib.c
4452

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

This revision now requires changes to proceed.Apr 25 2019, 3:08 PM

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

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 updated this revision to Diff 56825.Apr 29 2019, 7:21 PM
  • initialize media pointer for non miibus drivers
  • add flag for miibus drivers to set pass own media
mmacy updated this revision to Diff 56827.Apr 29 2019, 7:31 PM

use right patch

emaste added a comment.May 1 2019, 8:28 PM

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 added a comment.May 2 2019, 7:15 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.May 3 2019, 8:05 PM
This revision was automatically updated to reflect the committed changes.