diff mbox series

net: sfp: handle 100G/25G active optical cables in sfp_parse_support

Message ID 20230810094817.29262-1-josua@solid-run.com
State New
Headers
Series net: sfp: handle 100G/25G active optical cables in sfp_parse_support |

Commit Message

Josua Mayer Aug. 10, 2023, 9:48 a.m. UTC 
  Handle extended compliance code 0x1 (SFF8024_ECC_100G_25GAUI_C2M_AOC)
for active optical cables supporting 25G and 100G speeds.

Since the specification makes no statement about transmitter range, and
as the specific sfp module that had been tested features only 2m fiber -
short-range (SR) modes are selected.

sfp_parse_support already handles SFF8024_ECC_100GBASE_SR4_25GBASE_SR
with compatible properties: 100000baseSR4; 25000baseSR; protocol 25gbase-r.
Add SFF8024_ECC_100G_25GAUI_C2M_AOC to the same case.

Tested with fs.com S28-AO02 AOC SFP28 module.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 drivers/net/phy/sfp-bus.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Russell King (Oracle) Aug. 10, 2023, 10:39 a.m. UTC | #1 
On Thu, Aug 10, 2023 at 11:48:17AM +0200, Josua Mayer wrote:
> Handle extended compliance code 0x1 (SFF8024_ECC_100G_25GAUI_C2M_AOC)
> for active optical cables supporting 25G and 100G speeds.
> 
> Since the specification makes no statement about transmitter range, and
> as the specific sfp module that had been tested features only 2m fiber -
> short-range (SR) modes are selected.
> 
> sfp_parse_support already handles SFF8024_ECC_100GBASE_SR4_25GBASE_SR
> with compatible properties: 100000baseSR4; 25000baseSR; protocol 25gbase-r.
> Add SFF8024_ECC_100G_25GAUI_C2M_AOC to the same case.
> 
> Tested with fs.com S28-AO02 AOC SFP28 module.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>

Thanks. I think I would like one extra change:

> +	case SFF8024_ECC_100G_25GAUI_C2M_AOC:
>  	case SFF8024_ECC_100GBASE_SR4_25GBASE_SR:
>  		phylink_set(modes, 100000baseSR4_Full);

Since SFPs are single lane, SR4 doesn't make sense (which requires
four lanes), and I shouldn't have added it when adding these modes.
It would be a good idea to drop that, or at least for the
addition of the SFF8024_ECC_100G_25GAUI_C2M_AOC case.
Josua Mayer Aug. 10, 2023, 11:38 a.m. UTC | #2 
Hi Russell,

Am 10.08.23 um 12:39 schrieb Russell King (Oracle):
> On Thu, Aug 10, 2023 at 11:48:17AM +0200, Josua Mayer wrote:
>> Handle extended compliance code 0x1 (SFF8024_ECC_100G_25GAUI_C2M_AOC)
>> for active optical cables supporting 25G and 100G speeds.
> Thanks. I think I would like one extra change:
>
>> +	case SFF8024_ECC_100G_25GAUI_C2M_AOC:
>>   	case SFF8024_ECC_100GBASE_SR4_25GBASE_SR:
>>   		phylink_set(modes, 100000baseSR4_Full);
> Since SFPs are single lane, SR4 doesn't make sense (which requires
> four lanes), and I shouldn't have added it when adding these modes.
> It would be a good idea to drop that, or at least for the
> addition of the SFF8024_ECC_100G_25GAUI_C2M_AOC case.
>
Would it be okay changing 100000baseSR4 to 100000baseSR dropping the "4"?


- Josua Mayer
Russell King (Oracle) Aug. 10, 2023, 12:05 p.m. UTC | #3 
On Thu, Aug 10, 2023 at 01:38:13PM +0200, Josua Mayer wrote:
> Hi Russell,
> 
> Am 10.08.23 um 12:39 schrieb Russell King (Oracle):
> > On Thu, Aug 10, 2023 at 11:48:17AM +0200, Josua Mayer wrote:
> > > Handle extended compliance code 0x1 (SFF8024_ECC_100G_25GAUI_C2M_AOC)
> > > for active optical cables supporting 25G and 100G speeds.
> > Thanks. I think I would like one extra change:
> > 
> > > +	case SFF8024_ECC_100G_25GAUI_C2M_AOC:
> > >   	case SFF8024_ECC_100GBASE_SR4_25GBASE_SR:
> > >   		phylink_set(modes, 100000baseSR4_Full);
> > Since SFPs are single lane, SR4 doesn't make sense (which requires
> > four lanes), and I shouldn't have added it when adding these modes.
> > It would be a good idea to drop that, or at least for the
> > addition of the SFF8024_ECC_100G_25GAUI_C2M_AOC case.
> > 
> Would it be okay changing 100000baseSR4 to 100000baseSR dropping the "4"?

Not for SFF8024_ECC_100GBASE_SR4_25GBASE_SR. SFF-8024 states for this
code:

         02h        100GBASE-SR4 or 25GBASE-SR

100GBASE-SR4: IEEE 802.3 Physical Layer specification for 100 Gb/s using
100GBASE-R encoding over four lanes of multimode fiber, with reach
up to at least 100 m. (See IEEE Std 802.3, Clause 95.)

100GBASE-R encoding: The physical coding sublayer encoding defined in
Clause 82 for 100 Gb/s operation. (See IEEE Std 802.3, Clause 82.)

25GBASE-SR: IEEE 802.3 Physical Layer specification for 25 Gb/s using
25GBASE-R encoding over multimode fiber. (See IEEE Std 802.3, Clause 112.)

IEEE 802.3-2018 doesn't define 100GBASE-SR, so I assume that's a later
development, which would be 100GBASE-R encoding over one lane of fiber.

So, 100GBASE-SR and 100GBASE-SR4 are not equivalent, and since
SFF8024_ECC_100GBASE_SR4_25GBASE_SR specifies 100GBASE-SR4, that
being _four_ lanes of fiber, and SFP form-factor modules only being
capable of carrying a single lane, and sfp-bus.c only being for SFP
modules, 100GBASE-SR4 is just not relevant for our purposes in
sfp-bus.c - and it makes no sense to switch to 100GBASE-SR because
that is not what this code tells us.


For the SFF8024_ECC_100G_25GAUI_C2M_AOC in a SFP28 module, the SFP28
form factor only supports up to 28Gb/s, so that means the module is
definitely 25GBASE-R ethernet. So that also excludes 100G operation.

So, until we see a module in the SFP form factor (implying a single
lane) that does operate at 100G speeds, I think we should omit it.

I'm also wondering whether we should check br_nom/br_max/br_min now,
so that if we have to check that in the future, we don't start causing
regressions. Knowing how module EEPROMs are randomly wrong, it would
be a good idea to start with something sensible and see whether any
fail. Bear in mind that br_nom doesn't always get set to the correct
value - for example, 1G operates at 1250Mbps, and the SFP MSA specifies
that br_nom should be 1300 for 1G ethernet, but some modules use 1200.
I guess start at the correct value and then adjust to allow a range
as we see more modules.
Russell King (Oracle) Aug. 14, 2023, 12:22 p.m. UTC | #4 
On Mon, Aug 14, 2023 at 02:12:22PM +0200, Josua Mayer wrote:
> Hi Russell,
> 
> Am 10.08.23 um 14:05 schrieb Russell King (Oracle):
> > On Thu, Aug 10, 2023 at 01:38:13PM +0200, Josua Mayer wrote:
> > > Hi Russell,
> > > 
> > > Am 10.08.23 um 12:39 schrieb Russell King (Oracle):
> > > > On Thu, Aug 10, 2023 at 11:48:17AM +0200, Josua Mayer wrote:
> > > > > Handle extended compliance code 0x1 (SFF8024_ECC_100G_25GAUI_C2M_AOC)
> > > > > for active optical cables supporting 25G and 100G speeds.
> > > > Thanks. I think I would like one extra change:
> > > > 
> > > > > +	case SFF8024_ECC_100G_25GAUI_C2M_AOC:
> > > > >    	case SFF8024_ECC_100GBASE_SR4_25GBASE_SR:
> > > > >    		phylink_set(modes, 100000baseSR4_Full);
> > > > Since SFPs are single lane, SR4 doesn't make sense (which requires
> > > > four lanes), and I shouldn't have added it when adding these modes.
> > > > It would be a good idea to drop that, or at least for the
> > > > addition of the SFF8024_ECC_100G_25GAUI_C2M_AOC case.
> > > > 
> > > Would it be okay changing 100000baseSR4 to 100000baseSR dropping the "4"?
> > Not for SFF8024_ECC_100GBASE_SR4_25GBASE_SR. SFF-8024 states for this
> > code:
> > 
> >           02h        100GBASE-SR4 or 25GBASE-SR
> > 
> > 100GBASE-SR4: IEEE 802.3 Physical Layer specification for 100 Gb/s using
> > 100GBASE-R encoding over four lanes of multimode fiber, with reach
> > up to at least 100 m. (See IEEE Std 802.3, Clause 95.)
> > 
> > 100GBASE-R encoding: The physical coding sublayer encoding defined in
> > Clause 82 for 100 Gb/s operation. (See IEEE Std 802.3, Clause 82.)
> > 
> > 25GBASE-SR: IEEE 802.3 Physical Layer specification for 25 Gb/s using
> > 25GBASE-R encoding over multimode fiber. (See IEEE Std 802.3, Clause 112.)
> > 
> > IEEE 802.3-2018 doesn't define 100GBASE-SR, so I assume that's a later
> > development, which would be 100GBASE-R encoding over one lane of fiber.
> > 
> > So, 100GBASE-SR and 100GBASE-SR4 are not equivalent, and since
> > SFF8024_ECC_100GBASE_SR4_25GBASE_SR specifies 100GBASE-SR4, that
> > being _four_ lanes of fiber, and SFP form-factor modules only being
> > capable of carrying a single lane, and sfp-bus.c only being for SFP
> > modules, 100GBASE-SR4 is just not relevant for our purposes in
> > sfp-bus.c - and it makes no sense to switch to 100GBASE-SR because
> > that is not what this code tells us.
> > 
> > 
> > For the SFF8024_ECC_100G_25GAUI_C2M_AOC in a SFP28 module, the SFP28
> > form factor only supports up to 28Gb/s, so that means the module is
> > definitely 25GBASE-R ethernet. So that also excludes 100G operation.
> Okay. So probably the simple correct solution is to make a seperate
> case SFF8024_ECC_100G_25GAUI_C2M_AO that only sets 25gbase-r, and
> 25000baseSR_Full.
> > 
> > So, until we see a module in the SFP form factor (implying a single
> > lane) that does operate at 100G speeds, I think we should omit it.
> > 
> > I'm also wondering whether we should check br_nom/br_max/br_min now,
> > so that if we have to check that in the future, we don't start causing
> > regressions. Knowing how module EEPROMs are randomly wrong, it would
> > be a good idea to start with something sensible and see whether any
> > fail. Bear in mind that br_nom doesn't always get set to the correct
> > value - for example, 1G operates at 1250Mbps, and the SFP MSA specifies
> > that br_nom should be 1300 for 1G ethernet, but some modules use 1200.
> > I guess start at the correct value and then adjust to allow a range
> > as we see more modules.
> I don't fully understand how you would like to use br_nom.
> I see e.g. in sfp-bus.c at the end of sfp_parse_support a mapping of bitrate
> to 1000/2500 baseX modes.
> Are you referring to this section?
> 
> However there are no baseX modes for 25Gbps in ethtool.h - only SR/KR/CR.

I'm thinking something like:

	case SFF8024_ECC_100G_25GAUI_C2M_AO:
		if (br_min <= 28000 && br_max >= 25000) {
			/* 25GBASE-R, possibly with FEC */
			__set_bit(PHY_INTERFACE_MODE_25GBASER, interfaces);
			/* Re-use 25000baseSR as there is no 25Gbase- suffix
			 * for this
			 */
			phylink_set(modes, 25000baseSR_Full);
		}
		break;

I don't know what the actual numerical values should be though.
diff mbox series

Patch

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index e8dd47bffe43..db26e1d8a10d 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -258,6 +258,7 @@  void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 	switch (id->base.extended_cc) {
 	case SFF8024_ECC_UNSPEC:
 		break;
+	case SFF8024_ECC_100G_25GAUI_C2M_AOC:
 	case SFF8024_ECC_100GBASE_SR4_25GBASE_SR:
 		phylink_set(modes, 100000baseSR4_Full);
 		phylink_set(modes, 25000baseSR_Full);