Re: [Intel-wired-lan] [PATCH net-next v2] ixgbe: Add 10G-BX support

[Paul Menzel]

Dear Birger,


Thank you for your prompt reply.

Am 15.10.25 um 11:16 schrieb Birger Koblitz:

> On 15/10/2025 9:59 am, Paul Menzel wrote:
> > Am 14.10.25 um 06:18 schrieb Birger Koblitz:
> > > Adds support for 10G-BX modules, i.e. 10GBit Ethernet over a single strand
> > > Single-Mode fiber
> >
> > I’d use imperative mood, and add a dot/period at the end.
> I will put this into the next patch-version.
>
> > > @@ -1678,6 +1680,31 @@ int ixgbe_identify_sfp_module_generic(struct ixgbe_hw *hw)
> > >               else
> > >                   hw->phy.sfp_type =
> > >                       ixgbe_sfp_type_1g_bx_core1;
> > > +        /* Support Ethernet 10G-BX, checking the Bit Rate
> > > +         * Nominal Value as per SFF-8472 to be 12.5 Gb/s (67h) and
> > > +         * Single Mode fibre with at least 1km link length
> > > +         */
> > > +        } else if ((!comp_codes_10g) && (bitrate_nominal == 0x67) &&
> > > +               (!(cable_tech & IXGBE_SFF_DA_PASSIVE_CABLE)) &&
> > > +               (!(cable_tech & IXGBE_SFF_DA_ACTIVE_CABLE))) {
> > > +            status = hw->phy.ops.read_i2c_eeprom(hw,
> > > +                        IXGBE_SFF_SM_LENGTH_KM,
> > > +                        &sm_length_km);
> > > +            if (status != 0)
> > > +                goto err_read_i2c_eeprom;
> >
> > Should an error be logged?
> This needs to be read in the context of the rest of the SFP 
> identification function. Several bytes of the EEPROM have already been 
> read for module identification by the existing code before reaching this 
> point, and failure is handled everywhere by the same goto. What will 
> happen if EEPROM reading fails is that an error message will be logged 
> that the Module is not supported. This is because the type is not filled 
> in and the module therefore considered unsupported. The actual error 
> (ret_val = -ENOENT) is ignored e.g. in ixgbe_52599/ 
> ixgbe_init_phy_ops_82599(). The error logged is probably good enough: 
> the module cannot be positively identified and is not enabled. I say 
> good enough, because this is actually what is the case: the EEPROM is 
> broken and ther
>
> > > +            status = hw->phy.ops.read_i2c_eeprom(hw,
> > > +                        IXGBE_SFF_SM_LENGTH_100M,
> > > +                        &sm_length_100m);
> > > +            if (status != 0)
> > > +                goto err_read_i2c_eeprom;
> >
> > Should an error be logged?
> Same here.
>
> > > +            if (sm_length_km > 0 || sm_length_100m >= 10) {
> > > +                if (hw->bus.lan_id == 0)
> > > +                    hw->phy.sfp_type =
> > > +                        ixgbe_sfp_type_10g_bx_core0;
> > > +                else
> > > +                    hw->phy.sfp_type =
> > > +                        ixgbe_sfp_type_10g_bx_core1;
> >
> > I’d prefer the ternary operator, if only the same variable is assigned 
> > in both branches.
> Me, too. But this is merely code that can be found verbosely the same in 
> several places before in this identification function, for each type of 
> module identified basically once. If the same code would be written 
> differently in this place, it would probably confuse readers who would 
> wonder what is different.

You are right in all accounts. Thank you for the explanations.

Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>


Kind regards,

Paul