- Ported new phy_types and speeds based from latest firmware header file.
- Introduced a macro to avoid code duplication and improve readability for the invocation of ifmedia_add().
Details
Details
Verified with phy_types BASECR4 and BASESR.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
The speed and the media type are not the same thing. A 100G SR4 phy will never support 100BASE-FX or 1000BASE-CX. How were these media type lists chosen?
| sys/dev/bnxt/if_bnxt.c | ||
|---|---|---|
| 2132 ↗ | (On Diff #33239) | Why is CX (twisted pair) mixed in with CR (twinax) here? |
| 2133 ↗ | (On Diff #33239) | How would a CR module support FX media? |
| 2135 ↗ | (On Diff #33239) | How does a CR module support CX media? |
| 2138 ↗ | (On Diff #33239) | These media type don't seem even slightly related to the phy type. |
Comment Actions
Thank you for the review, Stephen. Please find my response below.
A 100G SR4 phy will never support 100BASE-FX or 1000BASE-CX
<Chenna> But, 100G_SR4 phy can be configured in 100MB mode (force speed), in that case, I have to advertise 100MB speed right? but I don't see macro for 100MB_SR.
- Looks like iflib's if_media.h doesn't contain macros for all supported speeds. For example, 50GBASE-SR is supported one as per
https://en.wikipedia.org/wiki/Ethernet_physical_layer but it is missing in if_media.h (only IFM_50G_CR2, IFM_50G_KR2 & IFM_50G_PCIE are present).
- When firmware HWRM reporting 'phy_type' and 'supported speeds list' separately, technically, it is possible that any phy_type can support all speeds, Right?
- My idea is to *not suppress* any speed that HWRM specifies as 'supported speeds'. can you please share your thoughts how to achieve this? Because, new phy_types can be introduced In future firmware's and old driver can not identify those types, so at least driver should be able to advertise speed (even 1000M_Unknown also fine for example).
Comment Actions
A 100G SR4 phy will not support 100Mb mode because there is no physical layer definition for 100Mb over SR let alone over SR4. If you were to force that speed you would likely not get link, and if you did, it would not be at 100Mb. While the wavelength and fibre specs for 100BASE-SX (and even 10BASE-FL) is compatible with SR4, I seriously doubt any manufacturer would do the extra work to provide 10Mb or 100Mb physical layer that way. Have you seen a 100G module that advertises 100Mb support?
If you do want to advertise theoretically not impossible modes, at least ensure they use the same physical layer... twisted pair, twinax, fiber type/frequency, etc. If someone plugs in an SR4 module and it tells them they can use it to do 1Gb over balanced STP (1000BASE-CX), they're more likely to complain about that rather than be glad they can use their 1000BASE‑SX switch port. There are "dual rate" modules out there that can do 10GBASE-SR/1000BASE-SX for example... but nothing can do 10GBASE-SR/1000BASE-CX.
- Looks like iflib's if_media.h doesn't contain macros for all supported speeds. For example, 50GBASE-SR is supported one as per
https://en.wikipedia.org/wiki/Ethernet_physical_layer but it is missing in if_media.h (only IFM_50G_CR2, IFM_50G_KR2 & IFM_50G_PCIE are present).
Then it should be added once either hardware exists or 802.3cd is finalized. I'm not sure how you mean "is supported" in this context since as far as I know there are no phys that support it anywhere in the world.
- When firmware HWRM reporting 'phy_type' and 'supported speeds list' separately, technically, it is possible that any phy_type can support all speeds, Right?
I don't really understand the question. A specific phy will have a specific set of supported media types. While a phy can support multiple media types (ie: 10GBASE-SR/1000BASE-SX), it's not technically possible to run CX signalling over fibre.
- My idea is to *not suppress* any speed that HWRM specifies as 'supported speeds'. can you please share your thoughts how to achieve this? Because, new phy_types can be introduced In future firmware's and old driver can not identify those types, so at least driver should be able to advertise speed (even 1000M_Unknown also fine for example).
There is no control over link speed in FreeBSD, nor any list of supported link speeds. The closest you can come is to grab the list of media types FreeBSD supports from if_media.h, dig up the physical layer specification for it, then group by physical layer.
For HWRM_PORT_PHY_QCFG_OUTPUT_PHY_TYPE_UNKNOWN, attempting to define a list of "supported" media types under those conditions is silly, and anything except auto is pretty much guaranteed to be wrong. If this happens, firmware and/or driver updates will be required.
If new firmware uses new types that are not supported by the driver, the driver will need to be updated. There is really no way around this. Trying to "future proof" it by providing an incorrect list of media types is as bad as or worse than not supporting a new media type.
Comment Actions
Thanks for the review, Stephen.
While I'm waiting for the response from our firmware team, let me try to get some clarifications from you as well.
Do you think it is good idea to have a Kernel / iflib function (then all drivers can utilize) which derives IFM macro (ex. IFM_100G_CR4) from speed & phy_type?
based on my experience with Oce & bnxt, both drivers gets phy_type and speeds separately (from it's firmware) and have to derive IFM macro based on that.
I'm still trying to get the list of supported speeds for a given phy_type, If possible, can you plz help?
There should be no vendor specific customization for phy_type vs speeds, Am I correct?
Thanks,
Chenna.
Comment Actions
I'm not sure how you're using phy_type here, but the suggestion makes sense. The phy_type enumeration in hsi_struct_def.h appears to be a single media type.
Another option we may want to look into would be if the i2c ioctl is implemented, iflib could potentially query SFP module serial ID and fill the media type from the SFP data directly. SFP modules would then Just Work, and non-SFP would be the only case that needs to do special processing.
Comment Actions
Taken care of Stephen's review comments by removing speed / phy_type combinations which doesn't exist currently or technically not possible.
| sys/dev/bnxt/if_bnxt.c | ||
|---|---|---|
| 2132 ↗ | (On Diff #33239) |
<Chenna> Taken care!!
<Chenna> Taken care!!
<Chenna> Taken care!!
<Chenna> |
Comment Actions
Stephen,
Trimmed switch-case block a lot and allowing a speed only when it is supported by it's phy_type.
Can you please review and let me know if still there are any issues?
Another option we may want to look into would be if the i2c ioctl is implemented, iflib could potentially
query SFP module serial ID and fill the media type from the SFP data directly. SFP modules would then
Just Work, and non-SFP would be the only case that needs to do special processing.
<Chenna>
This is interesting!!
from where iflib can get the data, is it from pci_config space?
Does this requires additional support from driver / firmwares?
Comment Actions
It's part of the SFP specification. There's an optional callback in iflib (that bnxt doesn't support yet) to make I2C calls to get this information (as well as optics data, temperature, etc). There's an experimental hwrm command for it, but I don't think it's made it into the external header file yet. Once it has that, iflib could call it and fetch the data directly from the SFP module.
Comment Actions
Thank you, Stephen.
Let me explore more about I2C and get back. Meantime, can we push my revised patch?
Thanks,
Chenna.
Comment Actions
Minor changes requested.
| bnxt.h | ||
|---|---|---|
| 232 | This macro should be update per style(9) #define BNXT_IFMEDIA_ADD(supported, fw_speed, ifm_speed) do { \
if ((supported) & HWRM_PORT_PHY_QCFG_OUTPUT_SUPPORT_ ## fw_speed) \
ifmedia_add(softc->media, IFM_ETHER | (ifm_speed), 0, NULL); \
} while(0) | |
| bnxt.h | ||
|---|---|---|
| 232 | Thanks, Taken care. | |
Comment Actions
Double-check "compatible" media types.
| sys/dev/bnxt/if_bnxt.c | ||
|---|---|---|
| 2153 ↗ | (On Diff #34285) | I don't think there is a 50G LR media type. |
| 2154 ↗ | (On Diff #34285) | Should be IFM_40G_LR4 |
| 2155 ↗ | (On Diff #34285) | Should be IFM_25G_LR |
| 2156 ↗ | (On Diff #34285) | Shouldn't this be IFM_10G_LR? |
| 2157 ↗ | (On Diff #34285) | Should likely be IFM_1000_LX |
| 2167 ↗ | (On Diff #34285) | This one is clearly wrong. I don't think there is a 50G SR media type. |
| 2171 ↗ | (On Diff #34285) | SR is fibre, not copper. Should likely be IFM_1000_SX |
| 2183 ↗ | (On Diff #34285) | Should likely be IFM_1000_KX |
| 2186 ↗ | (On Diff #34285) | What is this? Direct attach or InfiniBand? |
| 2192 ↗ | (On Diff #34285) | Regardless of what it is, it's certainly not backplane. This is a speed duplicate of the above IFM_50G_CR2. |
| 2195 ↗ | (On Diff #34285) | This should likely go with the other SR types above (HWRM_PORT_PHY_QCFG_OUTPUT_PHY_TYPE_100G_BASESR10 etc). |
| 2204 ↗ | (On Diff #34285) | If the 1G SR type is fixed, this case should likely go with the SR. |
| 2216 ↗ | (On Diff #34285) | Maybe fill this with KR stuff? IFM_10G_KX4 will prevent IFM_10G_KR, but the rest should not be unreasonable. |
| sys/dev/bnxt/if_bnxt.c | ||
|---|---|---|
| 2153 ↗ | (On Diff #34285) | Taken care by deleting it!! |
| 2154 ↗ | (On Diff #34285) | Taken care. |
| 2155 ↗ | (On Diff #34285) | Taken care. |
| 2156 ↗ | (On Diff #34285) | Taken care |
| 2157 ↗ | (On Diff #34285) | Taken care |
| 2167 ↗ | (On Diff #34285) | Deleted!! Taken care. |
| 2171 ↗ | (On Diff #34285) | Taken care |
| 2183 ↗ | (On Diff #34285) | Taken care |
| 2186 ↗ | (On Diff #34285) | That is "Direct attach" only, Let me use these till I get confirmation from firmware team. #define IFM_10G_AOC IFM_X(59) /* 10G active optical cable */ |
| 2192 ↗ | (On Diff #34285) | Yes, this is duplicate entry, I'll delete it. Sorry for that. |
| 2195 ↗ | (On Diff #34285) | Taken care!! |
| 2204 ↗ | (On Diff #34285) | Sure!! Taken care. |
| 2216 ↗ | (On Diff #34285) | Sure!! I'll go with IFM_10G_KR. |
Comment Actions
Stephen,
For now, taken care of all of your review comments, I'm sorry for couple of duplicate entries / incompatible entries in earlier patch.
I'll be coordinating with firmware team for future additions/deletions (of speeds / phy_type compatibility) if any.
Thanks,
Chenna.