+ 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);
```
