Re: [PATCH net-next v1 1/9] net: phy: Add cable test support to state machine

[Michal Kubecek <mkubecek@suse.cz>]

On Sat, Apr 25, 2020 at 08:06:13PM +0200, Andrew Lunn wrote:
> Running a cable test is desruptive to normal operation of the PHY and
> can take a 5 to 10 seconds to complete. The RTNL lock cannot be held
> for this amount of time, and add a new state to the state machine for
> running a cable test.
> 
> The driver is expected to implement two functions. The first is used
> to start a cable test. Once the test has started, it should return.
> 
> The second function is called once per second, or on interrupt to
> check if the cable test is complete, and to allow the PHY to report
> the status.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/phy.c | 65 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h   | 28 +++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 72c69a9c8a98..4a6279c4a3a3 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
[...]
> @@ -470,6 +472,51 @@ static void phy_trigger_machine(struct phy_device *phydev)
>  	phy_queue_state_machine(phydev, 0);
>  }
>  
> +static void phy_cable_test_abort(struct phy_device *phydev)
> +{
> +	genphy_soft_reset(phydev);
> +}
> +
> +int phy_start_cable_test(struct phy_device *phydev,
> +			 struct netlink_ext_ack *extack)

Nitpick: the naming (phy_cable_test_abort() vs phy_start_cable_test())
should probably be consistent.

> +{
> +	int err;
> +
> +	if (!(phydev->drv &&
> +	      phydev->drv->cable_test_start &&
> +	      phydev->drv->cable_test_get_status)) {
> +		NL_SET_ERR_MSG(extack,
> +			       "PHY driver does not support cable testing");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&phydev->lock);
> +	if (phydev->state < PHY_UP ||
> +	    phydev->state >= PHY_CABLETEST) {
> +		NL_SET_ERR_MSG(extack,
> +			       "PHY not configured. Try setting interface up");
> +		err = -EBUSY;
> +		goto out;
> +	}

If I read the code correctly, this check would also catch an attempt to
start a cable test while the test is already in progress in which case
the error message would be rather misleading. I'm not sure what would be
more appropriate in such case: we can return -EBUSY (with more fitting
error message) but we could also silently ignore the request and let the
client wait for the notification from the test which is already in
progress.

Michal