netdev - Re: [PATCH iwl-next v3 1/2] ixgbe: Refactor overtemp event handling

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]

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