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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0d2b88ac-d23d-43a5-813d-2a8c4edaa3eb@birger-koblitz.de>
Date: Wed, 15 Oct 2025 11:16:05 +0200
From: Birger Koblitz <mail@...ger-koblitz.de>
To: Paul Menzel <pmenzel@...gen.mpg.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 Paul,

thank you for your feedback!

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.

Cheers,
   Birger