| 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
| ||
|
Date: Thu, 8 Dec 2022 14:39:24 -0800
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Corinna Vinschen <vinschen@...hat.com>,
<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
Leon Romanovsky <leon@...nel.org>
CC: <patryk.piotrowski@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH net v2] igb: conditionalize I2C bit
banging on external thermal sensor support
On 12/8/2022 3:21 AM, Corinna Vinschen wrote:
> Commit a97f8783a937 ("igb: unbreak I2C bit-banging on i350") introduced
> code to change I2C settings to bit banging unconditionally.
>
> However, this patch introduced a regression: On an Intel S2600CWR
> Server Board with three NICs:
>
> - 1x dual-port copper
> Intel I350 Gigabit Network Connection [8086:1521] (rev 01)
> fw 1.63, 0x80000dda
>
> - 2x quad-port SFP+ with copper SFP Avago ABCU-5700RZ
> Intel I350 Gigabit Fiber Network Connection [8086:1522] (rev 01)
> fw 1.52.0
>
> the SFP NICs no longer get link at all. Reverting commit a97f8783a937
> or switching to the Intel out-of-tree driver both fix the problem.
>
> Per the igb out-of-tree driver, I2C bit banging on i350 depends on
> support for an external thermal sensor (ETS). However, commit
> a97f8783a937 added bit banging unconditionally. Additionally, the
> out-of-tree driver always calls init_thermal_sensor_thresh on probe,
> while our driver only calls init_thermal_sensor_thresh only in
> igb_reset(), and only if an ETS is present, ignoring the internal
> thermal sensor. The affected SFPs don't provide an ETS. Per Intel,
> the behaviour is a result of i350 firmware requirements.
>
> This patch fixes the problem by aligning the behaviour to the
> out-of-tree driver:
>
> - split igb_init_i2c() into two functions:
> - igb_init_i2c() only performs the basic I2C initialization.
> - igb_set_i2c_bb() makes sure that E1000_CTRL_I2C_ENA is set
> and enables bit-banging.
>
> - igb_probe() only calls igb_set_i2c_bb() if an ETS is present.
>
> - igb_probe() calls init_thermal_sensor_thresh() unconditionally.
>
> - igb_reset() aligns its behaviour to igb_probe(), i. e., call
> igb_set_i2c_bb() if an ETS is present and call
> init_thermal_sensor_thresh() unconditionally.
>
> v2: Add variable name in function declaration,
> rearrange declaration of local variables
>
> Fixes: a97f8783a937 ("igb: unbreak I2C bit-banging on i350")
> Co-authored-by: Jamie Bainbridge <jbainbri@...hat.com>
Checkpatch doesn't like this: WARNING: Non-standard signature:
Co-authored-by:
Co-developed-by: Jamie Bainbridge <jbainbri@...hat.com>
Signed-off-by: Jamie Bainbridge <jbainbri@...hat.com>
Would convey the same and keep checkpatch happy.
> Tested-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Signed-off-by: Corinna Vinschen <vinschen@...hat.com>
> Signed-off-by: Jamie Bainbridge <jbainbri@...hat.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 44 +++++++++++++++++------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 4e65ffe3f4e3..7f56322b3ec2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -138,6 +138,9 @@ static irqreturn_t igb_msix_ring(int irq, void *);
> static void igb_update_dca(struct igb_q_vector *);
> static void igb_setup_dca(struct igb_adapter *);
> #endif /* CONFIG_IGB_DCA */
> +#ifdef CONFIG_IGB_HWMON
> +static void igb_set_i2c_bb(struct e1000_hw *hw);
> +#endif /* CONFIG_IGB_HWMON */
I believe the preference is to put the function in a place where the
forward declaration isn't needed.
> static int igb_poll(struct napi_struct *, int);
> static bool igb_clean_tx_irq(struct igb_q_vector *, int);
> static int igb_clean_rx_irq(struct igb_q_vector *, int);
> @@ -2399,7 +2402,8 @@ void igb_reset(struct igb_adapter *adapter)
> * interface.
> */
> if (adapter->ets)
> - mac->ops.init_thermal_sensor_thresh(hw);
> + igb_set_i2c_bb(hw);
> + mac->ops.init_thermal_sensor_thresh(hw);
> }
> }
> #endif
> @@ -3116,21 +3120,12 @@ static void igb_init_mas(struct igb_adapter *adapter)
> **/
> static s32 igb_init_i2c(struct igb_adapter *adapter)
> {
> - struct e1000_hw *hw = &adapter->hw;
> s32 status = 0;
> - s32 i2cctl;
>
> /* I2C interface supported on i350 devices */
> if (adapter->hw.mac.type != e1000_i350)
> return 0;
>
> - i2cctl = rd32(E1000_I2CPARAMS);
> - i2cctl |= E1000_I2CBB_EN
> - | E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
> - | E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
> - wr32(E1000_I2CPARAMS, i2cctl);
> - wrfl();
> -
> /* Initialize the i2c bus which is controlled by the registers.
> * This bus will use the i2c_algo_bit structure that implements
> * the protocol through toggling of the 4 bits in the register.
> @@ -3146,6 +3141,30 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
> return status;
> }
>
> +#ifdef CONFIG_IGB_HWMON
> +/**
> + * igb_set_i2c_bb - Init I2C interface
> + * @adapter: pointer to adapter structure
Copy/paste issue I assume. This needs to document hw, not adapter.
> + **/
> +static void igb_set_i2c_bb(struct e1000_hw *hw)
> +{
> + u32 ctrl_ext;
> + s32 i2cctl;
> +
> + ctrl_ext = rd32(E1000_CTRL_EXT);
> + ctrl_ext |= E1000_CTRL_I2C_ENA;
> + wr32(E1000_CTRL_EXT, ctrl_ext);
> + wrfl();
> +
> + i2cctl = rd32(E1000_I2CPARAMS);
> + i2cctl |= E1000_I2CBB_EN
> + | E1000_I2C_CLK_OE_N
> + | E1000_I2C_DATA_OE_N;
> + wr32(E1000_I2CPARAMS, i2cctl);
> + wrfl();
> +}
> +#endif
> +
> /**
> * igb_probe - Device Initialization Routine
> * @pdev: PCI device information struct
> @@ -3520,6 +3539,11 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> adapter->ets = true;
> else
> adapter->ets = false;
> + /* Only enable I2C bit banging if an external thermal
> + sensor is supported. */
This comment style is incorrect.
/* Only enable I2C bit banging if an external thermal
* sensor is supported.
*/
> + if (adapter->ets)
> + igb_set_i2c_bb(hw);
Indentation is off: WARNING: suspect code indent for conditional
statements (16, 18)
> + hw->mac.ops.init_thermal_sensor_thresh(hw);
> if (igb_sysfs_init(adapter))
> dev_err(&pdev->dev,
> "failed to allocate sysfs resources\n");
Powered by blists - more mailing lists