| Message ID | 20190206181040.29539-1-mdf@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: fixed_phy: Fix fixed_phy not checking GPIO | expand |
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
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
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
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
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));
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(-)