[PATCH RFC] net: phy: add state PERM_FAIL

[PATCH RFC] net: phy: add state PERM_FAIL

[Heiner Kallweit]

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.

[0] https://marc.info/?t=155973142200002&r=1&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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,
-- 
2.21.0

Re: [PATCH RFC] net: phy: add state PERM_FAIL

[Florian Fainelli]

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

Re: [PATCH RFC] net: phy: add state PERM_FAIL

[Andrew Lunn]

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

Hi Florian

Both the Marvell10g and Aquantia PHY need the firmware in their FLASH.
It is a slow operation to perform. And so far, everybody has done it
from user space. I'm not sure we want to hold up the PHY driver probe
for multiple minutes if we where to do this in kernel.

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

The obvious second candidate is the Aquantia PHY. And i probably have
a board without firmware.

  Andrew

Re: [PATCH RFC] net: phy: add state PERM_FAIL

[Florian Fainelli]

On 6/10/19 11:51 AM, Andrew Lunn wrote:
>> - 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
> 
> Hi Florian
> 
> Both the Marvell10g and Aquantia PHY need the firmware in their FLASH.
> It is a slow operation to perform. And so far, everybody has done it
> from user space. I'm not sure we want to hold up the PHY driver probe
> for multiple minutes if we where to do this in kernel.
> 
>> 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.
> 
> The obvious second candidate is the Aquantia PHY. And i probably have
> a board without firmware.

Right, but that's kind of exceptional because you likely will want to do
development on this platform, so having it not provisioned is handy.

Maybe the broader question is how do you, Heiner and Russell imagine a
genuine case where the PHY does not have a firmware provided/loaded
before Linux does take over (say, BoM cost savings dictate no flash can
be used) and it must be loaded at runtime, do we call an user
helper/tool, or do we left the kernel load the firmware? I am not
talking about the case where the firmware is not found because it's the
first time you bring up the board, but it could be covered as well.
-- 
Florian

Re: [PATCH RFC] net: phy: add state PERM_FAIL

[Andrew Lunn]

> Maybe the broader question is how do you, Heiner and Russell imagine a
> genuine case where the PHY does not have a firmware provided/loaded
> before Linux does take over (say, BoM cost savings dictate no flash can
> be used

I've not seen either of these PHY devices not have a FLASH. I also
wonder how long such a download to RAM takes. I suspect it is
slow. The boards i have, have a 4Mbit Flash, so, 256K 16bit words.
How long does 256K MDIO transfers take, given that they are typically
polled IO? Is that a reasonable design/cost trade off?

       Andrew

Re: [PATCH RFC] net: phy: add state PERM_FAIL

[Florian Fainelli]

On 6/10/19 2:27 PM, Andrew Lunn wrote:
>> Maybe the broader question is how do you, Heiner and Russell imagine a
>> genuine case where the PHY does not have a firmware provided/loaded
>> before Linux does take over (say, BoM cost savings dictate no flash can
>> be used
> 
> I've not seen either of these PHY devices not have a FLASH. I also
> wonder how long such a download to RAM takes. I suspect it is
> slow. The boards i have, have a 4Mbit Flash, so, 256K 16bit words.
> How long does 256K MDIO transfers take, given that they are typically
> polled IO? Is that a reasonable design/cost trade off?

If you have a long enough uptime, sure. You could emulate the SPI flash
through the means of GPIO pins, it's embedded, so sky's (no pun
intended) is the limit :).
-- 
Florian