netdev - Re: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before link test

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] [day] [month] [year] [list]

Message-ID: <20250915062715.48528-1-enjuk@amazon.com>
Date: Mon, 15 Sep 2025 15:27:06 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <vitaly.lifshits@...el.com>
CC: <andrew+netdev@...n.ch>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <enjuk@...zon.com>,
	<intel-wired-lan@...ts.osuosl.org>, <kohei.enju@...il.com>,
	<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<przemyslaw.kitszel@...el.com>, <aleksandr.loktionov@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before link test

+ Alex

On Sun, 7 Sep 2025 16:51:53 +0300, Lifshits, Vitaly wrote:

>On 9/6/2025 6:49 AM, Kohei Enju wrote:
>> On Mon, 1 Sep 2025 07:36:21 +0300, Lifshits, Vitaly wrote:
>> 
>>>
>>>> ---
>>>>    drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>>> index f3e7218ba6f3..ca93629b1d3a 100644
>>>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>>> @@ -2094,6 +2094,9 @@ static void igc_ethtool_diag_test(struct net_device *netdev,
>>>>    		netdev_info(adapter->netdev, "Offline testing starting");
>>>>    		set_bit(__IGC_TESTING, &adapter->state);
>>>>    
>>>> +		/* power up PHY for link test */
>>>> +		igc_power_up_phy_copper(&adapter->hw);
>>>
>>> I suggest moving this to igc_link_test functionn igc_diags.c as it
>>> relates only to the link test.
>> 
>> Thank you for taking a look, Lifshits.
>> 
>> Now I'm considering changing only offline test path, not including
>> online test path.
>> This is because I think online test path should not trigger any
>> interruption and power down/up PHY may cause disruption.
>> 
>> So, I forgo the online path and my intention for this patch is to power
>> up PHY state only in offline test path.
>> I think introducing igc_power_up_phy_copper() in igc_link_test()
>> needs careful consideration in online test path.
>
>Yes, I see that now.
>Then it's ok to leave it as is.

Sorry for late response. Thank you for taking a look, Vitaly!

>
>Regarding the question whether to wrap igc_power_up_phy_copper with an 
>if (hw->phy.media_type == igc_media_type_copper), I think that it's not 
>necessary. First of all, igc devices are only copper devices. Secondly, 
>in the other place where we call this function (in igc_main), we don't 
>check the media type, therefore I suggest being consistent with the 
>already existing code and leaving it as it is now.

Indeed, I think you're right.

I looked at the existing code and found that indeed there are codes
which are assuming only copper devices at least for now. [1, 2, 3]

So, I'll rephrase the commit message in v2 to clarify:
- This change is applied only for offline testing, forgoing online
  testing.
- In this case, original power state is restored in igc_reset()
  afterwards.

and leave the diff as it is in V2 patch.

>
>> 
>>>
>>>> +
>>>>    		/* Link test performed before hardware reset so autoneg doesn't
>>>>    		 * interfere with test result
>>>>    		 */
>>>
>>>
>>> Thank you for this patch.

Thanks, 
Kohei

[1] igc_main.c
```
void igc_reset(struct igc_adapter *adapter)
{
  ...
  if (!netif_running(adapter->netdev))
      igc_power_down_phy_copper_base(&adapter->hw);
```

[2] igc_main.c
```
static void igc_power_up_link(struct igc_adapter *adapter)
{
  ...
  igc_power_up_phy_copper(&adapter->hw);
```

[3] igc_main.c
```
static int __igc_open(struct net_device *netdev, bool resuming)
{
  ...
  err_req_irq:
  igc_release_hw_control(adapter);
  igc_power_down_phy_copper_base(&adapter->hw);
```