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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB2731EFF4B5E7BDE8886EA913F08FA@DM6PR11MB2731.namprd11.prod.outlook.com>
Date: Mon, 11 Dec 2023 09:45:18 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH iwl-next v3 1/2] ixgbe: Refactor overtemp event handling

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?

>
>> +		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]