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

[Paul Menzel <pmenzel@molgen.mpg.de>]

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@molgen.mpg.de>


Kind regards,

Paul