Page MenuHomeFreeBSD

bnxt: Correct the logic to report supported speeds
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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 ↗(On Diff #36179)

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

sys/dev/bnxt/if_bnxt.c
2065 ↗(On Diff #36179)

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 ↗(On Diff #36179)

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.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 19 2017, 10:16 PM
Closed by commit rS327003: Add log messages for unknown and unhandled phy types (authored by shurd, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.