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