bnxt: Correct the logic to report supported speeds
ClosedPublic
Actions

Authored by siva.kallam_broadcom.com on Dec 4 2017, 9:10 AM.

Details

Reviewers

shurd
sbruno
bhargava.marreddy_broadcom.com

Commits

rS327003: Add log messages for unknown and unhandled phy types

Summary

phy_type UNKNOWN is treated as valid type and report auto if phy_type is unknown and log a debug message for user attention.
On Link up & down, update media types again. If Link state or media changes, user will observe correct supported media and speeds.
Adding debug message in default case for user attention.

Test Plan

If cable is not plugged in while driver load, making default supported speed as auto select.
Once cable is plugged in and on Link up, depend on PHY type populating the real supported speeds.
If Cable already plugged in while loading driver, populating supported speeds(This support already present).

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 13318
Build 13551: arc lint + arc unit

Event Timeline

siva.kallam_broadcom.com created this revision.Dec 4 2017, 9:10 AM

Harbormaster completed remote builds in B13317: Diff 36178.Dec 4 2017, 9:10 AM

Herald added a reviewer: shurd. · View Herald TranscriptDec 4 2017, 9:10 AM

Herald added a reviewer: shurd. · View Herald Transcript

Herald added subscribers: Contributor Reviews (src), imp. · View Herald Transcript

siva.kallam_broadcom.com edited the summary of this revision. (Show Details)Dec 4 2017, 9:12 AM

siva.kallam_broadcom.com edited the test plan for this revision. (Show Details)

siva.kallam_broadcom.com added reviewers: sbruno, bhargava.marreddy_broadcom.com.

Added proper comment for default case while checking phy_type

shurd added inline comments.Dec 4 2017, 7:10 PM

sys/dev/bnxt/if_bnxt.c
2065	What action is a user expected to take when seeing these two messages? Should the default case print the phy_type value as well? If new phy types are added in the future, old versions of the driver will report "ERROR: Invalid Phy type", but it should work aside from control over the media type... would that really be an error? This looks like the only place in the kernel that uses a "devX: INFORMATION: message" format... I don't think it's necessary to tell users that a log message contains information (the ERROR: prefix is fine if that actually indicates an error).

siva.kallam_broadcom.com added inline comments.Dec 5 2017, 10:34 AM

sys/dev/bnxt/if_bnxt.c
2065	Logging always gives hint for user if link is not up because of unhandled PHY type. Without this logging, User may get clue. Yes. I agree with you in logging phy_type in default/UNKNOWN phy_type case. Do you think unknown phy_type and default case should be clubbed and log single message as "UNKNOWN/INVALID phy type ID"? I am OK to club both UNKNOWN and default case with above message.

shurd added inline comments.Dec 5 2017, 6:36 PM

sys/dev/bnxt/if_bnxt.c
2065	I think in the default case, a "Reported PHY type %d not supported by driver" style message would be best. For the HWRM_PORT_PHY_QCFG_OUTPUT_PHY_TYPE_UNKNOWN case, it really depends on exactly what that message means. If it means there is no PHY, a message to that effect may make sense, but that's not what the message actually says, and it's the wrong field to pull that information from. I think that errors should be based on the module_status field, not the phy_type one. There's no clear indication in the documentation for phy_type that UNKNOWN indicates a cable is not plugged in.