[Intel-wired-lan] igc driver doesn't power down the PHY when link is brought down

[Intel-wired-lan] igc driver doesn't power down the PHY when link is brought down

[Lennert Buytenhek]

Hi,

When I run 'ip link set FOO down' on an I226 adapter, the link partner
keeps seeing link up, which is not what I would expect.  The igc driver
contains this code snippet that references this behavior (igc_phy.c):

> /**
>  * igc_power_down_phy_copper - Power down copper PHY
>  * @hw: pointer to the HW structure
>  *
>  * Power down PHY to save power when interface is down and wake on lan
>  * is not enabled.
>  */
> void igc_power_down_phy_copper(struct igc_hw *hw)
> {
>         u16 mii_reg = 0;
> 
>         /* The PHY will retain its settings across a power down/up cycle */
>         hw->phy.ops.read_reg(hw, PHY_CONTROL, &mii_reg);
>         mii_reg |= MII_CR_POWER_DOWN;
> 
>         /* Temporary workaround - should be removed when PHY will implement
>          * IEEE registers as properly
>          */
>         /* hw->phy.ops.write_reg(hw, PHY_CONTROL, mii_reg);*/
>         usleep_range(1000, 2000);
> }

I tried uncommenting the phy write and that seems to work fine, and the
link partner now sees link down when I bring down the interface.

Does anyone know what this comment refers to, and which revisions of the
I225/I226 this applies to?  I looked at the git history of this code and
it seems that it has been this way since the code was merged.  Is it safe
for me to uncomment this code for I226 adapters in my kernel tree?


Thanks,
Lennert
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] igc driver doesn't power down the PHY when link is brought down

[Neftin, Sasha]

On 2/4/2023 03:51, Lennert Buytenhek wrote:
> Hi,
> 
> When I run 'ip link set FOO down' on an I226 adapter, the link partner
> keeps seeing link up, which is not what I would expect.  The igc driver
> contains this code snippet that references this behavior (igc_phy.c):
> 
>> /**
>>   * igc_power_down_phy_copper - Power down copper PHY
>>   * @hw: pointer to the HW structure
>>   *
>>   * Power down PHY to save power when interface is down and wake on lan
>>   * is not enabled.
>>   */
>> void igc_power_down_phy_copper(struct igc_hw *hw)
>> {
>>          u16 mii_reg = 0;
>>
>>          /* The PHY will retain its settings across a power down/up cycle */
>>          hw->phy.ops.read_reg(hw, PHY_CONTROL, &mii_reg);
>>          mii_reg |= MII_CR_POWER_DOWN;
>>
>>          /* Temporary workaround - should be removed when PHY will implement
>>           * IEEE registers as properly
>>           */
>>          /* hw->phy.ops.write_reg(hw, PHY_CONTROL, mii_reg);*/
>>          usleep_range(1000, 2000);
>> }
> 
> I tried uncommenting the phy write and that seems to work fine, and the
> link partner now sees link down when I bring down the interface.
> 
> Does anyone know what this comment refers to, and which revisions of the
> I225/I226 this applies to?  I looked at the git history of this code and
> it seems that it has been this way since the code was merged.  Is it safe
> for me to uncomment this code for I226 adapters in my kernel tree?
PHY of i225/226 parts managed by FW. Within the very early version of 
PHY FW observed some problems in writing the PHY_CONTROL IEEE register 
(power down). I believe it is ok to uncomment it now. I need to say that 
we still do not validate this on many platforms yet. I would consider 
checking it before submit to the upstream kernels (include wake up flows).
> 
> 
> Thanks,
> Lennert
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan