Page MenuHomeFreeBSD

bnxt: Add support for new phy_types and speeds
ClosedPublic

Authored by bhargava.marreddy_broadcom.com on Sep 20 2017, 2:29 PM.

Details

Summary
  1. Ported new phy_types and speeds based from latest firmware header file.
  2. Introduced a macro to avoid code duplication and improve readability for the invocation of ifmedia_add().
Test Plan

Verified with phy_types BASECR4 and BASESR.

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

shurd added a comment.Sep 20 2017, 6:11 PM

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.

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.

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

  1. When firmware HWRM reporting 'phy_type' and 'supported speeds list' separately, technically, it is possible that any phy_type can support all speeds, Right?
  1. 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).
shurd added a comment.Sep 21 2017, 6:11 PM

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.

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.

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

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

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

sbruno requested changes to this revision.Sep 27 2017, 6:49 PM

I suspect this review needs to have stephen's comments addressed.

This revision now requires changes to proceed.Sep 27 2017, 6:49 PM

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.

shurd added a comment.Oct 5 2017, 1:47 PM

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?

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.

Taken care of Stephen's review comments by removing speed / phy_type combinations which doesn't exist currently or technically not possible.

bhargava.marreddy_broadcom.com marked 4 inline comments as done.Oct 6 2017, 2:26 PM
bhargava.marreddy_broadcom.com added inline comments.
sys/dev/bnxt/if_bnxt.c
2132 ↗(On Diff #33239)

Why is CX (twisted pair) mixed in with CR (twinax) here?

<Chenna> Taken care!!

How would a CR module support FX media?

<Chenna> Taken care!!

How does a CR module support CX media?

<Chenna> Taken care!!

These media type don't seem even slightly related to the phy type.

<Chenna>
Advertising a speed only when it matches with it's phy_type.

bhargava.marreddy_broadcom.com marked an inline comment as done.Oct 6 2017, 2:33 PM

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?

shurd added a comment.Oct 17 2017, 2:29 AM

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?

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.

Thank you, Stephen.

Let me explore more about I2C and get back. Meantime, can we push my revised patch?

Thanks,
Chenna.

shurd requested changes to this revision.Oct 23 2017, 4:57 PM

Minor changes requested.

bnxt.h
232 ↗(On Diff #33752)

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)
This revision now requires changes to proceed.Oct 23 2017, 4:57 PM
bhargava.marreddy_broadcom.com marked an inline comment as done.Oct 24 2017, 1:40 PM
bhargava.marreddy_broadcom.com added inline comments.
bnxt.h
232 ↗(On Diff #33752)

Thanks, Taken care.

bhargava.marreddy_broadcom.com marked an inline comment as done.

Taken care of review comments.

shurd accepted this revision.Oct 24 2017, 7:53 PM

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.

shurd requested changes to this revision.Oct 24 2017, 7:54 PM

Whoops, I meant to require changes.

This revision now requires changes to proceed.Oct 24 2017, 7:54 PM
bhargava.marreddy_broadcom.com marked 12 inline comments as done.Oct 27 2017, 7:24 AM
bhargava.marreddy_broadcom.com added inline comments.
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 */
#define IFM_25G_ACC IFM_X(60) /* 25G active copper 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.

bhargava.marreddy_broadcom.com marked 12 inline comments as done.

Taken care of all review comments.

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.

bhargava.marreddy_broadcom.com marked an inline comment as done.Oct 27 2017, 9:23 AM
sbruno accepted this revision.Oct 30 2017, 8:19 PM
This revision is now accepted and ready to land.Oct 30 2017, 8:57 PM
This revision was automatically updated to reflect the committed changes.