Page MenuHomeFreeBSD

igc: defer sysctl-driven reinit to the admin task
ClosedPublic

Authored by seuros on Mon, Aug 3, 11:32 PM.
Referenced Files
F169108839: D58629.id.diff
Mon, Aug 31, 10:49 AM
Unknown Object (File)
Sun, Aug 30, 11:53 AM
Unknown Object (File)
Sun, Aug 30, 9:03 AM
Unknown Object (File)
Thu, Aug 27, 11:02 AM
Unknown Object (File)
Thu, Aug 27, 9:00 AM
Unknown Object (File)
Thu, Aug 27, 9:00 AM
Unknown Object (File)
Wed, Aug 26, 7:21 PM
Unknown Object (File)
Wed, Aug 26, 5:00 PM
Subscribers

Details

Summary

igc_sysctl_eee() and igc_sysctl_dmac() called igc_if_init() directly.

Requested the reset through iflib instead, and skipping while the interface
is down; the new value is picked up by the next init.

This is the same defect fixed for e1000 in D58628. Unlike e1000, igc has
no ASSERT_CTX_LOCK_HELD and no acquire_swflag path, so the defect is
silent here rather than an assertion failure.

MFC: 1 week

Test Plan

Verified on an Intel I225-LM (igc0, 8086:15f2): eee_control 0/1 and dmac
250/1000/1/0 plus an illegal value all reinitialize cleanly, no panic,
and the link recovers after each write.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

seuros created this revision.
kbowling requested changes to this revision.Tue, Aug 4, 9:06 PM

Can you do the same for media_change and if_resume?

This revision now requires changes to proceed.Tue, Aug 4, 9:06 PM

Can you do the same for media_change and if_resume?

Yes, but i do not know how to trigger the panic, so i said i could do it in a follow-up diff.

It's one unit of work unifying a lifecycle issue

@kbowling resume and media_change are not the same. They already inside a CTX_LOCK already, no panic.

igc already does not panic per your own statement, so I'm not sure what you are arguing. My request is to unify lifecycle management as one commit. But now looking and iflib.c I think the igc_if_init should simply be dropped from resume and media_change so please do that instead.

This revision was not accepted when it landed; it landed in state Needs Revision.Wed, Aug 5, 4:44 AM
This revision was automatically updated to reflect the committed changes.