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

Powered by blists - more mailing lists