netdev - Re: Need help with mdiobus

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]

Message-ID: <58012437.6010706@codeaurora.org>
Date:   Fri, 14 Oct 2016 13:30:15 -0500
From:   Timur Tabi <timur@...eaurora.org>
To:     Andrew Lunn <andrew@...n.ch>, zefir.kurtisi@...atec.com
Cc:     netdev@...r.kernel.org
Subject: Re: Need help with mdiobus_register and phy

Andrew Lunn wrote:

> It is normal to get the phy-mode from device tree. I've no idea what
> ACPI is supposed to do. Setting it to PHY_INTERFACE_MODE_NA means you
> assume the boot loader has correctly setup the hardware. You ACPI
> firmware might of done this, but there is no guarantee a device tree
> base bootloader has. So i would prefer not changing this.

Fair enough.  I don't think it's specified anywhere what firmware is 
supposed to do.

What about specifying PHY_INTERFACE_MODE_NA on ACPI systems, but using 
the phy-mode property on device tree systems?  That doesn't sound like a 
great idea.

>> >I don't see any other driver issue BMCR_PDOWN in their functions.  I
>> >added some printks for the PHYSID1 and PHYSID2 registers before and
>> >after BMCR_PDOWN:
>> >
>> >at803x_suspend:235 MII_PHYSID1=004d MII_PHYSID2=d074
>> >at803x_suspend:242 MII_PHYSID1=ffff MII_PHYSID2=ffff
>> >
>> >So after calling BMCR_PDOWN, the PHYSID1 and PHYSID2 registers are
>> >no longer readable.  Is that expected?
> You are making two changes here. Is it the SGMII power down which is
> causing the id registers to return 0xffff, or the BMCR_PDOWN.
>
> The generic suspend code sets the PDOWN bit, so it is assuming the PHY
> will respond afterwards.

Ok, it took me a while to figure this out.  The driver does three writes:

phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL);
phy_write(phydev, MII_BMCR, phy_read(phydev, MII_BMCR) | BMCR_PDOWN);
phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL);

The first clears the AT803X_BT_BX_REG_SEL bit.  According to the 
datasheet, that changes the register set from copper to fiber mode. 
BMCR_PDOWN in fiber mode shuts off the SerDes bus.  That's not true in 
copper mode.

Then after shutting down SerDes, it switches back to copper mode.

I also noticed the at803x_suspend already sends BMCR_PDOWN in copper 
mode earlier in the function.

So the question remains: should drivers shut down the SerDes bus when 
they suspend?  In a sense, I'm wondering if we should revert

	at803x: fix suspend/resume for SGMII link

However, the changelog for that patch makes it sound like it's a 
necessary fix.

So I'm torn.  With the SerDes connection disabled, the driver no longer 
responds to ID register reads.  That seems like something that would be 
broken on device tree as well, but I don't understand why no one noticed 
it before.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.