| Message ID | 20200507132135.316-1-marek.behun@nic.cz |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: sfp: add some quirks for FreeTel direct attach modules | expand |
On Fri, May 08, 2020 at 05:30:06PM +0300, Denis Kirjanov wrote: > On Thursday, May 7, 2020, Marek Behún <marek.behun@nic.cz> wrote: > > > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from > > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do > > not report sfp_ct_passive nor sfp_ct_active in their ERPROM. > > > > These modules can also operate at 1000baseX and 2500baseX. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c > > index 6900c68260e0..f021709bedcc 100644 > > --- a/drivers/net/phy/sfp-bus.c > > +++ b/drivers/net/phy/sfp-bus.c > > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct > > sfp_eeprom_id *id, > > phylink_set(modes, 2500baseX_Full); > > } > > > > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id, > > + unsigned long *modes) > > +{ > > + phylink_set(modes, 10000baseCR_Full); > > + phylink_set(modes, 2500baseX_Full); > > + phylink_set(modes, 1000baseX_Full); > > +} > > + > > static const struct sfp_quirk sfp_quirks[] = { > > { > > // Alcatel Lucent G-010S-P can operate at 2500base-X, but > > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = { > > .vendor = "HUAWEI", > > .part = "MA5671A", > > .modes = sfp_quirk_2500basex, > > + }, { > > + // FreeTel P.C30.2 is a SFP+ direct attach that can > > operate at > > + // at 1000baseX, 2500baseX and 10000baseCR, but may report > > none > > + // of these in their EEPROM > > > Hi Marek, > > The comment style above is not what Linux kernel uses The comment style is per file - keeping consistency within a file is far more important than rigid conformance to some coding style. That is, unless someone is prepared to convert said file. The commenting style for these entries is to use "//" style.
On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote: > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do > not report sfp_ct_passive nor sfp_ct_active in their ERPROM. > > These modules can also operate at 1000baseX and 2500baseX. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Cc: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c > index 6900c68260e0..f021709bedcc 100644 > --- a/drivers/net/phy/sfp-bus.c > +++ b/drivers/net/phy/sfp-bus.c > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id, > phylink_set(modes, 2500baseX_Full); > } > > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id, > + unsigned long *modes) > +{ > + phylink_set(modes, 10000baseCR_Full); > + phylink_set(modes, 2500baseX_Full); > + phylink_set(modes, 1000baseX_Full); > +} > + > static const struct sfp_quirk sfp_quirks[] = { > { > // Alcatel Lucent G-010S-P can operate at 2500base-X, but > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = { > .vendor = "HUAWEI", > .part = "MA5671A", > .modes = sfp_quirk_2500basex, > + }, { > + // FreeTel P.C30.2 is a SFP+ direct attach that can operate at > + // at 1000baseX, 2500baseX and 10000baseCR, but may report none > + // of these in their EEPROM > + .vendor = "FreeTel", > + .part = "P.C30.2", > + .modes = sfp_quirk_direct_attach_10g, > + }, { > + // same as previous > + .vendor = "FreeTel", > + .part = "P.C30.3", > + .modes = sfp_quirk_direct_attach_10g, Looking at the EEPROM capabilities, it seems that these modules give either: Transceiver codes : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 Transceiver type : Infiniband: 1X Copper Passive Transceiver type : Passive Cable Transceiver type : FC: Twin Axial Pair (TW) Encoding : 0x06 (64B/66B) BR, Nominal : 10300MBd Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] BR margin, max : 0% BR margin, min : 0% or: Transceiver codes : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 Transceiver type : Passive Cable Transceiver type : FC: Twin Axial Pair (TW) Encoding : 0x06 (64B/66B) BR, Nominal : 10300MBd Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] BR margin, max : 0% BR margin, min : 0% These give ethtool capability mask of 000,00000600,0000e040, which is: 2500baseX (bit 15) 1000baseX (bit 41) 10000baseCR (bit 42) 10000baseCR, 2500baseX and 1000baseX comes from: if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) { /* This may look odd, but some manufacturers use 12000MBd */ if (br_min <= 12000 && br_max >= 10300) phylink_set(modes, 10000baseCR_Full); if (br_min <= 3200 && br_max >= 3100) phylink_set(modes, 2500baseX_Full); if (br_min <= 1300 && br_max >= 1200) phylink_set(modes, 1000baseX_Full); since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and br_min = 0. 10000baseCR will also come from: if (id->base.sfp_ct_passive) { if (id->base.passive.sff8431_app_e) phylink_set(modes, 10000baseCR_Full); } You claimed in your patch description that sfp_ct_passive is not set, but the EEPROM dumps contain: Transceiver type : Passive Cable which is correctly parsed by the kernel. So, I'm rather confused, and I don't see why this patch is needed.
On Fri, 8 May 2020 16:28:44 +0100 Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote: > > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from > > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do > > not report sfp_ct_passive nor sfp_ct_active in their ERPROM. > > > > These modules can also operate at 1000baseX and 2500baseX. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c > > index 6900c68260e0..f021709bedcc 100644 > > --- a/drivers/net/phy/sfp-bus.c > > +++ b/drivers/net/phy/sfp-bus.c > > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id, > > phylink_set(modes, 2500baseX_Full); > > } > > > > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id, > > + unsigned long *modes) > > +{ > > + phylink_set(modes, 10000baseCR_Full); > > + phylink_set(modes, 2500baseX_Full); > > + phylink_set(modes, 1000baseX_Full); > > +} > > + > > static const struct sfp_quirk sfp_quirks[] = { > > { > > // Alcatel Lucent G-010S-P can operate at 2500base-X, but > > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = { > > .vendor = "HUAWEI", > > .part = "MA5671A", > > .modes = sfp_quirk_2500basex, > > + }, { > > + // FreeTel P.C30.2 is a SFP+ direct attach that can operate at > > + // at 1000baseX, 2500baseX and 10000baseCR, but may report none > > + // of these in their EEPROM > > + .vendor = "FreeTel", > > + .part = "P.C30.2", > > + .modes = sfp_quirk_direct_attach_10g, > > + }, { > > + // same as previous > > + .vendor = "FreeTel", > > + .part = "P.C30.3", > > + .modes = sfp_quirk_direct_attach_10g, > > Looking at the EEPROM capabilities, it seems that these modules give > either: > > Transceiver codes : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 > Transceiver type : Infiniband: 1X Copper Passive > Transceiver type : Passive Cable > Transceiver type : FC: Twin Axial Pair (TW) > Encoding : 0x06 (64B/66B) > BR, Nominal : 10300MBd > Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] > BR margin, max : 0% > BR margin, min : 0% > > or: > > Transceiver codes : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 > Transceiver type : Passive Cable > Transceiver type : FC: Twin Axial Pair (TW) > Encoding : 0x06 (64B/66B) > BR, Nominal : 10300MBd > Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] > BR margin, max : 0% > BR margin, min : 0% > > These give ethtool capability mask of 000,00000600,0000e040, which > is: > > 2500baseX (bit 15) > 1000baseX (bit 41) > 10000baseCR (bit 42) > > 10000baseCR, 2500baseX and 1000baseX comes from: > > if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) { > /* This may look odd, but some manufacturers use 12000MBd */ > if (br_min <= 12000 && br_max >= 10300) > phylink_set(modes, 10000baseCR_Full); > if (br_min <= 3200 && br_max >= 3100) > phylink_set(modes, 2500baseX_Full); > if (br_min <= 1300 && br_max >= 1200) > phylink_set(modes, 1000baseX_Full); > > since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and > br_min = 0. > > 10000baseCR will also come from: > > if (id->base.sfp_ct_passive) { > if (id->base.passive.sff8431_app_e) > phylink_set(modes, 10000baseCR_Full); > } > > You claimed in your patch description that sfp_ct_passive is not set, > but the EEPROM dumps contain: > > Transceiver type : Passive Cable > > which is correctly parsed by the kernel. > > So, I'm rather confused, and I don't see why this patch is needed. > Russell, something is wrong here, and it is my bad. I hope I didn't mix the EEPROM images from when I was playing with the contents, but it seems possible now :( I probably sent you modified images and lost the original ones. The thing I know for sure is that it did not work when I got the cables and also that they had different contents inside - ie at least one side of one cable did not report ct_passive nor ct_active. And I think that they reported different things on each side. I will try to get another such cable and return to this. Marek
On Fri, May 08, 2020 at 06:27:06PM +0200, Marek Behun wrote: > On Fri, 8 May 2020 16:28:44 +0100 > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > > On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote: > > > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from > > > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do > > > not report sfp_ct_passive nor sfp_ct_active in their ERPROM. > > > > > > These modules can also operate at 1000baseX and 2500baseX. > > > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c > > > index 6900c68260e0..f021709bedcc 100644 > > > --- a/drivers/net/phy/sfp-bus.c > > > +++ b/drivers/net/phy/sfp-bus.c > > > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id, > > > phylink_set(modes, 2500baseX_Full); > > > } > > > > > > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id, > > > + unsigned long *modes) > > > +{ > > > + phylink_set(modes, 10000baseCR_Full); > > > + phylink_set(modes, 2500baseX_Full); > > > + phylink_set(modes, 1000baseX_Full); > > > +} > > > + > > > static const struct sfp_quirk sfp_quirks[] = { > > > { > > > // Alcatel Lucent G-010S-P can operate at 2500base-X, but > > > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = { > > > .vendor = "HUAWEI", > > > .part = "MA5671A", > > > .modes = sfp_quirk_2500basex, > > > + }, { > > > + // FreeTel P.C30.2 is a SFP+ direct attach that can operate at > > > + // at 1000baseX, 2500baseX and 10000baseCR, but may report none > > > + // of these in their EEPROM > > > + .vendor = "FreeTel", > > > + .part = "P.C30.2", > > > + .modes = sfp_quirk_direct_attach_10g, > > > + }, { > > > + // same as previous > > > + .vendor = "FreeTel", > > > + .part = "P.C30.3", > > > + .modes = sfp_quirk_direct_attach_10g, > > > > Looking at the EEPROM capabilities, it seems that these modules give > > either: > > > > Transceiver codes : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 > > Transceiver type : Infiniband: 1X Copper Passive > > Transceiver type : Passive Cable > > Transceiver type : FC: Twin Axial Pair (TW) > > Encoding : 0x06 (64B/66B) > > BR, Nominal : 10300MBd > > Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] > > BR margin, max : 0% > > BR margin, min : 0% > > > > or: > > > > Transceiver codes : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00 > > Transceiver type : Passive Cable > > Transceiver type : FC: Twin Axial Pair (TW) > > Encoding : 0x06 (64B/66B) > > BR, Nominal : 10300MBd > > Passive Cu cmplnce. : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only] > > BR margin, max : 0% > > BR margin, min : 0% > > > > These give ethtool capability mask of 000,00000600,0000e040, which > > is: > > > > 2500baseX (bit 15) > > 1000baseX (bit 41) > > 10000baseCR (bit 42) > > > > 10000baseCR, 2500baseX and 1000baseX comes from: > > > > if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) { > > /* This may look odd, but some manufacturers use 12000MBd */ > > if (br_min <= 12000 && br_max >= 10300) > > phylink_set(modes, 10000baseCR_Full); > > if (br_min <= 3200 && br_max >= 3100) > > phylink_set(modes, 2500baseX_Full); > > if (br_min <= 1300 && br_max >= 1200) > > phylink_set(modes, 1000baseX_Full); > > > > since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and > > br_min = 0. > > > > 10000baseCR will also come from: > > > > if (id->base.sfp_ct_passive) { > > if (id->base.passive.sff8431_app_e) > > phylink_set(modes, 10000baseCR_Full); > > } > > > > You claimed in your patch description that sfp_ct_passive is not set, > > but the EEPROM dumps contain: > > > > Transceiver type : Passive Cable > > > > which is correctly parsed by the kernel. > > > > So, I'm rather confused, and I don't see why this patch is needed. > > > > Russell, > > something is wrong here, and it is my bad. I hope I didn't mix > the EEPROM images from when I was playing with the contents, but it > seems possible now :( I probably sent you modified images and lost the > original ones. Ah. > The thing I know for sure is that it did not work when I got the > cables and also that they had different contents inside - ie at least > one side of one cable did not report ct_passive nor ct_active. And I > think that they reported different things on each side. One of your files did have a difference in the capabilities - it was missing the Infiniband capability mentioned above. > I will try to get another such cable and return to this. Thanks. One thing we could do is to augment the "guessing" towards the end of sfp_parse_support() so that if we encounter a module indicating support for 10300MBd and 64B/66B, we could set one of the 10G properties so that at least we get some functionality from modules that report no capabilities. See: /* If we haven't discovered any modes that this module supports, try * the encoding and bitrate to determine supported modes. Some BiDi * modules (eg, 1310nm/1550nm) are not 1000BASE-BX compliant due to * the differing wavelengths, so do not set any transceiver bits. */ if (bitmap_empty(modes, __ETHTOOL_LINK_MODE_MASK_NBITS)) { /* If the encoding and bit rate allows 1000baseX */ if (id->base.encoding == SFF8024_ENCODING_8B10B && br_nom && br_min <= 1300 && br_max >= 1200) phylink_set(modes, 1000baseX_Full); }
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c index 6900c68260e0..f021709bedcc 100644 --- a/drivers/net/phy/sfp-bus.c +++ b/drivers/net/phy/sfp-bus.c @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id, phylink_set(modes, 2500baseX_Full); } +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id, + unsigned long *modes) +{ + phylink_set(modes, 10000baseCR_Full); + phylink_set(modes, 2500baseX_Full); + phylink_set(modes, 1000baseX_Full); +} + static const struct sfp_quirk sfp_quirks[] = { { // Alcatel Lucent G-010S-P can operate at 2500base-X, but @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = { .vendor = "HUAWEI", .part = "MA5671A", .modes = sfp_quirk_2500basex, + }, { + // FreeTel P.C30.2 is a SFP+ direct attach that can operate at + // at 1000baseX, 2500baseX and 10000baseCR, but may report none + // of these in their EEPROM + .vendor = "FreeTel", + .part = "P.C30.2", + .modes = sfp_quirk_direct_attach_10g, + }, { + // same as previous + .vendor = "FreeTel", + .part = "P.C30.3", + .modes = sfp_quirk_direct_attach_10g, }, };
FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from their EEPROM. They report correct nominal bitrate of 10300 MBd, but do not report sfp_ct_passive nor sfp_ct_active in their ERPROM. These modules can also operate at 1000baseX and 2500baseX. Signed-off-by: Marek Behún <marek.behun@nic.cz> Cc: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)