[PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Russell King]

Allow the PHY to probe when there is no firmware, but do not allow the
link to come up by forcing the PHY state to PHY_HALTED in a similar way
to phy_error().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Depends on "net: phy: marvell10g: report if the PHY fails to boot firmware"

 drivers/net/phy/marvell10g.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 754cde873dde..86333d98b384 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -60,6 +60,8 @@ enum {
 };
 
 struct mv3310_priv {
+	bool firmware_failed;
+
 	struct device *hwmon_dev;
 	char *hwmon_name;
 };
@@ -214,6 +216,10 @@ static int mv3310_probe(struct phy_device *phydev)
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	ret = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT);
 	if (ret < 0)
 		return ret;
@@ -221,13 +227,9 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret & MV_PMA_BOOT_FATAL) {
 		dev_warn(&phydev->mdio.dev,
 			 "PHY failed to boot firmware, status=%04x\n", ret);
-		return -ENODEV;
+		priv->firmware_failed = true;
 	}
 
-	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
 	ret = mv3310_hwmon_probe(phydev);
@@ -247,6 +249,19 @@ static int mv3310_resume(struct phy_device *phydev)
 	return mv3310_hwmon_config(phydev, true);
 }
 
+static void mv3310_link_change_notify(struct phy_device *phydev)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum phy_state state = phydev->state;
+
+	if (priv->firmware_failed &&
+	    (state == PHY_UP || state == PHY_RESUMING)) {
+		dev_warn(&phydev->mdio.dev,
+			 "PHY firmware failure: link forced down");
+		phydev->state = PHY_HALTED;
+	}
+}
+
 /* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
  * don't set bit 14 in PMA Extended Abilities (1.11), although they do
  * support 2.5GBASET and 5GBASET. For these models, we can still read their
-- 
2.7.4

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Andrew Lunn]

On Wed, Jun 05, 2019 at 11:43:16AM +0100, Russell King wrote:
> Allow the PHY to probe when there is no firmware, but do not allow the
> link to come up by forcing the PHY state to PHY_HALTED in a similar way
> to phy_error().
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[David Miller]

From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Wed, 05 Jun 2019 11:43:16 +0100

> +	    (state == PHY_UP || state == PHY_RESUMING)) {

drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
      (state == PHY_UP || state == PHY_RESUMING)) {
                                   ^~~~~~~~~~~~
                                   RPM_RESUMING
drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
At top level:
drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
 static void mv3310_link_change_notify(struct phy_device *phydev)
             ^~~~~~~~~~~~~~~~~~~~~~~~~

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Russell King - ARM Linux admin]

On Wed, Jun 05, 2019 at 06:48:27PM -0700, David Miller wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Date: Wed, 05 Jun 2019 11:43:16 +0100
> 
> > +	    (state == PHY_UP || state == PHY_RESUMING)) {
> 
> drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
> drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
>       (state == PHY_UP || state == PHY_RESUMING)) {
>                                    ^~~~~~~~~~~~
>                                    RPM_RESUMING
> drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
> At top level:
> drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
>  static void mv3310_link_change_notify(struct phy_device *phydev)
>              ^~~~~~~~~~~~~~~~~~~~~~~~~

Hmm. Looks like Heiner's changes in net-next _totally_ screw this
approach - it's not just about PHY_RESUMING being removed, it's
also about the link change notifier being moved. :(

This link notifier change also screws up my long-standing patches
to add support for SFP for the PHYs on Macchiatobin which I was
going to post next.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Andrew Lunn]

> This link notifier change also screws up my long-standing patches
> to add support for SFP for the PHYs on Macchiatobin which I was
> going to post next.

Hi Russell

Is that with the SFP hanging off the Marvell 10G PHY? We are seeing
that sort of chain more often, so it is something i would like to see
supported. Lets try to figure this out, maybe revert part of Heiners
change.

	Andrew

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Andrew Lunn]

On Thu, Jun 06, 2019 at 08:59:19AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jun 05, 2019 at 06:48:27PM -0700, David Miller wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > Date: Wed, 05 Jun 2019 11:43:16 +0100
> > 
> > > +	    (state == PHY_UP || state == PHY_RESUMING)) {
> > 
> > drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
> > drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
> >       (state == PHY_UP || state == PHY_RESUMING)) {
> >                                    ^~~~~~~~~~~~
> >                                    RPM_RESUMING
> > drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
> > At top level:
> > drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
> >  static void mv3310_link_change_notify(struct phy_device *phydev)
> >              ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Hmm. Looks like Heiner's changes in net-next _totally_ screw this
> approach - it's not just about PHY_RESUMING being removed, it's
> also about the link change notifier being moved. :(

Hi Russell

The link change notifier still seems to be called, and it is still
part of the phy_driver structure.

Please could you be more specific about what changes Heiner made which
causes this patch problems?

       Thanks
	Andrew

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Heiner Kallweit]

On 06.06.2019 14:42, Andrew Lunn wrote:
> On Thu, Jun 06, 2019 at 08:59:19AM +0100, Russell King - ARM Linux admin wrote:
>> On Wed, Jun 05, 2019 at 06:48:27PM -0700, David Miller wrote:
>>> From: Russell King <rmk+kernel@armlinux.org.uk>
>>> Date: Wed, 05 Jun 2019 11:43:16 +0100
>>>
>>>> +	    (state == PHY_UP || state == PHY_RESUMING)) {
>>>
>>> drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
>>> drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
>>>       (state == PHY_UP || state == PHY_RESUMING)) {
>>>                                    ^~~~~~~~~~~~
>>>                                    RPM_RESUMING
>>> drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
>>> At top level:
>>> drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
>>>  static void mv3310_link_change_notify(struct phy_device *phydev)
>>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Hmm. Looks like Heiner's changes in net-next _totally_ screw this
>> approach - it's not just about PHY_RESUMING being removed, it's
>> also about the link change notifier being moved. :(
> 
> Hi Russell
> 
> The link change notifier still seems to be called, and it is still
> part of the phy_driver structure.
> 
Before my change the link change notifier didn't do what the name states.
It was an "I'm going to run the state machine now, and something may
have changed or not" callback.
Still we have state changes happening outside the state machine and
therefore not calling the link change notifier. This brings me to the
second point:
I don't like too much state changes outside control of the state machine,
like in phy_start / phy_stop / phy_error. I think it would be better
if a state change request is sent to the state machine, and the state
machine decides whether the requested transition is allowed.
But I didn't dig deep enough into a possible solution yet.

Coming to the use case of keeping the link down if the firmware isn't
loaded. I'm not sure whether the firmware is needed for all modes, or
whether e.g. basic modes like 100BaseT work also w/o firmware.
Instead of manually changing the state it may be better to remove all
modes needing the firmware from supported and advertising bitmap in
the config_init callback.
Then modes not needing the firmware can still be used, and if no mode
remains then nothing is advertised and the link stays down anyway.

> Please could you be more specific about what changes Heiner made which
> causes this patch problems?
> 
>        Thanks
> 	Andrew
> 
Heiner

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Andrew Lunn]

65;5402;1c> I don't like too much state changes outside control of the state machine,
> like in phy_start / phy_stop / phy_error. I think it would be better
> if a state change request is sent to the state machine, and the state
> machine decides whether the requested transition is allowed.

Hi Heiner

I initially though that phy_error() would be a good way to do what
Russell wants. But the locks get in the way. Maybe add an unlocked
version which PHY drivers can use to indicate something fatal has
happened?

	Andrew

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Russell King - ARM Linux admin]

On Thu, Jun 06, 2019 at 08:36:11PM +0200, Andrew Lunn wrote:
> 65;5402;1c> I don't like too much state changes outside control of the state machine,
> > like in phy_start / phy_stop / phy_error. I think it would be better
> > if a state change request is sent to the state machine, and the state
> > machine decides whether the requested transition is allowed.
> 
> Hi Heiner
> 
> I initially though that phy_error() would be a good way to do what
> Russell wants. But the locks get in the way. Maybe add an unlocked
> version which PHY drivers can use to indicate something fatal has
> happened?

I think a way better solution would be if the phy_start() and
phy_stop() calls were forwarded to the PHY driver so that it has
an opportunity to:

(1) refuse a phy_start() if the PHY is not able to operate correctly,
    which would mean ip link set dev X up fails.

(2) for fiber capable PHYs, pass on to the SFP layer whether the
    network device is up or not.

I seem to remember from the original report that the problem with
the PHY without firmware is that the link apparently comes up
(despite the MMDs reporting that they are in reset) but the
negotiation results are incorrect and no data is able to pass -
but don't quote me on that, and we've now lost a reporter to test
this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Heiner Kallweit]

On 06.06.2019 20:36, Andrew Lunn wrote:
> 65;5402;1c> I don't like too much state changes outside control of the state machine,
>> like in phy_start / phy_stop / phy_error. I think it would be better
>> if a state change request is sent to the state machine, and the state
>> machine decides whether the requested transition is allowed.
> 
> Hi Heiner
> 
> I initially though that phy_error() would be a good way to do what
> Russell wants. But the locks get in the way. Maybe add an unlocked
> version which PHY drivers can use to indicate something fatal has
> happened?
> 
phy_error() switches to PHY_HALTED, therefore phy_start() would start
another attempt to bring up the PHY. Maybe some new state like
PHY_PERMANENT_FAILURE could be helpful.  Or do we need a temporary
failure state?

After a recent patch from Russell the probe callback returns -ENODEV
if no firmware is loaded. With the patch starting this discussion
this would have changed to not failing in probe but preventing the
PHY from coming up. The commit message missed an explanation what we
gain with this behavior change.

> 	Andrew
> 
Heiner

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Russell King - ARM Linux admin]

On Mon, Jun 10, 2019 at 03:40:10PM +0200, Heiner Kallweit wrote:
> On 06.06.2019 20:36, Andrew Lunn wrote:
> > 65;5402;1c> I don't like too much state changes outside control of the state machine,
> >> like in phy_start / phy_stop / phy_error. I think it would be better
> >> if a state change request is sent to the state machine, and the state
> >> machine decides whether the requested transition is allowed.
> > 
> > Hi Heiner
> > 
> > I initially though that phy_error() would be a good way to do what
> > Russell wants. But the locks get in the way. Maybe add an unlocked
> > version which PHY drivers can use to indicate something fatal has
> > happened?
> > 
> phy_error() switches to PHY_HALTED, therefore phy_start() would start
> another attempt to bring up the PHY. Maybe some new state like
> PHY_PERMANENT_FAILURE could be helpful.  Or do we need a temporary
> failure state?
> 
> After a recent patch from Russell the probe callback returns -ENODEV
> if no firmware is loaded. With the patch starting this discussion
> this would have changed to not failing in probe but preventing the
> PHY from coming up. The commit message missed an explanation what we
> gain with this behavior change.

As previously discussed, it is to allow access to the MDIO bus, and
therefore to allow a userspace tool to program the flash via the
MII ioctls.

At least two people have already independently created such a tool.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

[Russell King - ARM Linux admin]

On Thu, Jun 06, 2019 at 08:24:23PM +0200, Heiner Kallweit wrote:
> On 06.06.2019 14:42, Andrew Lunn wrote:
> > On Thu, Jun 06, 2019 at 08:59:19AM +0100, Russell King - ARM Linux admin wrote:
> >> On Wed, Jun 05, 2019 at 06:48:27PM -0700, David Miller wrote:
> >>> From: Russell King <rmk+kernel@armlinux.org.uk>
> >>> Date: Wed, 05 Jun 2019 11:43:16 +0100
> >>>
> >>>> +	    (state == PHY_UP || state == PHY_RESUMING)) {
> >>>
> >>> drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
> >>> drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
> >>>       (state == PHY_UP || state == PHY_RESUMING)) {
> >>>                                    ^~~~~~~~~~~~
> >>>                                    RPM_RESUMING
> >>> drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
> >>> At top level:
> >>> drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
> >>>  static void mv3310_link_change_notify(struct phy_device *phydev)
> >>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Hmm. Looks like Heiner's changes in net-next _totally_ screw this
> >> approach - it's not just about PHY_RESUMING being removed, it's
> >> also about the link change notifier being moved. :(
> > 
> > Hi Russell
> > 
> > The link change notifier still seems to be called, and it is still
> > part of the phy_driver structure.
> > 
> Before my change the link change notifier didn't do what the name states.
> It was an "I'm going to run the state machine now, and something may
> have changed or not" callback.

Sure, but you've made an unexpected semantic change to the callback,
which means patches can be applied (even back-ported) that may depend
on one behaviour or the other, which will end up introducing bugs.

> Still we have state changes happening outside the state machine and
> therefore not calling the link change notifier. This brings me to the
> second point:
> 
> I don't like too much state changes outside control of the state machine,
> like in phy_start / phy_stop / phy_error. I think it would be better
> if a state change request is sent to the state machine, and the state
> machine decides whether the requested transition is allowed.

I do like to waste my time creating patches _after_ a discussion about
how this should be handled, writing code, getting people to test, and
then submitting the patches.  It makes me feel like a valued individual
when I end up wasting both my time and others time like that.

However, there is now a bigger issue - the person who reported the
issue has now programmed firmware into their PHYs, so can no longer
test.  So, whatever we now come up with has to be just submitted
without testing that it actually works.

> Coming to the use case of keeping the link down if the firmware isn't
> loaded. I'm not sure whether the firmware is needed for all modes, or
> whether e.g. basic modes like 100BaseT work also w/o firmware.
> Instead of manually changing the state it may be better to remove all
> modes needing the firmware from supported and advertising bitmap in
> the config_init callback.
> Then modes not needing the firmware can still be used, and if no mode
> remains then nothing is advertised and the link stays down anyway.

We've lost the ability to determine that now.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up