Differential D59578

ice(4): Fix link bringup on driver load
ClosedPublic
Actions

Authored by kgalazka on Thu, Sep 10, 8:12 PM.

Details

Reviewers

kbowling

Group Reviewers

Intel Networking

Commits

rGdcb84ddcfe89: ice(4): Fix link bringup on driver load

Summary

Patch adding Total Port Shutdown support incorrectly
handled a case when this feature was not enabled in the NVM.
When TPS bit is not set driver should apply link configuration
according to user settings and update the status. Those steps
were mistakenly omitted, while the state flag was still set
to prevent link renegotation and status update on first
attempt to bring interface up with ifconfig.

Signed-off-by: Krzysztof Galazka <krzysztof.galazka@intel.com>

Reported by: kbowling
Fixes: 0011cd9f8863 ("ice(4): Support Total Port Shutdown on E830 devices")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kgalazka created this revision.Thu, Sep 10, 8:12 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Sep 10, 8:12 PM

kgalazka requested review of this revision.Thu, Sep 10, 8:12 PM

Harbormaster completed remote builds in B76757: Diff 186413.Thu, Sep 10, 8:12 PM

kgalazka mentioned this in D59300: ice: Refresh link status on first initialization.Thu, Sep 10, 8:13 PM

This nullifies D59300 and is a good fix. D59335 is still necessary.

sys/dev/ice/ice_lib.c
9727–9728	Is this additional condition needed? TPS clears LINK_ACTIVE_ON_DOWN, so TPS ports take the preceding ice_set_link(sc, false) branch and never reach this test. Without TPS, the condition is always true, making it equivalent to the original else. I think this hunk can be dropped unless there is another supported state transition I am missing.

Address @kbowling comment.

Added condition in else branch was in fact wrong. Refactor
whole condition to act correctly when IFF_UP flag is set.

Harbormaster completed remote builds in B77055: Diff 187067.Fri, Sep 18, 10:22 AM

kbowling accepted this revision.Fri, Sep 18, 11:31 AM

This revision is now accepted and ready to land.Fri, Sep 18, 11:31 AM

kbowling mentioned this in D59335: ice: Re-enable link after reset when link-on-down is disabled.Fri, Sep 18, 11:44 AM

Are you waiting for internal validation? I don't like keeping head in a known broken state this long if their findings can be posted as a followup.

Closed by commit rGdcb84ddcfe89: ice(4): Fix link bringup on driver load (authored by kgalazka). · Explain WhyMon, Sep 28, 11:54 AM

This revision was automatically updated to reflect the committed changes.

kgalazka added a commit: rGdcb84ddcfe89: ice(4): Fix link bringup on driver load.

In D59578#1377319, @kbowling wrote:

Are you waiting for internal validation? I don't like keeping head in a known broken state this long if their findings can be posted as a followup.

Yeah, I hoped the final version will get verified, but we had problems with validation environment due to: https://reviews.freebsd.org/D59998, so I merged it as you suggested.