Re: [PATCH net 2/2] net: phy: Use C45 Helpers in PHY_FORCING state

On 04.03.2019 16:44, Andrew Lunn wrote:

On Mon, Mar 04, 2019 at 03:07:24PM +0000, Jose Abreu wrote:

quoted

Hi Andrew,

On 3/1/2019 1:53 PM, Andrew Lunn wrote:

quoted

On Fri, Mar 01, 2019 at 11:54:24AM +0100, Jose Abreu wrote:

quoted

+static inline int phy_update_link(struct phy_device *phydev)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (phydev->drv->read_status)
+		return phydev->drv->read_status(phydev);
+	else if (phydev->is_c45)
+		return gen10g_read_status(phydev);
+	else
+		return genphy_update_link(phydev);
+}

Hi Jose

The asymmetry here could be an issue.  We might fall into the trap
that a c45 PHY has the full state in phydev updated, were as a c22
only has the link updated. Somebody testing on C45 might miss a bug
for a C22 device.

Notice that this phy_update_link() is called from PHY_FORCING
state which in my case happens when autoneg is not enabled / is
not supported.

I think it makes sense, in this case, to only update link status,
no ?

 
Hi Jose

It is actually quite difficult to determine when the link is up. I
personally would not trust gen10g_read_status() to get this right, and
would always implement the read_status callback.

Not to forget that we just stopped exporting gen10g_read_status().
genphy_c45_read_link() seems to be the right function to be used
in phy_update_link().

Which PHY driver are you using, which does not support
read_status(). All the mainline PHY drivers do seem to have
read_status implemented.

    Andrew

Heiner