netdev - Re: [PATCH RFC] net: phy: add state PERM

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]

Message-ID: <9e1b2e30-139d-c3b9-0ac3-5775a4ade3a6@gmail.com>
Date:   Mon, 10 Jun 2019 10:41:38 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC] net: phy: add state PERM_FAIL

On 6/10/19 10:37 AM, Heiner Kallweit wrote:
> This RFC patch is a follow-up to discussion [0]. In cases like missing
> PHY firmware we may want to keep the PHY from being brought up, but
> still allow MDIO access. Setting state PERM_FAIL in the probe or
> config_init callback allows to achieve this.

While the use case is potentially applicable to PHY drivers beyond the
marvell10g driver, this concerns me for a number of reasons:

- the reasons why PHY_PERM_FAIL might be entered are entirely driver
specific, thus making it hard to diagnose

- a PHY driver that requires a firmware should either be loaded prior to
Linux taking over the PHY, or should be loaded by the PHY driver itself

So the bottom line of my reasoning is that, if we could make this
marvell10g specific for now, and we generalize that later once we find a
second candidate, that would seem preferable.

> 
> [0] https://marc.info/?t=155973142200002&r=1&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  drivers/net/phy/phy.c | 10 ++++++++--
>  include/linux/phy.h   |  5 +++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d91507650..889437512 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -44,6 +44,7 @@ static const char *phy_state_to_str(enum phy_state st)
>  	PHY_STATE_STR(RUNNING)
>  	PHY_STATE_STR(NOLINK)
>  	PHY_STATE_STR(HALTED)
> +	PHY_STATE_STR(PERM_FAIL)
>  	}
>  
>  	return NULL;
> @@ -744,7 +745,8 @@ static void phy_error(struct phy_device *phydev)
>  	WARN_ON(1);
>  
>  	mutex_lock(&phydev->lock);
> -	phydev->state = PHY_HALTED;
> +	if (phydev->state != PHY_PERM_FAIL)
> +		phydev->state = PHY_HALTED;
>  	mutex_unlock(&phydev->lock);
>  
>  	phy_trigger_machine(phydev);
> @@ -897,7 +899,10 @@ void phy_start(struct phy_device *phydev)
>  {
>  	mutex_lock(&phydev->lock);
>  
> -	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
> +	if (phydev->state == PHY_PERM_FAIL) {
> +		phydev_warn(phydev, "Can't start PHY because it's in state PERM_FAIL\n");
> +		goto out;
> +	} else if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
>  		WARN(1, "called from state %s\n",
>  		     phy_state_to_str(phydev->state));
>  		goto out;
> @@ -934,6 +939,7 @@ void phy_state_machine(struct work_struct *work)
>  	switch (phydev->state) {
>  	case PHY_DOWN:
>  	case PHY_READY:
> +	case PHY_PERM_FAIL:
>  		break;
>  	case PHY_UP:
>  		needs_aneg = true;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index d0af7d37f..7f47b6605 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -300,11 +300,16 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
>   * HALTED: PHY is up, but no polling or interrupts are done. Or
>   * PHY is in an error state.
>   * - phy_start moves to UP
> + *
> + * PERM_FAIL: A permanent failure was detected and PHY isn't allowed to be
> + * brought up. Still we don't want to fail in probe to allow MDIO access
> + * to the PHY, e.g. to load missing firmware.
>   */
>  enum phy_state {
>  	PHY_DOWN = 0,
>  	PHY_READY,
>  	PHY_HALTED,
> +	PHY_PERM_FAIL,
>  	PHY_UP,
>  	PHY_RUNNING,
>  	PHY_NOLINK,
> 


-- 
Florian