[PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[Moritz Fischer]

Fix fixed_phy not checking GPIO if no link_update callback
is registered.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

Hi all,

I've been trying to figure out where exactly this broke,
it must've been somewhere when the file was refactored
in connection with phylink?

Unfortunately I couldn't tell exactly so I don't have a
'Fixes' tag.

Should this also go be queued up for stable/5.0 if it is indeed
a bug?

Thanks,

Moritz

---
 drivers/net/phy/fixed_phy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index f136a23c1a35..d810f914aaa4 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -85,11 +85,11 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
 				s = read_seqcount_begin(&fp->seqcount);
 				fp->status.link = !fp->no_carrier;
 				/* Issue callback if user registered it. */
-				if (fp->link_update) {
+				if (fp->link_update)
 					fp->link_update(fp->phydev->attached_dev,
 							&fp->status);
-					fixed_phy_update(fp);
-				}
+				/* Check the GPIO for change in status */
+				fixed_phy_update(fp);
 				state = fp->status;
 			} while (read_seqcount_retry(&fp->seqcount, s));
 
-- 
2.20.1

Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[Andrew Lunn]

On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> Fix fixed_phy not checking GPIO if no link_update callback
> is registered.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Hi all,
> 
> I've been trying to figure out where exactly this broke,
> it must've been somewhere when the file was refactored
> in connection with phylink?

Hi Moritz

With a quick inspection, i also cannot see where it broken.

I think part of the issue is that all the current users have moved
onto using phylink, and phylink polls the GPIO, rather than having
fixed_link do it.

I would prefer to understand exactly which change broke it. Without
knowing how it broke, it is hard to say if this is the correct fix.

What is your use-case? You Cc: the usb list. So a USB-Ethernet dongle?
But then why fixed-link?

	Andrew

Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[Moritz Fischer]

Hi Andrew,

On Wed, Feb 06, 2019 at 10:59:05PM +0100, Andrew Lunn wrote:
> On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> > Fix fixed_phy not checking GPIO if no link_update callback
> > is registered.
> > 
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > 
> > Hi all,
> > 
> > I've been trying to figure out where exactly this broke,
> > it must've been somewhere when the file was refactored
> > in connection with phylink?
> 
> Hi Moritz
> 
> With a quick inspection, i also cannot see where it broken.
> 
> I think part of the issue is that all the current users have moved
> onto using phylink, and phylink polls the GPIO, rather than having
> fixed_link do it.

Yeah, I suspected at much :) I still feel we should fix fixed_phy as
long as there are still users for it.
> 
> I would prefer to understand exactly which change broke it. Without
> knowing how it broke, it is hard to say if this is the correct fix.

It might've been always this way. That being said I don't see why
one should've to implement an empty function (link_update) in my driver
just to read back the GPIO pin. Looking at the code it seems clear that
nothing else polls the GPIO, which doesn't seem right.

In my current understanding (correct me if I'm wrong), the link_update
callback would give the MAC a chance to update link parameters that
since we are a fixed phy we cannot read back from a PHY.

That seems conceptually independent from obtaining a link up/down info
from a GPIO pin. Wouldn't you agree?

> 
> What is your use-case? You Cc: the usb list. So a USB-Ethernet dongle?
> But then why fixed-link?

Not for this patch, did I ? I ran into this when testing nixge, but it
seemed unrelated with my changes.

Thanks,

Moritz

Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[Andrew Lunn]

On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> Fix fixed_phy not checking GPIO if no link_update callback
> is registered.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Hi all,
> 
> I've been trying to figure out where exactly this broke,
> it must've been somewhere when the file was refactored
> in connection with phylink?

After some digging, i now understand. It was 'broken' right from the
beginning. The only user of this when i introduced it was a board with
an Ethernet switch driven by a DSA driver. The GPIO is used to
indicate if an SFP has a loss of signal indication.

DSA would look for the fixed-link properties for the switch port, and
if it found it, use of_phy_register_fixed_link() to register a fixed
link.

However, the broadcom SF2 switch driver needs to use the callback
method of reporting link up/down for a fixed-link. So the DSA core
always registers a generic DSA callback, which then calls into the DSA
driver if its driver structure implements the callback.

So we always had the case of both, so despite it being broken, it
worked...

	Andrew

Re: [PATCH net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO

[Andrew Lunn]

On Wed, Feb 06, 2019 at 10:10:40AM -0800, Moritz Fischer wrote:
> Fix fixed_phy not checking GPIO if no link_update callback
> is registered.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>

Fixes: a5597008dbc2 ("phy: fixed_phy: Add gpio to determine link up/down.")

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

    Andrew