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

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]

Date:   Sun, 26 Apr 2020 21:46:54 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Chris Healy <cphealy@...il.com>
Subject: Re: [PATCH net-next v1 1/9] net: phy: Add cable test support to
 state machine

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@...n.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