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: <20161014172514.GA23455@lunn.ch>
Date:   Fri, 14 Oct 2016 19:25:14 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Timur Tabi <timur@...eaurora.org>
Cc:     netdev@...r.kernel.org
Subject: Re: Need help with mdiobus_register and phy

On Fri, Oct 14, 2016 at 11:57:29AM -0500, Timur Tabi wrote:
> Andrew Lunn wrote:
> >That is a basic assumption of the code. If you cannot read the IDs how
> >are you supposed to know what device it is, and what quirks you need
> >to work around its broken features...
> >
> >Does the datasheet say anything about this?
> >
> >I would say for this device, suspend() is too aggressive.
> 
> This change in my driver makes the problem go away (I'm not sure if
> it's a "fix"):
> 
> @@ -992,7 +992,7 @@ int emac_mac_up(struct emac_adapter *adpt)
>         emac_mac_rx_descs_refill(adpt, &adpt->rx_q);
> 
>         ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
> -                                PHY_INTERFACE_MODE_SGMII);
> +                                PHY_INTERFACE_MODE_NA);

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.
 
> With the interface not set as SGMII, the following code in
> at803x_suspend() is not executed:
> 
> /* also power-down SGMII interface */
> ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> 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);
> 
> 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.

     Andrew