Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

axgbe: Various link stability and module compatibilty improvements #768

Closed
wants to merge 1 commit into from

Conversation

swhite2
Copy link
Contributor

@swhite2 swhite2 commented Jun 7, 2023

Move the phy_stop() routine to if_detach() to prevent link interruptions when configuring the interface. Accompanying this is a sanity check using phy_started, which was already there but remained unused. We do not move phy_start(), as the logic there is needed for any init routine, be it attach or start.

Also bring in the linux PMA_PLL change
which addresses the flapping of back-to-back fiber connections.

Use miibus for SFP PHYs up to 1G copper. We retry in cases where the PHY is not directly reachable.
Set the correct IFM_100_SGMII flag when the phy speed has been set to 100. We remove xgbe_phy_start_aneg() since it's handled by miibus.

Add support for 100 and 1000 BASE-BX fiber modules

Add support for 25G multirate DACs which are capable of 10G.

While here, also fixup the LINK_ERR state. It was impossible to recover from this previously.

Move the phy_stop() routine to if_detach() to prevent
link interruptions when configuring the interface. Accompanying
this is a sanity check using phy_started, which was already there
but remained unused. We do not move phy_start(), as the logic
there is needed for any init routine, be it attach or start.

Also bring in the linux PMA_PLL change
which addresses the flapping of back-to-back fiber connections.

Use miibus for SFP PHYs up to 1G copper. We retry in cases where
the PHY is not directly reachable.
Set the correct IFM_100_SGMII flag when the phy speed
has been set to 100. We remove xgbe_phy_start_aneg() since
it's handled by miibus.

Add support for 100 and 1000 BASE-BX fiber modules

Add support for 25G multirate DACs which are capable of 10G.

While here, also fixup the LINK_ERR state. It was impossible
to recover from this previously.
@bsdimp
Copy link
Member

bsdimp commented Jun 26, 2023

This change is too big. I'd suggest separating out things by each of the features that you call out in the commit message.
This will allow us to selectively back things out should there be trouble and allow users to bisect in case this breaks something.
I've not run tools/build/checkstyle9.pl on it, now it says:

ERROR: unnecessary whitespace before a quoted newline
#103: FILE: sys/dev/axgbe/xgbe-i2c.c:447:
+		axgbe_printf(1, "%s: i2c xfer ret %d abrt_source 0x%x \n", __func__,
ERROR: else should follow close brace '}'
#463: FILE: sys/dev/axgbe/xgbe-phy-v2.c:1294:
+	}
+	else if (sfp_base[XGBE_SFP_BASE_1GBE_CC] & XGBE_SFP_BASE_CC_PX)
ERROR: braces {} are necessary even for single statement blocks
#664: FILE: sys/dev/axgbe/xgbe-phy-v2.c:2930:
+		} else
+			xgbe_phy_rrc(pdata);

There's a few lines that are > 80 colmns, but are otherwise short enough.

@swhite2
Copy link
Contributor Author

swhite2 commented Jun 27, 2023

FWIW, this PR is already a subset (it pertains solely to link issues) of a larger diff, of which I've taken the liberty to set up for review in phabricator: https://reviews.freebsd.org/D40725. I can only assume the same logic you mentioned applies there, so if you'd like me to break it up into smaller changes here, I can abandon the diff in phabricator and start opening separate PRs.

@bsdimp
Copy link
Member

bsdimp commented Jul 7, 2023

Yes, the same logic applies... It might help get things reviewed, but also eases my concerns with too many things going in at once on the off chance there's an undetected issue (for hardware, it can be hard to get testers, or people really familiar with it). So if you break things up, I'll make sure they get into the tree and I'll try to beat the bushes to get reviewers).

freebsd-git pushed a commit that referenced this pull request Feb 2, 2024
Move the phy_stop() routine to if_detach() to prevent link interruptions
when configuring the interface. Accompanying this is a sanity check
using phy_started, which was already there but remained unused. We do
not move phy_start(), as the logic there is needed for any init routine,
be it attach or start.

Also bring in the linux PMA_PLL change which addresses the flapping of
back-to-back fiber connections.

Use miibus for SFP PHYs up to 1G copper. We retry in cases where the PHY
is not directly reachable.  Set the correct IFM_100_SGMII flag when the
phy speed has been set to 100. We remove xgbe_phy_start_aneg() since
it's handled by miibus.

Add support for 100 and 1000 BASE-BX fiber modules

Add support for 25G multirate DACs which are capable of 10G.

While here, also fixup the LINK_ERR state. It was impossible to recover
from this previously.

[[ Note: light style fixes by imp, slight commit message adjustment,
   and a warning that I don't have the hardware to validate, but
   the changes do track the commit message and seem otherwise OK ]]

Reviewed by: imp
Pull Request: #768
@bsdimp
Copy link
Member

bsdimp commented Feb 2, 2024

I sent ahead and landed this change. I took some time to study it since I had thought it would be good to give better feedback than I had. I found most of the complexity was due to the large number of cases being handled due to the many different link types. While it could be broken down, I'm not sure there's good RoI on doing that. It seems close enough to me, and I'll keep an eye out for regressions in axgbe.

Sorry for taking so long to resolve this bit. It's hard to review changes for hardware you're unfamiliar with.

445bed5

@swhite2
Copy link
Contributor Author

swhite2 commented Feb 3, 2024

I appreciate you taking the time to review this. I agree the changes were too big to begin with, but FWIW most of these changes have been running stable in production for well over a year now.

These changes specifically target SFP(+) connectivity, which is a bit of a rabbit hole in terms of possible configurations. While I stayed away from other areas as much as possible, regressions for other connection types (RJ45, backplane etc.) cannot be ruled out - though I doubt adoption of this is very high, otherwise you would likely have seen more patches around this area as this driver needed quite some work to get into a stable state.

There are a few more changes here, I'll be sure to split these up for future PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants