[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3a899036-be91-4ac3-1ced-f32df9ba9c13@intel.com>
Date: Mon, 11 Dec 2023 13:35:04 -0800
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next v3 1/2] ixgbe: Refactor overtemp event handling
On 12/11/2023 1:45 AM, Jagielski, Jedrzej wrote:
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
> 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 <jedrzej.jagielski@...el.com>
>>> ---
>>> 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
there's no need for it, we shouldn't have it... however, aren't you
using/checking it here?
@@ -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
Powered by blists - more mailing lists