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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <bf36b4ed-e35f-4943-93ea-b24b27a48ad3@molgen.mpg.de>
Date: Wed, 15 Oct 2025 11:38:59 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Birger Koblitz <mail@...ger-koblitz.de>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH net-next v2] ixgbe: Add 10G-BX support

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


Kind regards,

Paul