Re: [Intel-wired-lan] [PATCH iwl-next v2 1/2] ixgbe: Refactor overtemp event handling
From: Nguyen, Anthony L Sent: Friday, December 8, 2023 12:17 AM >On 12/5/2023 3:28 AM, Jedrzej Jagielski wrote: >> Currently ixgbe driver is notified of overheating events via internal >> IXGBE_ERR_OVERTEMP error code. >> >> Change the approach to use freshly introduced is_overtemp function >> parameter which set when such event occurs. >> Add new parameter to the check_overtemp() and handle_lasi() phy ops. > >These patches don't apply. There are also checkpatch warnings on this. WARNING: function definition argument 'struct ixgbe_hw *' should also have an identifier name #186: FILE: drivers/net/ethernet/intel/ixgbe/ixgbe_type.h:3512: + s32 (*check_overtemp)(struct ixgbe_hw *, bool *); I am aware of these warnings but that is common that ops are declared without specifying name of function params in ixgbe driver so i just kept that rule. > >> Signed-off-by: Jedrzej Jagielski ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-next v3 1/2] ixgbe: Refactor overtemp event handling
From: Kitszel, Przemyslaw
Sent: Friday, December 8, 2023 11:07 AM
>On 12/8/23 10:00, Jedrzej Jagielski wrote:
>> Currently ixgbe driver is notified of overheating events
>> via internal IXGBE_ERR_OVERTEMP error code.
>>
>> Change the approach to use freshly introduced is_overtemp
>> function parameter which set when such event occurs.
>> Add new parameter to the check_overtemp() and handle_lasi()
>> phy ops.
>>
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> v2: change aproach to use additional function parameter to notify when
>> overheat
>
>on public mailing lists its best to require links to previous versions
>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20
>> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 33 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h | 2 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 4 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 47 ---
>> 5 files changed, 67 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 227415d61efc..f6200f0d1e06 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -2756,7 +2756,7 @@ static void ixgbe_check_overtemp_subtask(struct
>> ixgbe_adapter *adapter)
>> {
>> struct ixgbe_hw *hw = &adapter->hw;
>> u32 eicr = adapter->interrupt_event;
>> -s32 rc;
>> +bool overtemp;
>>
>> if (test_bit(__IXGBE_DOWN, &adapter->state))
>> return;
>> @@ -2790,14 +2790,15 @@ static void ixgbe_check_overtemp_subtask(struct
>> ixgbe_adapter *adapter)
>> }
>>
>> /* Check if this is not due to overtemp */
>> -if (hw->phy.ops.check_overtemp(hw) != IXGBE_ERR_OVERTEMP)
>> +hw->phy.ops.check_overtemp(hw, &overtemp);
>
>you newer (at least in the scope of this patch) check return code of
>.check_overtemp(), so you could perhaps instead change it to return
>bool, and just return "true if overtemp detected
Generally I think it is good think to give a possibility to return error code,
despite here it is not used (no possibility to handle it since called from
void function, just return).
All other phy ops are also s32 type, so this one is aligned with them.
@Nguyen, Anthony L What do you think on that solution?
>
>> +if (!overtemp)
>> return;
>>
>> break;
>> case IXGBE_DEV_ID_X550EM_A_1G_T:
>> case IXGBE_DEV_ID_X550EM_A_1G_T_L:
>> -rc = hw->phy.ops.check_overtemp(hw);
>> -if (rc != IXGBE_ERR_OVERTEMP)
>> +hw->phy.ops.check_overtemp(hw, &overtemp);
>> +if (!overtemp)
>> return;
>> break;
>> default:
>> @@ -2807,6 +2808,7 @@ static void ixgbe_check_overtemp_subtask(struct
>> ixgbe_adapter *adapter)
>> return;
>> break;
>> }
>> +
>
>I would remove chunks that are whitespace only
>
>> e_crit(drv, "%s\n", ixgbe_overheat_msg);
>>
>> adapter->interrupt_event = 0;
>> @@ -7938,7 +7940,7 @@ static void ixgbe_service_timer(struct timer_list *t)
>
>[snip]
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-next v3 1/2] ixgbe: Refactor overtemp event handling
From: Nguyen, Anthony L
Sent: Monday, December 11, 2023 10:35 PM
>On 12/11/2023 1:45 AM, Jagielski, Jedrzej wrote:
>> From: Kitszel, Przemyslaw
>> Sent: Friday, December 8, 2023 11:07 AM
>>
>>> On 12/8/23 10:00, Jedrzej Jagielski wrote:
>>>> Currently ixgbe driver is notified of overheating events
>>>> via internal IXGBE_ERR_OVERTEMP error code.
>>>>
>>>> Change the approach to use freshly introduced is_overtemp
>>>> function parameter which set when such event occurs.
>>>> Add new parameter to the check_overtemp() and handle_lasi()
>>>> phy ops.
>>>>
>>>> Signed-off-by: Jedrzej Jagielski
>>>> ---
>>>> v2: change aproach to use additional function parameter to notify when
>>>> overheat
>>>
>>> on public mailing lists its best to require links to previous versions
>>>
>>>> ---
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 33 +
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h | 2 +-
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 4 +-
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 47 ---
>>>>5 files changed, 67 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 227415d61efc..f6200f0d1e06 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -2756,7 +2756,7 @@ static void ixgbe_check_overtemp_subtask(struct
>>>> ixgbe_adapter *adapter)
>>>>{
>>>>struct ixgbe_hw *hw = &adapter->hw;
>>>>u32 eicr = adapter->interrupt_event;
>>>> - s32 rc;
>>>> + bool overtemp;
>>>>
>>>>if (test_bit(__IXGBE_DOWN, &adapter->state))
>>>>return;
>>>> @@ -2790,14 +2790,15 @@ static void ixgbe_check_overtemp_subtask(struct
>>>> ixgbe_adapter *adapter)
>>>>}
>>>>
>>>>/* Check if this is not due to overtemp */
>>>> - if (hw->phy.ops.check_overtemp(hw) != IXGBE_ERR_OVERTEMP)
>>>> + hw->phy.ops.check_overtemp(hw, &overtemp);
>>>
>>> you newer (at least in the scope of this patch) check return code of
>>> .check_overtemp(), so you could perhaps instead change it to return
>>> bool, and just return "true if overtemp detected
>>
>> Generally I think it is good think to give a possibility to return error
>> code,
>> despite here it is not used (no possibility to handle it since called from
>> void function, just return).
>> All other phy ops are also s32 type, so this one is aligned with them.
>>
>> @Nguyen, Anthony L What do you think on that solution?
>
>We shouldn't carry a return value only to align with other ops. If we
Sure, just thought it is standardized some way in that case.
>there's no need for it, we shouldn't have it... however, aren't you
>using/checking it here?
actually there is no need since just overtemp indication is checked
>
>@@ -406,9 +407,12 @@ s32 ixgbe_reset_phy_generic(struct ixgbe_hw *hw)
> if (status != 0 || hw->phy.type == ixgbe_phy_none)
> return status;
>
>+ status = hw->phy.ops.check_overtemp(hw, &overtemp);
>+ if (status)
>+ return status;
>
>Thanks,
>Tony
Thanks
Jedrek
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-next v4 1/2] ixgbe: Refactor overtemp event handling
From: Simon Horman
Sent: Thursday, December 14, 2023 10:56 AM
>On Tue, Dec 12, 2023 at 11:46:41AM +0100, Jedrzej Jagielski wrote:
>> Currently ixgbe driver is notified of overheating events
>> via internal IXGBE_ERR_OVERTEMP error code.
>>
>> Change the approach for handle_lasi() to use freshly introduced
>> is_overtemp function parameter which set when such event occurs.
>> Change check_overtemp() to bool and return true if overtemp
>> event occurs.
>>
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> v2: change aproach to use additional function parameter to notify when
>> overheat
>> v4: change check_overtemp to bool
>>
>> https://lore.kernel.org/netdev/20231208090055.303507-1-jedrzej.jagiel...@intel.com/T/
>> ---
>
>Hi Jedrzej,
>
>I like where this patch-set is going.
>Please find some feedback from my side inline.
Hi Simon
thank you for the review.
>
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19
>> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 26 ++-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h | 2 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 4 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 45 +++
>> 5 files changed, 54 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 227415d61efc..9bff614788a2 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -2756,7 +2756,7 @@ static void ixgbe_check_overtemp_subtask(struct
>> ixgbe_adapter *adapter)
>> {
>> struct ixgbe_hw *hw = &adapter->hw;
>> u32 eicr = adapter->interrupt_event;
>> -s32 rc;
>> +bool overtemp;
>>
>> if (test_bit(__IXGBE_DOWN, &adapter->state))
>> return;
>> @@ -2790,14 +2790,15 @@ static void ixgbe_check_overtemp_subtask(struct
>> ixgbe_adapter *adapter)
>> }
>>
>> /* Check if this is not due to overtemp */
>> -if (hw->phy.ops.check_overtemp(hw) != IXGBE_ERR_OVERTEMP)
>> +overtemp = hw->phy.ops.check_overtemp(hw);
>> +if (!overtemp)
>> return;
>
>I like the readability of the above, but FWIIW, I think it could
>also be slightly more compactly written as (completely untested!):
>
> if (!hw->phy.ops.check_overtemp(hw))
> return;
I decided to do that this way in order to improve readability,
but sure it can be changed.
>
>>
>> break;
>> case IXGBE_DEV_ID_X550EM_A_1G_T:
>> case IXGBE_DEV_ID_X550EM_A_1G_T_L:
>> -rc = hw->phy.ops.check_overtemp(hw);
>> -if (rc != IXGBE_ERR_OVERTEMP)
>> +overtemp = hw->phy.ops.check_overtemp(hw);
>> +if (!overtemp)
>> return;
>> break;
>> default:
>> @@ -7938,7 +7939,7 @@ static void ixgbe_service_timer(struct timer_list *t)
>> static void ixgbe_phy_interrupt_subtask(struct ixgbe_adapter *adapter)
>> {
>> struct ixgbe_hw *hw = &adapter->hw;
>> -u32 status;
>> +bool overtemp;
>>
>> if (!(adapter->flags2 & IXGBE_FLAG2_PHY_INTERRUPT))
>> return;
>> @@ -7948,11 +7949,9 @@ static void ixgbe_phy_interrupt_subtask(struct
>> ixgbe_adapter *adapter)
>> if (!hw->phy.ops.handle_lasi)
>> return;
>>
>> -status = hw->phy.ops.handle_lasi(&adapter->hw);
>> -if (status != IXGBE_ERR_OVERTEMP)
>> -return;
>> -
>> -e_crit(drv, "%s\n", ixgbe_overheat_msg);
>> +hw->phy.ops.handle_lasi(&adapter->hw, &overtemp);
>
>Unless I am mistaken, the above can return an error. Should it be checked?
Since we are inside a void function we don't have many options to handle that.
I could be:
err = hw->phy.ops.handle_lasi(&adapter->hw, &overtemp);
if (err)
return;
if (!overtemp)
return;
So i decided to shorten it just to
if (overtemp)
...
Some solution instead of returning here is to log warning when error
encountered.
>
>Or alternatively, as this seems to be the only call-site,
>could handle_lasi() return overtemp as a bool?
Actually handle_lasi() was designed to handle not only overtemp events
but also link status ones. When changing it to bool it would be
hard to differentiate them - then true only for overtemp case and false
when link change or any error? I am not sure if this is a good direction.
>
>> +if (overtemp)
>> +e_crit(drv, "%s\n", ixgbe_overheat_msg);
>> }
>>
>> static void ixgbe_reset_subtask(struct ixgbe_adapter *adapter)
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
>> index ca31638c6fb8..343c3ca9b1c9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/
Re: [Intel-wired-lan] [PATCH iwl-next v2] ixgbe: Convert ret val type from s32 to int
From: Nguyen, Anthony L
Sent: Friday, January 5, 2024 10:50 PM
>On 1/5/2024 2:31 AM, Jedrzej Jagielski wrote:
>> Currently big amount of the functions returning standard error codes
>> are of type s32. Convert them to regular ints as typdefs here are
>> not necessary to return standard error codes.
>>
>> Suggested-by: Jacob Keller
>> Reviewed-by: Jacob Keller
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> v2: fix smatch warnings,
>
>These changes aren't mentioned in the commit message, however, it should
>probably be in it's own commit to keep the changes separate and easier
>to see/review (since they pre-existed this patch).
OK, i agree it's better to create a separate commit for that
>
>Also, not seeing changes/fixes to the second warning [1]
>
>New smatch warnings:
>...
>drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c:3130
>ixgbe_enter_lplu_t_x550em() warn: missing error code? 'status'
>
>> alter the commit msg
>> ---
>
>...
>
>> @@ -93,11 +93,11 @@ static s32 ixgbe_get_invariants_82598(struct ixgbe_hw
>> *hw)
>>* not known. Perform the SFP init if necessary.
>>*
>>**/
>> -static s32 ixgbe_init_phy_ops_82598(struct ixgbe_hw *hw)
>> +static int ixgbe_init_phy_ops_82598(struct ixgbe_hw *hw)
>> {
>> struct ixgbe_mac_info *mac = &hw->mac;
>> struct ixgbe_phy_info *phy = &hw->phy;
>> -s32 ret_val;
>> +int ret_val;
>> u16 list_offset, data_offset;
>
>Should we RCT what we're touching? (many other instances in the patch as
>well)
I am not sure if that's still in the scope of that patch
>
>...
>
>> @@ -1866,9 +1866,9 @@ static s32 ixgbe_enable_rx_dma_82599(struct ixgbe_hw
>> *hw, u32 regval)
>>* Returns IXGBE_ERR_EEPROM_VERSION if the FW is not present or
>>* if the FW version is not supported.
>>**/
>> -static s32 ixgbe_verify_fw_version_82599(struct ixgbe_hw *hw)
>> +static int ixgbe_verify_fw_version_82599(struct ixgbe_hw *hw)
>> {
>> -s32 status = IXGBE_ERR_EEPROM_VERSION;
>> +int status = IXGBE_ERR_EEPROM_VERSION;
>
>Does IXGBE_ERR_EEPROM_VERSION exist anymore after your other patch? [2]
Sure, need to rebase it
>
>> u16 fw_offset, fw_ptp_cfg_offset;
>> u16 offset;
>> u16 fw_version = 0;
>
>...
>
>> @@ -2062,12 +2062,12 @@ static s32 ixgbe_reset_pipeline_82599(struct
>> ixgbe_hw *hw)
>>* Performs byte read operation to SFP module's EEPROM over I2C interface
>> at
>>* a specified device address.
>>**/
>> -static s32 ixgbe_read_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>> +static int ixgbe_read_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>> u8 dev_addr, u8 *data)
>> {
>> u32 esdp;
>> -s32 status;
>> -s32 timeout = 200;
>> +int status;
>> +int timeout = 200;
>
>unrelated change (at least a few others in this patch as well)
Thank you for pointing this out, need to take a look on that once again
>
>>
>> if (hw->phy.qsfp_shared_i2c_bus == true) {
>> /* Acquire I2C bus ownership. */
>> @@ -2116,12 +2116,12 @@ static s32 ixgbe_read_i2c_byte_82599(struct ixgbe_hw
>> *hw, u8 byte_offset,
>>* Performs byte write operation to SFP module's EEPROM over I2C
>> interface at
>>* a specified device address.
>>**/
>> -static s32 ixgbe_write_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>> +static int ixgbe_write_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>>u8 dev_addr, u8 data)
>> {
>> u32 esdp;
>> -s32 status;
>> -s32 timeout = 200;
>> +int status;
>> +int timeout = 200;
>>
>> if (hw->phy.qsfp_shared_i2c_bus == true) {
>> /* Acquire I2C bus ownership. */
>
>Thanks,
>Tony
>
>[1]
>https://lore.kernel.org/intel-wired-lan/08d8b75e-af80-438b-8006-9121b8444f49@moroto.mountain/
>[2]
>https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5795f533f30a80aa0473652876296ebc9129e33a
Re: [Intel-wired-lan] [PATCH iwl-next v1 2/2] i40e-linux: Add support for reading Trace Buffer
From: Jiri Pirko
Sent: Friday, January 12, 2024 1:49 PM
>Fri, Jan 12, 2024 at 10:59:45AM CET, jedrzej.jagiel...@intel.com wrote:
>>Currently after entering FW Recovery Mode we have no info in logs
>>regarding current FW state.
>>
>>Add function reading content of the alternate RAM storing that info and
>>print it into the log. Additionally print state of CSR register.
>>
>>Reviewed-by: Jan Sokolowski
>>Signed-off-by: Jedrzej Jagielski
>>---
>> drivers/net/ethernet/intel/i40e/i40e.h| 2 ++
>> drivers/net/ethernet/intel/i40e/i40e_main.c | 35 +++
>> .../net/ethernet/intel/i40e/i40e_register.h | 2 ++
>> drivers/net/ethernet/intel/i40e/i40e_type.h | 5 +++
>> 4 files changed, 44 insertions(+)
>>
>>diff --git a/drivers/net/ethernet/intel/i40e/i40e.h
>>b/drivers/net/ethernet/intel/i40e/i40e.h
>>index ba24f3fa92c3..6ebd2fd15e0e 100644
>>--- a/drivers/net/ethernet/intel/i40e/i40e.h
>>+++ b/drivers/net/ethernet/intel/i40e/i40e.h
>>@@ -23,6 +23,8 @@
>> /* Useful i40e defaults */
>> #define I40E_MAX_VEB 16
>>
>>+#define I40_BYTES_PER_WORD 2
>>+
>> #define I40E_MAX_NUM_DESCRIPTORS 4096
>> #define I40E_MAX_NUM_DESCRIPTORS_XL710 8160
>> #define I40E_MAX_CSR_SPACE (4 * 1024 * 1024 - 64 * 1024)
>>diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>index 4977ff391fed..f5abe8c9a88d 100644
>>--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>@@ -15414,6 +15414,39 @@ static int i40e_handle_resets(struct i40e_pf *pf)
>> return is_empr ? -EIO : pfr;
>> }
>>
>>+/**
>>+ * i40e_log_fw_recovery_mode - log current FW state in Recovery Mode
>>+ * @pf: board private structure
>>+ *
>>+ * Read alternate RAM and CSR registers and print them to the log
>>+ **/
>>+static void i40e_log_fw_recovery_mode(struct i40e_pf *pf)
>>+{
>>+ u8 buf[I40E_FW_STATE_BUFF_SIZE] = {0};
>>+ struct i40e_hw *hw = &pf->hw;
>>+ u8 fws0b, fws1b;
>>+ u32 fwsts;
>>+ int ret;
>>+
>>+ ret = i40e_aq_alternate_read_indirect(hw, I40E_ALT_CANARY,
>>+ I40E_ALT_BUFF_DWORD_SIZE, buf);
>>+ if (ret) {
>>+ dev_warn(&pf->pdev->dev,
>>+ "Cannot get FW trace buffer due to FW err %d aq_err
>>%s\n",
>>+ ret, i40e_aq_str(hw, hw->aq.asq_last_status));
>>+ return;
>>+ }
>>+
>>+ fwsts = rd32(&pf->hw, I40E_GL_FWSTS);
>>+ fws0b = FIELD_GET(I40E_GL_FWSTS_FWS0B_MASK, fwsts);
>>+ fws1b = FIELD_GET(I40E_GL_FWSTS_FWS1B_MASK, fwsts);
>>+
>>+ print_hex_dump(KERN_DEBUG, "Trace Buffer: ", DUMP_PREFIX_NONE,
>>+BITS_PER_BYTE * I40_BYTES_PER_WORD, 1, buf,
>>+I40E_FW_STATE_BUFF_SIZE, true);
>
>I don't follow. Why exactly you want to pollute dmesg with another
>messages? Can't you use some other interface? Devlink health reporter
>looks like a suitable alternative for this kind of operations.
There is no devlink support for the i40e driver at this point.
Dumping log in that case happen rather occasionally and debug log lvl is used
so this should mitigate polluting the dmesg.
>
>
>
>>+ dev_dbg(&pf->pdev->dev, "FWS0B=0x%x, FWS1B=0x%x\n", fws0b, fws1b);
>>+}
>>+
>> /**
>> * i40e_init_recovery_mode - initialize subsystems needed in recovery mode
>> * @pf: board private structure
>>@@ -15497,6 +15530,8 @@ static int i40e_init_recovery_mode(struct i40e_pf
>>*pf, struct i40e_hw *hw)
>> mod_timer(&pf->service_timer,
>>round_jiffies(jiffies + pf->service_timer_period));
>>
>>+ i40e_log_fw_recovery_mode(pf);
>>+
>> return 0;
>>
>> err_switch_setup:
>>diff --git a/drivers/net/ethernet/intel/i40e/i40e_register.h
>>b/drivers/net/ethernet/intel/i40e/i40e_register.h
>>index 14ab642cafdb..8e254ff9c035 100644
>>--- a/drivers/net/ethernet/intel/i40e/i40e_register.h
>>+++ b/drivers/net/ethernet/intel/i40e/i40e_register.h
>>@@ -169,6 +169,8 @@
>> #define I40E_PRTDCB_TPFCTS_PFCTIMER_SHIFT 0
>> #define I40E_PRTDCB_TPFCTS_PFCTIMER_MASK I40E_MASK(0x3FFF,
>> I40E_PRTDCB_TPFCTS_PFCTIMER_SHIFT)
>> #define I40E_GL_FWSTS 0x00083048 /* Reset: POR */
>>+#define I40E_GL_FWSTS_FWS0B_SHIFT 0
>>+#define I40E_GL_FWSTS_FWS0B_MASK I40E_MASK(0xFF, I40E_GL_FWSTS_FWS0B_SHIFT)
>> #define I40E_GL_FWSTS_FWS1B_SHIFT 16
>> #define I40E_GL_FWSTS_FWS1B_MASK I40E_MASK(0xFF, I40E_GL_FWSTS_FWS1B_SHIFT)
>> #define I40E_GL_FWSTS_FWS1B_EMPR_0 I40E_MASK(0x20, I40E_GL_FWSTS_FWS1B_SHIFT)
>>diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h
>>b/drivers/net/ethernet/intel/i40e/i40e_type.h
>>index 725da7edbca3..0372a8d519ad 100644
>>--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
>>+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
>>@@ -1372,6 +1372,11 @@ struct i40e_lldp_variables {
>> #define I40E_ALT_BW_VALUE_MASK 0xFF
>> #define I40E_ALT_BW_VALID_MASK 0x8000
>>
>>+/* Alternate Ram Tr
Re: [Intel-wired-lan] [PATCH iwl-next v3 3/3] ixgbe: Cleanup after type convertion
From: Kitszel, Przemyslaw Sent: Thursday, January 18, 2024 3:34 PM >On 1/18/24 14:43, Jedrzej Jagielski wrote: >> Clean up code where touched during type convertion by the patch >> 8035560dbfaf. Rearrange to fix reverse Christmas tree >I don't see this SHA in my copy, please fix it. I see this as the SHA of the patch 1/3 'ixgbe: Convert ret val type from s32 to int' What is the SHA of this patch in your copy then? > >> >> Suggested-by: Tony Nguyen >> Signed-off-by: Jedrzej Jagielski >> --- >> .../net/ethernet/intel/ixgbe/ixgbe_82598.c| 14 ++-- >> .../net/ethernet/intel/ixgbe/ixgbe_82599.c| 40 +-- >> .../net/ethernet/intel/ixgbe/ixgbe_common.c | 66 +-- >> .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 2 +- >> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 54 +++ >> drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 12 ++-- >> drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 50 +++--- >> 7 files changed, 119 insertions(+), 119 deletions(-) >> > >code changes are fine, >Reviewed-by: Przemek Kitszel
Re: [Intel-wired-lan] [PATCH iwl-next v3 1/3] ixgbe: Convert ret val type from s32 to int
From: Nguyen, Anthony L Sent: Monday, January 22, 2024 10:19 PM >On 1/18/2024 5:43 AM, Jedrzej Jagielski wrote: >> Currently big amount of the functions returning standard error codes >> are of type s32. Convert them to regular ints as typdefs here are not >> necessary to return standard error codes. >> >> Suggested-by: Jacob Keller >> Reviewed-by: Jacob Keller >> Signed-off-by: Jedrzej Jagielski > >There's various checkpatch issues being reported: > >CHECK: Alignment should match open parenthesis >ERROR: space prohibited before that ',' (ctx:WxW) >WARNING: please, no space before tabs > >Seems like a number of these can be remedied. Can they be addressed in the separate, already existing clean up patch. Or obligatorily they must be remedied in this patch? Thanks > >Thanks, >Tony
Re: [Intel-wired-lan] [PATCH iwl-next v1 1/2] ixgbe: Refactor overtemp event handling
From: Nguyen, Anthony L Sent: Friday, November 17, 2023 12:36 AM >On 11/14/2023 1:10 AM, Jedrzej Jagielski wrote: >> Currently ixgbe driver is notified of overheating events >> via internal IXGBE_ERR_OVERTEMP erorr code. >> >> Change the approach to use is_overhaet variable >> which set when such event occurs. > >I'll likely have more questions/comments, but an initial question. Is >this variable intended to hold the state of the PHY as overheated or is >this just communicating at the time of the check that it's overheated? My intention was to introduce this variable in order to inform the calling function, not to hold the state, that's why it's cleared right after overtemp detected. So basically it is IXGBE_ERR_OVERTEMPworkaround. Actually i could add dedicated parameter to phy.ops.check_overtemp() and phy.ops.handle_lasi() but I choose not to interfere with their declarations and decided to add this variable. If you have any advices please share. > >> Reviewed-by: Przemek Kitszel >> Signed-off-by: Jedrzej Jagielski >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 12 +++- >> drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c | 7 +-- >> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 2 +- >> drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 19 +-- >> 4 files changed, 22 insertions(+), 18 deletions(-) ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-next v1 01/13] ixgbe: add initial devlink support
From: Simon Horman Sent: Friday, February 7, 2025 11:47 AM >On Mon, Feb 03, 2025 at 04:03:16PM +0100, Jedrzej Jagielski wrote: >> Add an initial support for devlink interface to ixgbe driver. >> >> Similarly to i40e driver the implementation doesn't enable >> devlink to manage device-wide configuration. Devlink instance >> is created for each physical function of PCIe device. >> >> Create separate directory for devlink related ixgbe files >> and use naming scheme similar to the one used in the ice driver. >> >> Add a stub for Documentation, to be extended by further patches. >> >> Reviewed-by: Mateusz Polchlopek >> Signed-off-by: Jedrzej Jagielski > >... > >> diff --git a/Documentation/networking/devlink/ixgbe.rst >> b/Documentation/networking/devlink/ixgbe.rst >> new file mode 100644 >> index ..ca920d421d42 >> --- /dev/null >> +++ b/Documentation/networking/devlink/ixgbe.rst >> @@ -0,0 +1,8 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> + >> + >> +ixgbe devlink support >> + > >nit: the '=' lines are one character too short wrt the text they decorate. > >Flagged by make htmldocs. Thanks for catching it. Will be fixed. > >> + >> +This document describes the devlink features implemented by the ``ixgbe`` >> +device driver. > >...
Re: [Intel-wired-lan] [PATCH iwl-next v1 02/13] ixgbe: add handler for devlink .info_get()
From: Simon Horman
Sent: Sunday, February 9, 2025 5:27 PM
>On Mon, Feb 03, 2025 at 04:03:17PM +0100, Jedrzej Jagielski wrote:
>> Provide devlink .info_get() callback implementation to allow the
>> driver to report detailed version information. The following info
>> is reported:
>>
>> "serial_number" -> The PCI DSN of the adapter
>> "fw.bundle_id" -> Unique identifier for the combined flash image
>> "fw.undi" -> Version of the Option ROM containing the UEFI driver
>> "board.id" -> The PBA ID string
>>
>> Reviewed-by: Mateusz Polchlopek
>> Signed-off-by: Jedrzej Jagielski
>
>...
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>> b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>
>...
>
>> +static void ixgbe_info_nvm_ver(struct ixgbe_adapter *adapter,
>> + struct ixgbe_info_ctx *ctx)
>> +{
>> +struct ixgbe_hw *hw = &adapter->hw;
>> +struct ixgbe_nvm_version nvm_ver;
>> +
>> +ixgbe_get_oem_prod_version(hw, &nvm_ver);
>> +if (nvm_ver.oem_valid) {
>> +snprintf(ctx->buf, sizeof(ctx->buf), "%x.%x.%x",
>> + nvm_ver.oem_major, nvm_ver.oem_minor,
>> + nvm_ver.oem_release);
>> +
>> +return;
>> +}
>> +
>> +ixgbe_get_orom_version(hw, &nvm_ver);
>> +if (nvm_ver.or_valid)
>> +snprintf(ctx->buf, sizeof(ctx->buf), "%d.%d.%d",
>> + nvm_ver.or_major, nvm_ver.or_build, nvm_ver.or_patch);
>
>Hi Jedrzej,
>
>If neither of the conditions above are met then it seems that ctx->buf will
>contain whatever string was present when the function was called. Is
>something like the following needed here?
>
> ctx->buf[0] = '\0';
Hi Simon,
thanks for suggestion, it's definitely worth do be incorporated in the next
revision.
Thanks!
>
>> +}
>> +
>> +static void ixgbe_info_eetrack(struct ixgbe_adapter *adapter,
>> + struct ixgbe_info_ctx *ctx)
>> +{
>> +struct ixgbe_hw *hw = &adapter->hw;
>> +struct ixgbe_nvm_version nvm_ver;
>> +
>> +ixgbe_get_oem_prod_version(hw, &nvm_ver);
>> +/* No ETRACK version for OEM */
>> +if (nvm_ver.oem_valid)
>> +return;
>
>Likewise, here.
>
>> +
>> +ixgbe_get_etk_id(hw, &nvm_ver);
>> +snprintf(ctx->buf, sizeof(ctx->buf), "0x%08x", nvm_ver.etk_id);
>> +}
>> +
>> +static int ixgbe_devlink_info_get(struct devlink *devlink,
>> + struct devlink_info_req *req,
>> + struct netlink_ext_ack *extack)
>> +{
>> +struct ixgbe_devlink_priv *devlink_private = devlink_priv(devlink);
>> +struct ixgbe_adapter *adapter = devlink_private->adapter;
>> +struct ixgbe_hw *hw = &adapter->hw;
>> +struct ixgbe_info_ctx *ctx;
>> +int err;
>> +
>> +ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
>> +if (!ctx)
>> +return -ENOMEM;
>> +
>> +ixgbe_info_get_dsn(adapter, ctx);
>> +err = devlink_info_serial_number_put(req, ctx->buf);
>> +if (err)
>> +goto free_ctx;
>> +
>> +ixgbe_info_nvm_ver(adapter, ctx);
>> +err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>> + DEVLINK_INFO_VERSION_GENERIC_FW_UNDI,
>> + ctx->buf);
>> +if (err)
>> +goto free_ctx;
>> +
>> +ixgbe_info_eetrack(adapter, ctx);
>> +err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>> + DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID,
>> + ctx->buf);
>> +if (err)
>> +goto free_ctx;
>> +
>> +err = ixgbe_read_pba_string_generic(hw, ctx->buf, sizeof(ctx->buf));
>> +if (err)
>> +goto free_ctx;
>> +
>> +err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_FIXED,
>> + DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,
>> + ctx->buf);
>> +free_ctx:
>> +kfree(ctx);
>> +return err;
>> +}
>
>...
Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: add support for thermal sensor event reception
From: Paul Menzel
Sent: Monday, February 10, 2025 12:40 PM
>Dear Jedrzej,
>
>
>Thank you for your patch.
>
>Am 10.02.25 um 11:40 schrieb Jedrzej Jagielski:
>> E610 NICs unlike the previous devices utilising ixgbe driver
>> are notified in the case of overheatning by the FW ACI event.
>
>overheating (without n)
>
>> In event of overheat when threshold is exceeded, FW suspends all
>> traffic and sends overtemp event to the driver.
>
>I guess this was already the behavior before your patch, and now it’s
>only logged, and the device stopped properly?
Hi Paul,
Without this patch driver cannot receive the overtemp info. FW handles
the event - device stops but user has no clue what has happened.
FW does the major work but this patch adds this minimal piece of
overtemp handling done by SW - informing user at the very end.
>
>> Then driver
>> logs appropriate message and closes the adapter instance.
>> The card remains in that state until the platform is rebooted.
>
>As a user I’d be interested what the threshold is, and what the measured
>temperature is. Currently, the log seems to be just generic?
These details are FW internals.
Driver just gets info that such event has happened.
There's no additional information.
In that case driver's job is just to inform user that such scenario
has happened and tell what should be the next steps.
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:static const char
>ixgbe_overheat_msg[] = "Network adapter has been stopped because it has
>over heated. Restart the computer. If the problem persists, power off
>the system and replace the adapter";
>
>> Reviewed-by: Przemek Kitszel
>> Reviewed-by: Mateusz Polchlopek
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> v2,3 : commit msg tweaks
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h | 3 +++
>> 2 files changed, 8 insertions(+)
>
>
>Kind regards,
>
>Paul
>
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 7236f20c9a30..5c804948dd1f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3165,6 +3165,7 @@ static void ixgbe_aci_event_cleanup(struct
>> ixgbe_aci_event *event)
>> static void ixgbe_handle_fw_event(struct ixgbe_adapter *adapter)
>> {
>> struct ixgbe_aci_event event __cleanup(ixgbe_aci_event_cleanup);
>> +struct net_device *netdev = adapter->netdev;
>> struct ixgbe_hw *hw = &adapter->hw;
>> bool pending = false;
>> int err;
>> @@ -3185,6 +3186,10 @@ static void ixgbe_handle_fw_event(struct
>> ixgbe_adapter *adapter)
>> case ixgbe_aci_opc_get_link_status:
>> ixgbe_handle_link_status_event(adapter, &event);
>> break;
>> +case ixgbe_aci_opc_temp_tca_event:
>> +e_crit(drv, "%s\n", ixgbe_overheat_msg);
>> +ixgbe_close(netdev);
>> +break;
>> default:
>> e_warn(hw, "unknown FW async event captured\n");
>> break;
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> index 8d06ade3c7cd..617e07878e4f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> @@ -171,6 +171,9 @@ enum ixgbe_aci_opc {
>> ixgbe_aci_opc_done_alt_write= 0x0904,
>> ixgbe_aci_opc_clear_port_alt_write = 0x0906,
>>
>> +/* TCA Events */
>> +ixgbe_aci_opc_temp_tca_event= 0x0C94,
>> +
>> /* debug commands */
>> ixgbe_aci_opc_debug_dump_internals = 0xFF08,
>>
Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: add support for thermal sensor event reception
From: Paul Menzel
Sent: Monday, February 10, 2025 1:07 PM
>Dear Jedrzej,
>
>
>Thank you for the quick reply.
>
>
>Am 10.02.25 um 12:59 schrieb Jagielski, Jedrzej:
>> From: Paul Menzel
>> Sent: Monday, February 10, 2025 12:40 PM
>
>>> Am 10.02.25 um 11:40 schrieb Jedrzej Jagielski:
>>>> E610 NICs unlike the previous devices utilising ixgbe driver
>>>> are notified in the case of overheatning by the FW ACI event.
>>>
>>> overheating (without n)
>>>
>>>> In event of overheat when threshold is exceeded, FW suspends all
>>>> traffic and sends overtemp event to the driver.
>>>
>>> I guess this was already the behavior before your patch, and now it’s
>>> only logged, and the device stopped properly?
>
>> Without this patch driver cannot receive the overtemp info. FW handles
>> the event - device stops but user has no clue what has happened.
>> FW does the major work but this patch adds this minimal piece of
>> overtemp handling done by SW - informing user at the very end.
>
>Thank you for the confirmation. Maybe add a small note, that it’s not
>logged to make it crystal clear for people like myself.
OK, i will extend the commit msg.
>
>>>> Then driver
>>>> logs appropriate message and closes the adapter instance.
>>>> The card remains in that state until the platform is rebooted.
>>>
>>> As a user I’d be interested what the threshold is, and what the measured
>>> temperature is. Currently, the log seems to be just generic?
>>
>> These details are FW internals.
>> Driver just gets info that such event has happened.
>> There's no additional information.
>>
>> In that case driver's job is just to inform user that such scenario
>> has happened and tell what should be the next steps.
>
> From a user perspective that is a suboptimal behavior, and shows
>another time that the Linux kernel should have all the control, and
>stuff like this should be moved *out* of the firmware and not into the
>firmware.
>
>It’d be great if you could talk to the card designers/engineers to take
>that into account, and maybe revert this decision for future versions or
>future adapters.
I will have it on my mind.
Thanks,
Jedrek
>
>
>Kind regards,
>
>Paul
>
>
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:static const char
>>> ixgbe_overheat_msg[] = "Network adapter has been stopped because it has
>>> over heated. Restart the computer. If the problem persists, power off the
>>> system and replace the adapter";
>>>
>>>> Reviewed-by: Przemek Kitszel
>>>> Reviewed-by: Mateusz Polchlopek
>>>> Signed-off-by: Jedrzej Jagielski
>>>> ---
>>>> v2,3 : commit msg tweaks
>>>> ---
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +
>>>>drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h | 3 +++
>>>>2 files changed, 8 insertions(+)
>>>
>>>
>>> Kind regards,
>>>
>>> Paul
>>>
>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 7236f20c9a30..5c804948dd1f 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3165,6 +3165,7 @@ static void ixgbe_aci_event_cleanup(struct
>>>> ixgbe_aci_event *event)
>>>>static void ixgbe_handle_fw_event(struct ixgbe_adapter *adapter)
>>>>{
>>>>struct ixgbe_aci_event event __cleanup(ixgbe_aci_event_cleanup);
>>>> + struct net_device *netdev = adapter->netdev;
>>>>struct ixgbe_hw *hw = &adapter->hw;
>>>>bool pending = false;
>>>>int err;
>>>> @@ -3185,6 +3186,10 @@ static void ixgbe_handle_fw_event(struct
>>>> ixgbe_adapter *adapter)
>>>>case ixgbe_aci_opc_get_link_status:
>>>>ixgbe_handle_link_status_event(adapter, &event);
>>>>break;
>>>> + case ixgbe_aci_opc_temp_tca_event:
>>>> + e_crit(drv, "%s\n", ixgbe_overheat_msg);
>>>> + ixgbe_close(netdev);
>>>> + break;
>>>>default:
>>>>e_warn(hw, "unknown FW async event captured\n");
>>>>break;
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>>>> index 8d06ade3c7cd..617e07878e4f 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>>>> @@ -171,6 +171,9 @@ enum ixgbe_aci_opc {
>>>>ixgbe_aci_opc_done_alt_write= 0x0904,
>>>>ixgbe_aci_opc_clear_port_alt_write = 0x0906,
>>>>
>>>> + /* TCA Events */
>>>> + ixgbe_aci_opc_temp_tca_event= 0x0C94,
>>>> +
>>>>/* debug commands */
>>>>ixgbe_aci_opc_debug_dump_internals = 0xFF08,
Re: [Intel-wired-lan] [PATCH iwl-next v2] ixgbe: add support for thermal sensor event reception
From: Andrew Lunn Sent: Tuesday, February 4, 2025 2:09 PM >On Tue, Feb 04, 2025 at 08:17:00AM +0100, Jedrzej Jagielski wrote: >> E610 NICs unlike the previous devices utilising ixgbe driver >> are notified in the case of overheatning by the FW ACI event. >> >> In event of overheat when treshold is exceeded, FW suspends all >> traffic and sends overtemp event to the driver. Then driver >> logs appropriate message and closes the adapter instance. >> The card remains in that state until the platform is rebooted. > >There is also an HWMON temp[1-*]_emergency_alarm you can set. I >_think_ that should also cause a udev event, so user space knows the >print^h^h^h^h^hnetwork is on fire. > > Andrew I am not sure whether HWMON is applicable in that case. Driver receives an async notification from the FW that an overheating occurred, so has to handle it. In that case - by printing msg and making the interface disabled for the user. FW is responsible for monitoring temperature itself. There's even no possibility to read temperature by the driver Thanks Jedrek
Re: [Intel-wired-lan] [PATCH iwl-next v2] ixgbe: add support for thermal sensor event reception
From: Andrew Lunn Sent: Thursday, February 6, 2025 2:59 PM >On Thu, Feb 06, 2025 at 01:05:27PM +0000, Jagielski, Jedrzej wrote: >> From: Andrew Lunn >> Sent: Tuesday, February 4, 2025 2:09 PM >> >On Tue, Feb 04, 2025 at 08:17:00AM +0100, Jedrzej Jagielski wrote: >> >> E610 NICs unlike the previous devices utilising ixgbe driver >> >> are notified in the case of overheatning by the FW ACI event. >> >> >> >> In event of overheat when treshold is exceeded, FW suspends all >> >> traffic and sends overtemp event to the driver. Then driver >> >> logs appropriate message and closes the adapter instance. >> >> The card remains in that state until the platform is rebooted. >> > >> >There is also an HWMON temp[1-*]_emergency_alarm you can set. I >> >_think_ that should also cause a udev event, so user space knows the >> >print^h^h^h^h^hnetwork is on fire. >> > >> >Andrew >> >> I am not sure whether HWMON is applicable in that case. >> Driver receives an async notification from the FW that an overheating >> occurred, so has to handle it. In that case - by printing msg >> and making the interface disabled for the user. >> FW is responsible for monitoring temperature itself. >> There's even no possibility to read temperature by the driver > >https://elixir.bootlin.com/linux/v6.13.1/source/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c#L27 > >ixgbe_hwmon_show_temp() is some other temperature sensor? Which you do >have HWMON support for? This feature is not supported for E610 which has no support for reading temperature hw->mac.ops.get_thermal_sensor_data() callback used in ixgbe_hwmon_show_temp has no implementation for E610, as there is no such support from the FW side > >Or is the E610 not really an ixgbe, it has a different architecture, ixgbe is used by several adapters, each is slightly different in this case monitoring stuff is pushed into FW >more stuff pushed into firmware, less visibility from the kernel, no >temperature monitoring, just a NIC on fire indication? yeah, right Jedrek
Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: add support for thermal sensor event reception
From: Andrew Lunn Sent: Monday, February 10, 2025 2:37 PM >> > > > Then driver >> > > > logs appropriate message and closes the adapter instance. >> > > > The card remains in that state until the platform is rebooted. >> > > >> > > As a user I’d be interested what the threshold is, and what the measured >> > > temperature is. Currently, the log seems to be just generic? >> > >> > These details are FW internals. >> > Driver just gets info that such event has happened. >> > There's no additional information. >> > >> > In that case driver's job is just to inform user that such scenario >> > has happened and tell what should be the next steps. >> >> From a user perspective that is a suboptimal behavior, and shows another >> time that the Linux kernel should have all the control, and stuff like this >> should be moved *out* of the firmware and not into the firmware. > >The older generations of hardware driven by this driver actually have >HWMON support for the temperature sensor. I can understand the >hardware protecting itself, and shutting down, but i agree with you, >all the infrastructure already exists to report the temperature so why >drop it? That actually seems like more work, and makes the device less >friendly. Actually there is only one adapter across all portfolio of ixgbe adapters which supports this feature. That is 82599, none other supports it. Even next generations (x540, x550) didn't provide support for reading thermal data sensor. As E610 is some type of extending x550 it also follows this path at this point. Thanks Jedrek
Re: [Intel-wired-lan] [PATCH iwl-next v2 02/13] ixgbe: add handler for devlink .info_get()
From: Jiri Pirko
Sent: Monday, February 10, 2025 5:26 PM
>Mon, Feb 10, 2025 at 02:56:28PM +0100, jedrzej.jagiel...@intel.com wrote:
>
>[...]
>
>>+enum ixgbe_devlink_version_type {
>>+ IXGBE_DL_VERSION_FIXED,
>>+ IXGBE_DL_VERSION_RUNNING,
>>+};
>>+
>>+static int ixgbe_devlink_info_put(struct devlink_info_req *req,
>>+ enum ixgbe_devlink_version_type type,
>>+ const char *key, const char *value)
>
>I may be missing something, but what's the benefit of having this helper
>instead of calling directly devlink_info_version_*_put()?
ixgbe devlink .info_get() supports various adapters across ixgbe portfolio which
have various sets of version types - some version types are not applicable
for some of the adapters - so we want just to check if it's *not empty.*
If so then we don't want to create such entry at all so avoid calling
devlink_info_version_*_put() in this case.
Putting value check prior each calling of devlink_info_version_*_put()
would provide quite a code redundancy and would look not so good imho.
Me and Przemek are not fully convinced by adding such additional
layer of abstraction but we defineltly need this value check to not
print empty type or get error and return from the function.
Another solution would be to add such check to devlink function.
>
>
>
>>+{
>>+ if (!*value)
>>+ return 0;
>>+
>>+ switch (type) {
>>+ case IXGBE_DL_VERSION_FIXED:
>>+ return devlink_info_version_fixed_put(req, key, value);
>>+ case IXGBE_DL_VERSION_RUNNING:
>>+ return devlink_info_version_running_put(req, key, value);
>>+ }
>>+
>>+ return 0;
>>+}
>>+
>
>[...]
>
>
>>+static int ixgbe_devlink_info_get(struct devlink *devlink,
>>+ struct devlink_info_req *req,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ struct ixgbe_devlink_priv *devlink_private = devlink_priv(devlink);
>>+ struct ixgbe_adapter *adapter = devlink_private->adapter;
>>+ struct ixgbe_hw *hw = &adapter->hw;
>>+ struct ixgbe_info_ctx *ctx;
>>+ int err;
>>+
>>+ ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
>>+ if (!ctx)
>>+ return -ENOMEM;
>>+
>>+ ixgbe_info_get_dsn(adapter, ctx);
>>+ err = devlink_info_serial_number_put(req, ctx->buf);
>>+ if (err)
>>+ goto free_ctx;
>>+
>>+ ixgbe_info_nvm_ver(adapter, ctx);
>>+ err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>>+ DEVLINK_INFO_VERSION_GENERIC_FW_UNDI,
>>+ ctx->buf);
>>+ if (err)
>>+ goto free_ctx;
>>+
>>+ ixgbe_info_eetrack(adapter, ctx);
>>+ err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_RUNNING,
>>+ DEVLINK_INFO_VERSION_GENERIC_FW_BUNDLE_ID,
>>+ ctx->buf);
>>+ if (err)
>>+ goto free_ctx;
>>+
>>+ err = ixgbe_read_pba_string_generic(hw, ctx->buf, sizeof(ctx->buf));
>>+ if (err)
>>+ goto free_ctx;
>>+
>>+ err = ixgbe_devlink_info_put(req, IXGBE_DL_VERSION_FIXED,
>>+ DEVLINK_INFO_VERSION_GENERIC_BOARD_ID,
>>+ ctx->buf);
>>+free_ctx:
>>+ kfree(ctx);
>>+ return err;
>>+}
>>+
>> static const struct devlink_ops ixgbe_devlink_ops = {
>>+ .info_get = ixgbe_devlink_info_get,
>> };
>>
>> /**
>>--
>>2.31.1
>>
>>
Re: [Intel-wired-lan] [PATCH iwl-next v3 02/14] ixgbe: add initial devlink support
From: Jiri Pirko
Sent: Wednesday, February 12, 2025 4:09 PM
>Wed, Feb 12, 2025 at 02:14:01PM +0100, jedrzej.jagiel...@intel.com wrote:
>>Add an initial support for devlink interface to ixgbe driver.
>>
>>Similarly to i40e driver the implementation doesn't enable
>>devlink to manage device-wide configuration. Devlink instance
>>is created for each physical function of PCIe device.
>>
>>Create separate directory for devlink related ixgbe files
>>and use naming scheme similar to the one used in the ice driver.
>>
>>Add a stub for Documentation, to be extended by further patches.
>>
>>Reviewed-by: Mateusz Polchlopek
>>Signed-off-by: Jedrzej Jagielski
>>---
>>v2: fix error patch in probe; minor tweaks
>>---
>> Documentation/networking/devlink/index.rst| 1 +
>> Documentation/networking/devlink/ixgbe.rst| 8 ++
>> drivers/net/ethernet/intel/Kconfig| 1 +
>> drivers/net/ethernet/intel/ixgbe/Makefile | 3 +-
>> .../ethernet/intel/ixgbe/devlink/devlink.c| 80 +++
>> .../ethernet/intel/ixgbe/devlink/devlink.h| 10 +++
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 8 ++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 22 +
>> 8 files changed, 132 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/networking/devlink/ixgbe.rst
>> create mode 100644 drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>> create mode 100644 drivers/net/ethernet/intel/ixgbe/devlink/devlink.h
>>
>>diff --git a/Documentation/networking/devlink/index.rst
>>b/Documentation/networking/devlink/index.rst
>>index 948c8c44e233..8319f43b5933 100644
>>--- a/Documentation/networking/devlink/index.rst
>>+++ b/Documentation/networking/devlink/index.rst
>>@@ -84,6 +84,7 @@ parameters, info versions, and other features it supports.
>>i40e
>>ionic
>>ice
>>+ ixgbe
>>mlx4
>>mlx5
>>mlxsw
>>diff --git a/Documentation/networking/devlink/ixgbe.rst
>>b/Documentation/networking/devlink/ixgbe.rst
>>new file mode 100644
>>index ..c04ac51c6d85
>>--- /dev/null
>>+++ b/Documentation/networking/devlink/ixgbe.rst
>>@@ -0,0 +1,8 @@
>>+.. SPDX-License-Identifier: GPL-2.0
>>+
>>+=
>>+ixgbe devlink support
>>+=
>>+
>>+This document describes the devlink features implemented by the ``ixgbe``
>>+device driver.
>>diff --git a/drivers/net/ethernet/intel/Kconfig
>>b/drivers/net/ethernet/intel/Kconfig
>>index 1640d2f27833..56ee58c9df21 100644
>>--- a/drivers/net/ethernet/intel/Kconfig
>>+++ b/drivers/net/ethernet/intel/Kconfig
>>@@ -147,6 +147,7 @@ config IXGBE
>> depends on PCI
>> depends on PTP_1588_CLOCK_OPTIONAL
>> select MDIO
>>+ select NET_DEVLINK
>> select PHYLIB
>> help
>>This driver supports Intel(R) 10GbE PCI Express family of
>>diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile
>>b/drivers/net/ethernet/intel/ixgbe/Makefile
>>index b456d102655a..11f37140c0a3 100644
>>--- a/drivers/net/ethernet/intel/ixgbe/Makefile
>>+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
>>@@ -4,12 +4,13 @@
>> # Makefile for the Intel(R) 10GbE PCI Express ethernet driver
>> #
>>
>>+subdir-ccflags-y += -I$(src)
>> obj-$(CONFIG_IXGBE) += ixgbe.o
>>
>> ixgbe-y := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
>>ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
>>ixgbe_mbx.o ixgbe_x540.o ixgbe_x550.o ixgbe_lib.o ixgbe_ptp.o \
>>- ixgbe_xsk.o ixgbe_e610.o
>>+ ixgbe_xsk.o ixgbe_e610.o devlink/devlink.o
>>
>> ixgbe-$(CONFIG_IXGBE_DCB) += ixgbe_dcb.o ixgbe_dcb_82598.o \
>> ixgbe_dcb_82599.o ixgbe_dcb_nl.o
>>diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>>b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>>new file mode 100644
>>index ..c052e95c9496
>>--- /dev/null
>>+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
>>@@ -0,0 +1,80 @@
>>+// SPDX-License-Identifier: GPL-2.0
>>+/* Copyright (c) 2025, Intel Corporation. */
>>+
>>+#include "ixgbe.h"
>>+#include "devlink.h"
>>+
>>+static const struct devlink_ops ixgbe_devlink_ops = {
>>+};
>>+
>>+/**
>>+ * ixgbe_allocate_devlink - Allocate devlink instance
>>+ * @adapter: pointer to the device adapter structure
>>+ *
>>+ * Allocate a devlink instance for this physical function.
>>+ *
>>+ * Return: 0 on success, -ENOMEM when allocation failed.
>>+ */
>>+int ixgbe_allocate_devlink(struct ixgbe_adapter *adapter)
>>+{
>>+ struct ixgbe_devlink_priv *devlink_private;
>>+ struct device *dev = &adapter->pdev->dev;
>>+ struct devlink *devlink;
>>+
>>+ devlink = devlink_alloc(&ixgbe_devlink_ops,
>>+ sizeof(*devlink_private), dev);
>>+ if (!devlink)
>>+ return -ENOMEM;
>>+
>>+ devlink_private = devlink_priv(devlink);
>>+ devlink_private->adapter = adapter;
>
>struct ixgbe_adapter * should be returned by devlink_priv(), that is the
>idea, to let devlink allocate the driver private for you.
Re: [Intel-wired-lan] [PATCH iwl-next v1] ixgbe: add support for thermal sensor event reception
From: Paul Menzel
Sent: Wednesday, January 15, 2025 2:40 PM
>Dear Jedrzej,
>
>Thank you for the patch.
>
>Am 15.01.25 um 13:27 schrieb Jedrzej Jagielski:
>> E610 NICs unlike the previous devices utilising ixgbe driver
>> are notified in the case of overheat by the FW ACI event.
>
>overheat*ing*
>
>> In event of overheat when treshhold is exceeded, FW suspends all
>
>threshold
>
>> traffic and sends overtemp event to the driver. Then driver
>> loggs appropriate message and closes the adapter instance.
>
>logs
Hello Paul,
thank you for the review.
Next patch revision will incorporate all commit msg tweaks.
>
>> The card remains in that state until the platform is rebooted.
>
>How did you test this? Can you please paste the message?
This was tested by forcing the card (by the xternal source of heat)
to reach temperature exceeding the treshold.
>
>> Reviewed-by: Przemek Kitszel
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h | 3 +++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 7236f20c9a30..5c804948dd1f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -3165,6 +3165,7 @@ static void ixgbe_aci_event_cleanup(struct
>> ixgbe_aci_event *event)
>> static void ixgbe_handle_fw_event(struct ixgbe_adapter *adapter)
>> {
>> struct ixgbe_aci_event event __cleanup(ixgbe_aci_event_cleanup);
>> +struct net_device *netdev = adapter->netdev;
>> struct ixgbe_hw *hw = &adapter->hw;
>> bool pending = false;
>> int err;
>> @@ -3185,6 +3186,10 @@ static void ixgbe_handle_fw_event(struct
>> ixgbe_adapter *adapter)
>> case ixgbe_aci_opc_get_link_status:
>> ixgbe_handle_link_status_event(adapter, &event);
>> break;
>> +case ixgbe_aci_opc_temp_tca_event:
>> +e_crit(drv, "%s\n", ixgbe_overheat_msg);
>> +ixgbe_close(netdev);
>> +break;
>> default:
>> e_warn(hw, "unknown FW async event captured\n");
>> break;
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> index 8d06ade3c7cd..617e07878e4f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> @@ -171,6 +171,9 @@ enum ixgbe_aci_opc {
>> ixgbe_aci_opc_done_alt_write= 0x0904,
>> ixgbe_aci_opc_clear_port_alt_write = 0x0906,
>>
>> +/* TCA Events */
>> +ixgbe_aci_opc_temp_tca_event= 0x0C94,
>> +
>> /* debug commands */
>> ixgbe_aci_opc_debug_dump_internals = 0xFF08,
>
>Kind regards,
>
>Paul
Re: [Intel-wired-lan] [PATCH iwl-next v3] ixgbe: add support for thermal sensor event reception
From: Andrew Lunn Sent: Tuesday, February 11, 2025 3:17 PM >> Actually there is only one adapter across all portfolio of ixgbe adapters >> which supports this feature. That is 82599, none other supports it. >> Even next generations (x540, x550) didn't provide support for reading thermal >> data sensor. >> As E610 is some type of extending x550 it also follows this path at this >> point. > >It is something you should consider. The machine disappears off the >net for no obvious reason, and needs a cold boot to get it back? vs >HWMON entries you can monitor, a warning when the critical value is >reached, which probably reaches the network console and so gets logged >somewhere, and then the emergency value which shuts down the NIC >without any notification getting out of the box. > >Also, if there is temperature information, Linux can take an active >part in managing it. If the critical value is reached, it could >downshift to a lower link mode. Better to have a slower link than no >link and a cold boot. > Yeah, definitely But at this point there is no support from the device/FW itself, so the approach presented here is the solution to give at least any info to user, without device disappearing without any note. Once there will be possibility to get FW temp data this will be applied into already existing HWMON infrastructure. Thanks Jedrek
Re: [Intel-wired-lan] [PATCH iwl-next v3 03/14] ixgbe: add handler for devlink .info_get()
From: Jiri Pirko Sent: Wednesday, February 12, 2025 4:05 PM >Wed, Feb 12, 2025 at 02:14:02PM +0100, jedrzej.jagiel...@intel.com wrote: >>Provide devlink .info_get() callback implementation to allow the >>driver to report detailed version information. The following info >>is reported: >> >> "serial_number" -> The PCI DSN of the adapter >> "fw.bundle_id" -> Unique identifier for the combined flash image >> "fw.undi" -> Version of the Option ROM containing the UEFI driver >> "board.id" -> The PBA ID string > >Do you have some board.serial_number by any chance? This could be handy >in some cases, for example if you have a board with multiple nic asics. Unfortunately i cannot spot nothing more for identification than the PCI DSN number.
Re: [Intel-wired-lan] [PATCH iwl-next v2 1/4] ixgbe: create E610 specific ethtool_ops structure
From: Paul Menzel Sent: Friday, February 28, 2025 11:17 AM >Dear Jedrzej, > > >Thank you for your patch. > >Am 28.02.25 um 09:37 schrieb Jedrzej Jagielski: >> E610's implementation of various ethtool ops is different than >> the ones corresponding to ixgbe legacy products. Therefore create >> separate E610 ethtool_ops struct which will be filled out in the >> forthcoming patches. >> >> Assing adequate ops struct basing on mac type. This step requires > >1. A*dd*ing >2. Maybe even imperative mood: Add. >3. … based on MAC type. Hello Paul that's really unfortunate mistake, i don't know how i could pass it by thank you for pointing it out! :) Regards Jedrek > >> changing a bit the flow of probing by placing ixgbe_set_ethtool_ops >> after mac type is assigned. So move the whole netdev assignment >> block after mac_type is known. This step doesn't have any additional >> impact on probing sequence. >> >> Suggested-by: Aleksandr Loktionov >> Reviewed-by: Aleksandr Loktionov >> Signed-off-by: Jedrzej Jagielski >> ---
Re: [Intel-wired-lan] [PATCH iwl-next v5 12/15] ixgbe: add support for devlink reload
From: Simon Horman >Sent: Thursday, March 6, 2025 10:41 AM >To: Jagielski, Jedrzej >Cc: intel-wired-...@lists.osuosl.org; Nguyen, Anthony L >; net...@vger.kernel.org; j...@nvidia.com; >Polchlopek, Mateusz ; Mrozowicz, SlawomirX >; Kwapulinski, Piotr >; Wegrzyn, Stefan >Subject: Re: [PATCH iwl-next v5 12/15] ixgbe: add support for devlink reload > >On Fri, Feb 21, 2025 at 12:51:13PM +0100, Jedrzej Jagielski wrote: >> The E610 adapters contain an embedded chip with firmware which can be >> updated using devlink flash. The firmware which runs on this chip is >> referred to as the Embedded Management Processor firmware (EMP >> firmware). >> >> Activating the new firmware image currently requires that the system be >> rebooted. This is not ideal as rebooting the system can cause unwanted >> downtime. >> >> The EMP firmware itself can be reloaded by issuing a special update >> to the device called an Embedded Management Processor reset (EMP >> reset). This reset causes the device to reset and reload the EMP >> firmware. >> >> Implement support for devlink reload with the "fw_activate" flag. This >> allows user space to request the firmware be activated immediately. >> >> Reviewed-by: Mateusz Polchlopek >> Co-developed-by: Slawomir Mrozowicz >> Signed-off-by: Slawomir Mrozowicz >> Co-developed-by: Piotr Kwapulinski >> Signed-off-by: Piotr Kwapulinski >> Co-developed-by: Stefan Wegrzyn >> Signed-off-by: Stefan Wegrzyn >> Signed-off-by: Jedrzej Jagielski > >... > >> diff --git a/Documentation/networking/devlink/ixgbe.rst >> b/Documentation/networking/devlink/ixgbe.rst >> index 41aedf4b8017..e5fef951c6f5 100644 >> --- a/Documentation/networking/devlink/ixgbe.rst >> +++ b/Documentation/networking/devlink/ixgbe.rst >> @@ -88,3 +88,18 @@ combined flash image that contains the ``fw.mgmt``, >> ``fw.undi``, and >> and device serial number. It is expected that this combination be >> used with an >> image customized for the specific device. >> >> +Reload >> +== >> + >> +The ``ixgbe`` driver supports activating new firmware after a flash update >> +using ``DEVLINK_CMD_RELOAD`` with the ``DEVLINK_RELOAD_ACTION_FW_ACTIVATE`` >> +action. >> + >> +.. code:: shell >> +$ devlink dev reload pci/:01:00.0 reload action fw_activate >> +The new firmware is activated by issuing a device specific Embedded >> +Management Processor reset which requests the device to reset and reload the >> +EMP firmware image. >> + >> +The driver does not currently support reloading the driver via >> +``DEVLINK_RELOAD_ACTION_DRIVER_REINIT``. > >Hi Jedrzej, all, > >This is not a proper review. And I didn't look into this, but make htmldocs >complains that: > > .../ixgbe.rst:98: ERROR: Error in "code" directive: > maximum 1 argument(s) allowed, 9 supplied. > > .. code:: shell > $ devlink dev reload pci/:01:00.0 reload action fw_activate > >... Hi Simon looks like blank lines might be missing there. I will fix that in the next revision then.
Re: [Intel-wired-lan] [PATCH iwl-next v7 11/15] ixgbe: add device flash update via devlink
From: Simon Horman Sent: Wednesday, March 12, 2025 3:56 PM >On Wed, Mar 12, 2025 at 01:58:39PM +0100, Jedrzej Jagielski wrote: > >... > >> diff --git a/Documentation/networking/devlink/ixgbe.rst >> b/Documentation/networking/devlink/ixgbe.rst >> index a41073a62776..41aedf4b8017 100644 >> --- a/Documentation/networking/devlink/ixgbe.rst >> +++ b/Documentation/networking/devlink/ixgbe.rst >> @@ -64,3 +64,27 @@ The ``ixgbe`` driver reports the following versions >>- running >>- 0xee16ced7 >>- The first 4 bytes of the hash of the netlist module contents. >> + >> +Flash Update >> + >> +The ``ixgbe`` driver implements support for flash update using the >> +``devlink-flash`` interface. It supports updating the device flash using a >> +combined flash image that contains the ``fw.mgmt``, ``fw.undi``, and >> +``fw.netlist`` components. >> +.. list-table:: List of supported overwrite modes >> + :widths: 5 95 > >Hi Jedrzej, > >make htmldocs flags two warnings, which I believe can be resolved by >adding a blank line here. > > .../ixgbe.rst:75: ERROR: Unexpected indentation. > .../ixgbe.rst:76: WARNING: Field list ends without a blank line; unexpected > unindent. Hello Simon thanks for finding these. I will fix it and resend it later today probably. > >> + * - Bits >> + - Behavior >> + * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS`` >> + - Do not preserve settings stored in the flash components being >> + updated. This includes overwriting the port configuration that >> + determines the number of physical functions the device will >> + initialize with. >> + * - ``DEVLINK_FLASH_OVERWRITE_SETTINGS`` and >> ``DEVLINK_FLASH_OVERWRITE_IDENTIFIERS`` >> + - Do not preserve either settings or identifiers. Overwrite everything >> + in the flash with the contents from the provided image, without >> + performing any preservation. This includes overwriting device >> + identifying fields such as the MAC address, Vital product Data (VPD) >> area, >> + and device serial number. It is expected that this combination be >> used with an >> + image customized for the specific device. >> + > >...
Re: [Intel-wired-lan] [PATCH iwl-net v1] ixgbe: fix ixgbe_orom_civd_info struct layout
From: Simon Horman
Sent: Wednesday, July 30, 2025 5:54 PM
>On Wed, Jul 30, 2025 at 04:52:09PM +0200, Jedrzej Jagielski wrote:
>> The current layout of struct ixgbe_orom_civd_info causes incorrect data
>> storage due to compiler-inserted padding. This results in issues when
>> writing OROM data into the structure.
>>
>> Add the __packed attribute to ensure the structure layout matches the
>> expected binary format without padding.
>>
>> Fixes: 70db0788a262 ("ixgbe: read the OROM version information")
>> Reviewed-by: Aleksandr Loktionov
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> index 09df67f03cf4..38a41d81de0f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type_e610.h
>> @@ -1150,7 +1150,7 @@ struct ixgbe_orom_civd_info {
>> __le32 combo_ver; /* Combo Image Version number */
>> u8 combo_name_len; /* Length of the unicode combo image version
>> string, max of 32 */
>> __le16 combo_name[32]; /* Unicode string representing the Combo Image
>> version */
>> -};
>> +} __packed;
>
>...
>
>Hi Jedrzej,
>
>I agree that this is correct. Otherwise there will be a 3 byte hole before
>combo_ver and a 1 byte hole before and combo_name. Which, based on the
>commit message, I assume is not part of the layout this structure
>represents.
>
>A side effect of this change is that both combo_ver and elements of
>combo_name become unaligned. As elements combo_ver does not seem to be
>accessed directly, things seem clean there. But in the case of combo_ver,
>I wonder if this change is also needed. (Compile tested only!)
Hi Simon
it's definitely worth incorporating into the patch, due to risk of unaligmnent.
Thanks for your suggestion!
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>index 71ea25de1bac..754c176fd4a7 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>@@ -3123,7 +3123,7 @@ static int ixgbe_get_orom_ver_info(struct ixgbe_hw *hw,
> if (err)
> return err;
>
>- combo_ver = le32_to_cpu(civd.combo_ver);
>+ combo_ver = get_unaligned_le32(&civd.combo_ver);
>
> orom->major = (u8)FIELD_GET(IXGBE_OROM_VER_MASK, combo_ver);
> orom->patch = (u8)FIELD_GET(IXGBE_OROM_VER_PATCH_MASK, combo_ver);
>
Re: [Intel-wired-lan] [PATCH iwl-net v2 1/2] devlink: allow driver to freely name interfaces
From: Keller, Jacob E
Sent: Saturday, August 2, 2025 1:28 AM
>On 7/7/2025 1:58 AM, Jedrzej Jagielski wrote:
>> Currently when adding devlink port it is prohibited to let
>> a driver name an interface on its own. In some scenarios
>> it would not be preferable to provide such limitation,
>> eg some compatibility purposes.
>>
>> Add flag skip_phys_port_name_get to devlink_port_attrs struct
>> which indicates if devlink should not alter name of interface.
>>
>
>I feel like this somewhat lacks context in the commit message. The
>renaming is really the result of a policy provided by most distributions
>which uses the physical port name to determine its interface name, right?
>
>The fact that without the devlink support, this would be skipped, and
>adding devlink support made it change is not *exactly* the kernel's
>fault, but.. I think it does make sense to have the option for older
>drivers so they can add devlink support without triggering this
>behavioral change.
>
>While I might prefer more detail in the commit message, with or without
>that:
>Reviewed-by: Jacob Keller
Thanks for suggestion Jake! I will try to be more precise ;)
>
>
>> Suggested-by: Jiri Pirko
>> Signed-off-by: Jedrzej Jagielski
>> ---
>> v2: add skip_phys_port_name_get flag to skip changing if name
>> ---
>> include/net/devlink.h | 7 ++-
>> net/devlink/port.c| 3 +++
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 0091f23a40f7..414ae25de897 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -78,6 +78,7 @@ struct devlink_port_pci_sf_attrs {
>> * @flavour: flavour of the port
>> * @split: indicates if this is split port
>> * @splittable: indicates if the port can be split.
>> + * @skip_phys_port_name_get: if set devlink doesn't alter interface name
>> * @lanes: maximum number of lanes the port supports. 0 value is not passed
>> to netlink.
>> * @switch_id: if the port is part of switch, this is buffer with ID,
>> otherwise this is NULL
>> * @phys: physical port attributes
>> @@ -87,7 +88,11 @@ struct devlink_port_pci_sf_attrs {
>> */
>> struct devlink_port_attrs {
>> u8 split:1,
>> - splittable:1;
>> + splittable:1,
>> + skip_phys_port_name_get:1; /* This is for compatibility only,
>> + * newly added driver/port instance
>> + * should never set this.
>> + */
>> u32 lanes;
>> enum devlink_port_flavour flavour;
>> struct netdev_phys_item_id switch_id;
>> diff --git a/net/devlink/port.c b/net/devlink/port.c
>> index 939081a0e615..bf52c8a57992 100644
>> --- a/net/devlink/port.c
>> +++ b/net/devlink/port.c
>> @@ -1522,6 +1522,9 @@ static int __devlink_port_phys_port_name_get(struct
>> devlink_port *devlink_port,
>> if (!devlink_port->attrs_set)
>> return -EOPNOTSUPP;
>>
>> +if (devlink_port->attrs.skip_phys_port_name_get)
>> +return 0;
>> +
>> switch (attrs->flavour) {
>> case DEVLINK_PORT_FLAVOUR_PHYSICAL:
>> if (devlink_port->linecard)
