[PATCH net 0/2] Use C45 Helpers when possible

For -net. Please review!

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <redacted>

Jose Abreu (2):
  net: phy: Use C45 Helpers in phy_read_status()
  net: phy: Use C45 Helpers in PHY_FORCING state

 drivers/net/phy/phy.c |  2 +-
 include/linux/phy.h   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.7.4

When using a C45 PHY that is in PHY_FORCING state we are currently not
taking into account that this kind of PHY has different update_link
functions.

Use the C45 Helpers instead or the driver built-in read_status() helper,
if possible.

Signed-off-by: Jose Abreu <redacted>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <redacted>
---
 drivers/net/phy/phy.c |  2 +-
 include/linux/phy.h   | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c5675df5fc6f..1ef5dbc384d0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -940,7 +940,7 @@ void phy_state_machine(struct work_struct *work)
 		err = phy_check_link_status(phydev);
 		break;
 	case PHY_FORCING:
-		err = genphy_update_link(phydev);
+		err = phy_update_link(phydev);
 		if (err)
 			break;
 

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 872899136fdc..67caa7eb93b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1036,6 +1036,19 @@ static inline int phy_read_status(struct phy_device *phydev)
 		return genphy_read_status(phydev);
 }
 
+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);
+}
+
 void phy_driver_unregister(struct phy_driver *drv);
 void phy_drivers_unregister(struct phy_driver *drv, int n);
 int phy_driver_register(struct phy_driver *new_driver, struct module *owner);

-- 
2.7.4

Currently phy_read_status() considers that either the PHY driver has the
read_status() callback or uses the generic callback.

For C45 PHYs we need to use the gen10g_read_status() callback.

Signed-off-by: Jose Abreu <redacted>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <redacted>
---
 include/linux/phy.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 333b56d8f746..872899136fdc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -1030,6 +1030,8 @@ static inline int phy_read_status(struct phy_device *phydev)
 
 	if (phydev->drv->read_status)
 		return phydev->drv->read_status(phydev);
+	else if (phydev->is_c45)
+		return gen10g_read_status(phydev);
 	else
 		return genphy_read_status(phydev);
 }

-- 
2.7.4

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

For -net. Please review!

Hi Jose

Patches for net should have fixes: tags, making it clear what they
fix, and how far back the patches should be ported.

Thanks
	Andrew

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

+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.

Maybe this should be called phy_read_state(), and calls
genphy_read_status() not genphy_update_link().

     Andrew

On 3/1/2019 2:54 AM, Jose Abreu wrote:

Currently phy_read_status() considers that either the PHY driver has the
read_status() callback or uses the generic callback.

For C45 PHYs we need to use the gen10g_read_status() callback.

Right, so we could expect your C45 PHY driver to assign the read_status
callback to gen10g_read_status() if it is appropriate. So far most of
the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
own read_status() callback to be feature complete. Unlike C22 PHYs that
can really be driven with a simple generic PHY driver model for standard
features, C45 PHYs seem to be quirky enough this does not work anymore.

-- 
Florian

On Fri, Mar 01, 2019 at 07:14:20PM -0800, Florian Fainelli wrote:

On 3/1/2019 2:54 AM, Jose Abreu wrote:

quoted

Currently phy_read_status() considers that either the PHY driver has the
read_status() callback or uses the generic callback.

For C45 PHYs we need to use the gen10g_read_status() callback.

Right, so we could expect your C45 PHY driver to assign the read_status
callback to gen10g_read_status() if it is appropriate. So far most of
the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
own read_status() callback to be feature complete. Unlike C22 PHYs that
can really be driven with a simple generic PHY driver model for standard
features, C45 PHYs seem to be quirky enough this does not work anymore.

Hi Jose

Does your PHY support 1000Base-T? If so you need read_status() because
the registers for that link mode don't appear to be standardized.

    Andrew

On 02.03.2019 15:11, Andrew Lunn wrote:

On Fri, Mar 01, 2019 at 07:14:20PM -0800, Florian Fainelli wrote:

quoted

On 3/1/2019 2:54 AM, Jose Abreu wrote:

quoted

Currently phy_read_status() considers that either the PHY driver has the
read_status() callback or uses the generic callback.

For C45 PHYs we need to use the gen10g_read_status() callback.

The gen10g_ functions are deprecated and shouldn't be used in new code.
Consider the (partially brand-new) genphy_c45_ functions instead.
This should be ok because I think the two changes are material for
net-next.

The gen10g functions belong to the old gen10g driver which knows about
10G only. I think sooner or later we'll replace it with a genc45
driver or similar.

quoted

Right, so we could expect your C45 PHY driver to assign the read_status
callback to gen10g_read_status() if it is appropriate. So far most of
the 10g PHY drivers (cortina, marvell10g, aquantia) have to define their
own read_status() callback to be feature complete. Unlike C22 PHYs that
can really be driven with a simple generic PHY driver model for standard
features, C45 PHYs seem to be quirky enough this does not work anymore.

Hi Jose

Does your PHY support 1000Base-T? If so you need read_status() because
the registers for that link mode don't appear to be standardized.

    Andrew

Heiner

Hi Andrew,

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

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 ?

Thanks,
Jose Miguel Abreu

Maybe this should be called phy_read_state(), and calls
genphy_read_status() not genphy_update_link().

     Andrew

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

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.

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

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