[PATCH net-next 0/4] phylink/sfp updates

[PATCH net-next 0/4] phylink/sfp updates

[Russell King - ARM Linux admin]

Hi,

I realise that net-next probably isn't open yet, but I believe folk
will find these patches "interesting" so I'm sending them to share
them with people working on this code, rather than expecting them to
be picked up this week.

The first patch adds support for using interrupts when using a GPIO
for link status tracking, rather than polling it at one second
intervals.  This reduces the need to wakeup the CPU every second.

The second patch adds support to the MII ioctl API to read and write
Clause 45 PHY registers.  I don't know how desirable this is for
mainline, but I have used this facility extensively to investigate
the Marvell 88x3310 PHY.

There have been discussions about removing "netdev" from phylink.
The last two patches remove netdev from the sfp code, which would be
a necessary step in that direction.

 drivers/net/phy/phy.c     | 33 ++++++++++++++++++++--------
 drivers/net/phy/phylink.c | 55 +++++++++++++++++++++++++++++++++++++++++------
 drivers/net/phy/sfp-bus.c | 14 +++++-------
 include/linux/sfp.h       | 12 +++++++----
 4 files changed, 86 insertions(+), 28 deletions(-)

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

[PATCH net-next 1/4] net: phylink: support for link gpio interrupt

[Russell King]

Add support for using GPIO interrupts with a fixed-link GPIO rather than
polling the GPIO every second and invoking the phylink resolution.  This
avoids unnecessary calls to mac_config().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 74983593834b..bdee5f307a7f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -52,6 +52,7 @@ struct phylink {
 	/* The link configuration settings */
 	struct phylink_link_state link_config;
 	struct gpio_desc *link_gpio;
+	unsigned int link_irq;
 	struct timer_list link_poll;
 	void (*get_fixed_state)(struct net_device *dev,
 				struct phylink_link_state *s);
@@ -608,7 +609,7 @@ void phylink_destroy(struct phylink *pl)
 {
 	if (pl->sfp_bus)
 		sfp_unregister_upstream(pl->sfp_bus);
-	if (!IS_ERR_OR_NULL(pl->link_gpio))
+	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
 
 	cancel_work_sync(&pl->resolve);
@@ -871,6 +872,15 @@ void phylink_mac_change(struct phylink *pl, bool up)
 }
 EXPORT_SYMBOL_GPL(phylink_mac_change);
 
+static irqreturn_t phylink_link_handler(int irq, void *data)
+{
+	struct phylink *pl = data;
+
+	phylink_run_resolve(pl);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * phylink_start() - start a phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -906,7 +916,22 @@ void phylink_start(struct phylink *pl)
 	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	phylink_run_resolve(pl);
 
-	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
+	if (pl->link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
+		int irq = gpiod_to_irq(pl->link_gpio);
+
+		if (irq > 0) {
+			if (!request_irq(irq, phylink_link_handler,
+					 IRQF_TRIGGER_RISING |
+					 IRQF_TRIGGER_FALLING,
+					 "netdev link", pl))
+				pl->link_irq = irq;
+			else
+				irq = 0;
+		}
+		if (irq <= 0)
+			mod_timer(&pl->link_poll, jiffies + HZ);
+	}
+	if (pl->link_an_mode == MLO_AN_FIXED && pl->get_fixed_state)
 		mod_timer(&pl->link_poll, jiffies + HZ);
 	if (pl->sfp_bus)
 		sfp_upstream_start(pl->sfp_bus);
@@ -932,8 +957,11 @@ void phylink_stop(struct phylink *pl)
 		phy_stop(pl->phydev);
 	if (pl->sfp_bus)
 		sfp_upstream_stop(pl->sfp_bus);
-	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
-		del_timer_sync(&pl->link_poll);
+	del_timer_sync(&pl->link_poll);
+	if (pl->link_irq) {
+		free_irq(pl->link_irq, pl);
+		pl->link_irq = 0;
+	}
 
 	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
 }
-- 
2.7.4

Re: [PATCH net-next 1/4] net: phylink: support for link gpio interrupt

[Florian Fainelli]

On 5/20/19 8:22 AM, Russell King wrote:
> Add support for using GPIO interrupts with a fixed-link GPIO rather than
> polling the GPIO every second and invoking the phylink resolution.  This
> avoids unnecessary calls to mac_config().
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Just one comment, see below:

[snip]

> -	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
> -		del_timer_sync(&pl->link_poll);
> +	del_timer_sync(&pl->link_poll);

Removing a timer that was only set-up if pl->link_an_mode ==
MLO_AN_FIXED probably does not hurt, but this breaks symmetry a bit here.
-- 
Florian

Re: [PATCH net-next 1/4] net: phylink: support for link gpio interrupt

[Russell King - ARM Linux admin]

On Mon, May 20, 2019 at 11:22:21AM -0700, Florian Fainelli wrote:
> On 5/20/19 8:22 AM, Russell King wrote:
> > Add support for using GPIO interrupts with a fixed-link GPIO rather than
> > polling the GPIO every second and invoking the phylink resolution.  This
> > avoids unnecessary calls to mac_config().
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Just one comment, see below:
> 
> [snip]
> 
> > -	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
> > -		del_timer_sync(&pl->link_poll);
> > +	del_timer_sync(&pl->link_poll);
> 
> Removing a timer that was only set-up if pl->link_an_mode ==
> MLO_AN_FIXED probably does not hurt, but this breaks symmetry a bit here.

The reason for this change is because that is no longer the only case
that the timer would be running.  The timer will be running if either
of the following are true:

1. We are in fixed mode, and we have a get_fixed_state function
   registered.
2. We are in fixed mode, and have a GPIO, but are unable to get an
   interrupt for it.

It's way simpler and less error-prone to just delete the timer here,
rather than trying to keep track of whether we armed it.

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

[PATCH net-next 2/4] net: phy: allow Clause 45 access via mii ioctl

[Russell King]

Allow userspace to generate Clause 45 MII access cycles via phylib.
This is useful for tools such as mii-diag to be able to inspect Clause
45 PHYs.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e8885429293a..cd8990e34905 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -407,6 +407,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
 	bool change_autoneg = false;
+	int prtad, devad;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -414,14 +415,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		/* fall through */
 
 	case SIOCGMIIREG:
-		mii_data->val_out = mdiobus_read(phydev->mdio.bus,
-						 mii_data->phy_id,
-						 mii_data->reg_num);
+		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+			prtad = mdio_phy_id_prtad(mii_data->phy_id);
+			devad = mdio_phy_id_devad(mii_data->phy_id);
+			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+		} else {
+			prtad = mii_data->phy_id;
+			devad = mii_data->reg_num;
+		}
+		mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
+						 devad);
 		return 0;
 
 	case SIOCSMIIREG:
-		if (mii_data->phy_id == phydev->mdio.addr) {
-			switch (mii_data->reg_num) {
+		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+			prtad = mdio_phy_id_prtad(mii_data->phy_id);
+			devad = mdio_phy_id_devad(mii_data->phy_id);
+			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+		} else {
+			prtad = mii_data->phy_id;
+			devad = mii_data->reg_num;
+		}
+		if (prtad == phydev->mdio.addr) {
+			switch (devad) {
 			case MII_BMCR:
 				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
 					if (phydev->autoneg == AUTONEG_ENABLE)
@@ -454,11 +470,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 			}
 		}
 
-		mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
-			      mii_data->reg_num, val);
+		mdiobus_write(phydev->mdio.bus, prtad, devad, val);
 
-		if (mii_data->phy_id == phydev->mdio.addr &&
-		    mii_data->reg_num == MII_BMCR &&
+		if (prtad == phydev->mdio.addr &&
+		    devad == MII_BMCR &&
 		    val & BMCR_RESET)
 			return phy_init_hw(phydev);
 
-- 
2.7.4

Re: [PATCH net-next 2/4] net: phy: allow Clause 45 access via mii ioctl

[Florian Fainelli]

On 5/20/19 8:22 AM, Russell King wrote:
> Allow userspace to generate Clause 45 MII access cycles via phylib.
> This is useful for tools such as mii-diag to be able to inspect Clause
> 45 PHYs.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 3/4] net: sfp: add mandatory attach/detach methods for sfp buses

[Russell King]

Add attach and detach methods for SFP buses, which will allow us to get
rid of the netdev storage in sfp-bus.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 16 ++++++++++++++++
 drivers/net/phy/sfp-bus.c |  4 ++--
 include/linux/sfp.h       |  6 ++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index bdee5f307a7f..7161c0fbb03c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1595,6 +1595,20 @@ int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
 }
 EXPORT_SYMBOL_GPL(phylink_mii_ioctl);
 
+static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
+{
+	struct phylink *pl = upstream;
+
+	pl->netdev->sfp_bus = bus;
+}
+
+static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
+{
+	struct phylink *pl = upstream;
+
+	pl->netdev->sfp_bus = NULL;
+}
+
 static int phylink_sfp_module_insert(void *upstream,
 				     const struct sfp_eeprom_id *id)
 {
@@ -1713,6 +1727,8 @@ static void phylink_sfp_disconnect_phy(void *upstream)
 }
 
 static const struct sfp_upstream_ops sfp_phylink_ops = {
+	.attach = phylink_sfp_attach,
+	.detach = phylink_sfp_detach,
 	.module_insert = phylink_sfp_module_insert,
 	.link_up = phylink_sfp_link_up,
 	.link_down = phylink_sfp_link_down,
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index fef701bfad62..c664c905830a 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -350,7 +350,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
 	bus->socket_ops->attach(bus->sfp);
 	if (bus->started)
 		bus->socket_ops->start(bus->sfp);
-	bus->netdev->sfp_bus = bus;
+	bus->upstream_ops->attach(bus->upstream, bus);
 	bus->registered = true;
 	return 0;
 }
@@ -359,8 +359,8 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
 {
 	const struct sfp_upstream_ops *ops = bus->upstream_ops;
 
-	bus->netdev->sfp_bus = NULL;
 	if (bus->registered) {
+		bus->upstream_ops->detach(bus->upstream, bus);
 		if (bus->started)
 			bus->socket_ops->stop(bus->sfp);
 		bus->socket_ops->detach(bus->sfp);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index d9d9de3fcf8e..a3f0336dd703 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -469,6 +469,10 @@ struct sfp_bus;
 
 /**
  * struct sfp_upstream_ops - upstream operations structure
+ * @attach: called when the sfp socket driver is bound to the upstream
+ *   (mandatory).
+ * @detach: called when the sfp socket driver is unbound from the upstream
+ *   (mandatory).
  * @module_insert: called after a module has been detected to determine
  *   whether the module is supported for the upstream device.
  * @module_remove: called after the module has been removed.
@@ -481,6 +485,8 @@ struct sfp_bus;
  *   been removed.
  */
 struct sfp_upstream_ops {
+	void (*attach)(void *priv, struct sfp_bus *bus);
+	void (*detach)(void *priv, struct sfp_bus *bus);
 	int (*module_insert)(void *priv, const struct sfp_eeprom_id *id);
 	void (*module_remove)(void *priv);
 	void (*link_down)(void *priv);
-- 
2.7.4

Re: [PATCH net-next 3/4] net: sfp: add mandatory attach/detach methods for sfp buses

[Florian Fainelli]

On 5/20/19 8:22 AM, Russell King wrote:
> Add attach and detach methods for SFP buses, which will allow us to get
> rid of the netdev storage in sfp-bus.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 4/4] net: sfp: remove sfp-bus use of netdevs

[Russell King]

The sfp-bus code now no longer has any use for the network device
structure, so remove its use.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c |  3 +--
 drivers/net/phy/sfp-bus.c | 10 +++-------
 include/linux/sfp.h       |  6 ++----
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7161c0fbb03c..e9ffe92c0650 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -518,8 +518,7 @@ static int phylink_register_sfp(struct phylink *pl,
 		return ret;
 	}
 
-	pl->sfp_bus = sfp_register_upstream(ref.fwnode, pl->netdev, pl,
-					    &sfp_phylink_ops);
+	pl->sfp_bus = sfp_register_upstream(ref.fwnode, pl, &sfp_phylink_ops);
 	if (!pl->sfp_bus)
 		return -ENOMEM;
 
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index c664c905830a..5bc8099eaaf1 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -23,7 +23,6 @@ struct sfp_bus {
 
 	const struct sfp_upstream_ops *upstream_ops;
 	void *upstream;
-	struct net_device *netdev;
 	struct phy_device *phydev;
 
 	bool registered;
@@ -442,13 +441,11 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
 {
 	bus->upstream_ops = NULL;
 	bus->upstream = NULL;
-	bus->netdev = NULL;
 }
 
 /**
  * sfp_register_upstream() - Register the neighbouring device
  * @fwnode: firmware node for the SFP bus
- * @ndev: network device associated with the interface
  * @upstream: the upstream private data
  * @ops: the upstream's &struct sfp_upstream_ops
  *
@@ -459,7 +456,7 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
  * On error, returns %NULL.
  */
 struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
-				      struct net_device *ndev, void *upstream,
+				      void *upstream,
 				      const struct sfp_upstream_ops *ops)
 {
 	struct sfp_bus *bus = sfp_bus_get(fwnode);
@@ -469,7 +466,6 @@ struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
 		rtnl_lock();
 		bus->upstream_ops = ops;
 		bus->upstream = upstream;
-		bus->netdev = ndev;
 
 		if (bus->sfp) {
 			ret = sfp_register_bus(bus);
@@ -591,7 +587,7 @@ struct sfp_bus *sfp_register_socket(struct device *dev, struct sfp *sfp,
 		bus->sfp = sfp;
 		bus->socket_ops = ops;
 
-		if (bus->netdev) {
+		if (bus->upstream_ops) {
 			ret = sfp_register_bus(bus);
 			if (ret)
 				sfp_socket_clear(bus);
@@ -611,7 +607,7 @@ EXPORT_SYMBOL_GPL(sfp_register_socket);
 void sfp_unregister_socket(struct sfp_bus *bus)
 {
 	rtnl_lock();
-	if (bus->netdev)
+	if (bus->upstream_ops)
 		sfp_unregister_bus(bus);
 	sfp_socket_clear(bus);
 	rtnl_unlock();
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index a3f0336dd703..1c35428e98bc 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -464,7 +464,6 @@ enum {
 struct fwnode_handle;
 struct ethtool_eeprom;
 struct ethtool_modinfo;
-struct net_device;
 struct sfp_bus;
 
 /**
@@ -510,7 +509,7 @@ int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
 void sfp_upstream_start(struct sfp_bus *bus);
 void sfp_upstream_stop(struct sfp_bus *bus);
 struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
-				      struct net_device *ndev, void *upstream,
+				      void *upstream,
 				      const struct sfp_upstream_ops *ops);
 void sfp_unregister_upstream(struct sfp_bus *bus);
 #else
@@ -555,8 +554,7 @@ static inline void sfp_upstream_stop(struct sfp_bus *bus)
 }
 
 static inline struct sfp_bus *sfp_register_upstream(
-	struct fwnode_handle *fwnode,
-	struct net_device *ndev, void *upstream,
+	struct fwnode_handle *fwnode, void *upstream,
 	const struct sfp_upstream_ops *ops)
 {
 	return (struct sfp_bus *)-1;
-- 
2.7.4

Re: [PATCH net-next 4/4] net: sfp: remove sfp-bus use of netdevs

[Florian Fainelli]

On 5/20/19 8:22 AM, Russell King wrote:
> The sfp-bus code now no longer has any use for the network device
> structure, so remove its use.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>--
Florian

Re: [PATCH net-next 0/4] phylink/sfp updates

[Vladimir Oltean]

On Mon, 20 May 2019 at 18:21, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> Hi,
>
> I realise that net-next probably isn't open yet, but I believe folk
> will find these patches "interesting" so I'm sending them to share
> them with people working on this code, rather than expecting them to
> be picked up this week.
>
> The first patch adds support for using interrupts when using a GPIO
> for link status tracking, rather than polling it at one second
> intervals.  This reduces the need to wakeup the CPU every second.
>
> The second patch adds support to the MII ioctl API to read and write
> Clause 45 PHY registers.  I don't know how desirable this is for
> mainline, but I have used this facility extensively to investigate
> the Marvell 88x3310 PHY.
>
> There have been discussions about removing "netdev" from phylink.
> The last two patches remove netdev from the sfp code, which would be
> a necessary step in that direction.

Hi Russell,

I've been working with Ioana to introduce a
phylink_create_raw/phylink_attach_raw set of function that work in the
absence of an attached_dev. It does not remove the netdev references
from the existing code, just guards the few places that needed that.
Then we plugged it into DSA (via a notifier block) and now phylink is
used for both the user and non-user ports. Next step is to make the
dpaa2-mac driver use this new API and send the patchset for review.
Hopefully that will happen tomorrow.

-Vladimir

>
>  drivers/net/phy/phy.c     | 33 ++++++++++++++++++++--------
>  drivers/net/phy/phylink.c | 55 +++++++++++++++++++++++++++++++++++++++++------
>  drivers/net/phy/sfp-bus.c | 14 +++++-------
>  include/linux/sfp.h       | 12 +++++++----
>  4 files changed, 86 insertions(+), 28 deletions(-)
>
> --
> 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-next 0/4] phylink/sfp updates

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Mon, 20 May 2019 16:21:34 +0100

> I realise that net-next probably isn't open yet, but I believe folk
> will find these patches "interesting" so I'm sending them to share
> them with people working on this code, rather than expecting them to
> be picked up this week.

Please repost when you think these are ready to be included as net-next
is open now.

Thanks.

[PATCH net-next 0/5] phylink/sfp updates

[Russell King - ARM Linux admin]

Hi,

This is a series of updates to phylink and sfp:

- Remove an unused net device argument from the phylink MII ioctl
  emulation code.

- add support for using interrupts when using a GPIO for link status
  tracking, rather than polling it at one second intervals.  This
  reduces the need to wakeup the CPU every second.

- add support to the MII ioctl API to read and write Clause 45 PHY
  registers.  I don't know how desirable this is for mainline, but I
  have used this facility extensively to investigate the Marvell
  88x3310 PHY.  A recent illustration of use for this was debugging
  the PHY-without-firmware problem recently reported.

- add mandatory attach/detach methods for the upstream side of sfp
  bus code, which will allow us to remove the "netdev" structure from
  the SFP layers.

- remove the "netdev" structure from the SFP upstream registration
  calls, which simplifies PHY to SFP links.

 drivers/net/phy/marvell10g.c | 101 +++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy.c        |  33 ++++++++++----
 drivers/net/phy/phylink.c    |  75 ++++++++++++++++++++++++--------
 drivers/net/phy/sfp-bus.c    |  14 +++---
 include/linux/sfp.h          |  12 +++--
 5 files changed, 195 insertions(+), 40 deletions(-)

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

[PATCH net-next 1/5] net: phylink: remove netdev from phylink mii ioctl emulation

[Russell King]

The netdev used in the phylink ioctl emulation is never used, so let's
remove it.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9044b95d2afe..219a061572d2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1340,8 +1340,8 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee);
  *
  * FIXME: should deal with negotiation state too.
  */
-static int phylink_mii_emul_read(struct net_device *ndev, unsigned int reg,
-				 struct phylink_link_state *state, bool aneg)
+static int phylink_mii_emul_read(unsigned int reg,
+				 struct phylink_link_state *state)
 {
 	struct fixed_phy_status fs;
 	int val;
@@ -1356,8 +1356,6 @@ static int phylink_mii_emul_read(struct net_device *ndev, unsigned int reg,
 	if (reg == MII_BMSR) {
 		if (!state->an_complete)
 			val &= ~BMSR_ANEGCOMPLETE;
-		if (!aneg)
-			val &= ~BMSR_ANEGCAPABLE;
 	}
 	return val;
 }
@@ -1453,8 +1451,7 @@ static int phylink_mii_read(struct phylink *pl, unsigned int phy_id,
 	case MLO_AN_FIXED:
 		if (phy_id == 0) {
 			phylink_get_fixed_state(pl, &state);
-			val = phylink_mii_emul_read(pl->netdev, reg, &state,
-						    true);
+			val = phylink_mii_emul_read(reg, &state);
 		}
 		break;
 
@@ -1467,8 +1464,7 @@ static int phylink_mii_read(struct phylink *pl, unsigned int phy_id,
 			if (val < 0)
 				return val;
 
-			val = phylink_mii_emul_read(pl->netdev, reg, &state,
-						    true);
+			val = phylink_mii_emul_read(reg, &state);
 		}
 		break;
 	}
-- 
2.7.4

Re: [PATCH net-next 1/5] net: phylink: remove netdev from phylink mii ioctl emulation

[Andrew Lunn]

On Tue, May 28, 2019 at 10:57:18AM +0100, Russell King wrote:
> The netdev used in the phylink ioctl emulation is never used, so let's
> remove it.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

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

    Andrew

[PATCH net-next 2/5] net: phylink: support for link gpio interrupt

[Russell King]

Add support for using GPIO interrupts with a fixed-link GPIO rather than
polling the GPIO every second and invoking the phylink resolution.  This
avoids unnecessary calls to mac_config().

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 219a061572d2..00cd0ed7ff3d 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -56,6 +56,7 @@ struct phylink {
 	phy_interface_t cur_interface;
 
 	struct gpio_desc *link_gpio;
+	unsigned int link_irq;
 	struct timer_list link_poll;
 	void (*get_fixed_state)(struct net_device *dev,
 				struct phylink_link_state *s);
@@ -612,7 +613,7 @@ void phylink_destroy(struct phylink *pl)
 {
 	if (pl->sfp_bus)
 		sfp_unregister_upstream(pl->sfp_bus);
-	if (!IS_ERR_OR_NULL(pl->link_gpio))
+	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
 
 	cancel_work_sync(&pl->resolve);
@@ -875,6 +876,15 @@ void phylink_mac_change(struct phylink *pl, bool up)
 }
 EXPORT_SYMBOL_GPL(phylink_mac_change);
 
+static irqreturn_t phylink_link_handler(int irq, void *data)
+{
+	struct phylink *pl = data;
+
+	phylink_run_resolve(pl);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * phylink_start() - start a phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -910,7 +920,22 @@ void phylink_start(struct phylink *pl)
 	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	phylink_run_resolve(pl);
 
-	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
+	if (pl->link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
+		int irq = gpiod_to_irq(pl->link_gpio);
+
+		if (irq > 0) {
+			if (!request_irq(irq, phylink_link_handler,
+					 IRQF_TRIGGER_RISING |
+					 IRQF_TRIGGER_FALLING,
+					 "netdev link", pl))
+				pl->link_irq = irq;
+			else
+				irq = 0;
+		}
+		if (irq <= 0)
+			mod_timer(&pl->link_poll, jiffies + HZ);
+	}
+	if (pl->link_an_mode == MLO_AN_FIXED && pl->get_fixed_state)
 		mod_timer(&pl->link_poll, jiffies + HZ);
 	if (pl->sfp_bus)
 		sfp_upstream_start(pl->sfp_bus);
@@ -936,8 +961,11 @@ void phylink_stop(struct phylink *pl)
 		phy_stop(pl->phydev);
 	if (pl->sfp_bus)
 		sfp_upstream_stop(pl->sfp_bus);
-	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
-		del_timer_sync(&pl->link_poll);
+	del_timer_sync(&pl->link_poll);
+	if (pl->link_irq) {
+		free_irq(pl->link_irq, pl);
+		pl->link_irq = 0;
+	}
 
 	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
 }
-- 
2.7.4

Re: [PATCH net-next 2/5] net: phylink: support for link gpio interrupt

[Russell King - ARM Linux admin]

Hi David,

I was intending to add a note to this patch indicating that it
depends on "net: phylink: ensure consistent phy interface mode" but
failed to do before sending it out - sorry!  If you'd prefer a patch
that doesn't depend on that, please ask.  The only difference is the
first two lines of context of the first hunk.

Russell.

On Tue, May 28, 2019 at 10:57:23AM +0100, Russell King wrote:
> Add support for using GPIO interrupts with a fixed-link GPIO rather than
> polling the GPIO every second and invoking the phylink resolution.  This
> avoids unnecessary calls to mac_config().
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 219a061572d2..00cd0ed7ff3d 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -56,6 +56,7 @@ struct phylink {
>  	phy_interface_t cur_interface;
>  
>  	struct gpio_desc *link_gpio;
> +	unsigned int link_irq;
>  	struct timer_list link_poll;
>  	void (*get_fixed_state)(struct net_device *dev,
>  				struct phylink_link_state *s);
> @@ -612,7 +613,7 @@ void phylink_destroy(struct phylink *pl)
>  {
>  	if (pl->sfp_bus)
>  		sfp_unregister_upstream(pl->sfp_bus);
> -	if (!IS_ERR_OR_NULL(pl->link_gpio))
> +	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
>  	cancel_work_sync(&pl->resolve);
> @@ -875,6 +876,15 @@ void phylink_mac_change(struct phylink *pl, bool up)
>  }
>  EXPORT_SYMBOL_GPL(phylink_mac_change);
>  
> +static irqreturn_t phylink_link_handler(int irq, void *data)
> +{
> +	struct phylink *pl = data;
> +
> +	phylink_run_resolve(pl);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  /**
>   * phylink_start() - start a phylink instance
>   * @pl: a pointer to a &struct phylink returned from phylink_create()
> @@ -910,7 +920,22 @@ void phylink_start(struct phylink *pl)
>  	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	phylink_run_resolve(pl);
>  
> -	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
> +	if (pl->link_an_mode == MLO_AN_FIXED && pl->link_gpio) {
> +		int irq = gpiod_to_irq(pl->link_gpio);
> +
> +		if (irq > 0) {
> +			if (!request_irq(irq, phylink_link_handler,
> +					 IRQF_TRIGGER_RISING |
> +					 IRQF_TRIGGER_FALLING,
> +					 "netdev link", pl))
> +				pl->link_irq = irq;
> +			else
> +				irq = 0;
> +		}
> +		if (irq <= 0)
> +			mod_timer(&pl->link_poll, jiffies + HZ);
> +	}
> +	if (pl->link_an_mode == MLO_AN_FIXED && pl->get_fixed_state)
>  		mod_timer(&pl->link_poll, jiffies + HZ);
>  	if (pl->sfp_bus)
>  		sfp_upstream_start(pl->sfp_bus);
> @@ -936,8 +961,11 @@ void phylink_stop(struct phylink *pl)
>  		phy_stop(pl->phydev);
>  	if (pl->sfp_bus)
>  		sfp_upstream_stop(pl->sfp_bus);
> -	if (pl->link_an_mode == MLO_AN_FIXED && !IS_ERR(pl->link_gpio))
> -		del_timer_sync(&pl->link_poll);
> +	del_timer_sync(&pl->link_poll);
> +	if (pl->link_irq) {
> +		free_irq(pl->link_irq, pl);
> +		pl->link_irq = 0;
> +	}
>  
>  	phylink_run_resolve_and_disable(pl, PHYLINK_DISABLE_STOPPED);
>  }
> -- 
> 2.7.4
> 
> 

-- 
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-next 2/5] net: phylink: support for link gpio interrupt

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Tue, 28 May 2019 11:02:49 +0100

> I was intending to add a note to this patch indicating that it
> depends on "net: phylink: ensure consistent phy interface mode" but
> failed to do before sending it out - sorry!  If you'd prefer a patch
> that doesn't depend on that, please ask.  The only difference is the
> first two lines of context of the first hunk.

Ok, I just applied that dependency to 'net' and then I'll make sure
I apply this net-next series after my next net --> net-next merge
which should be either later tonight or tomorrow.

Thanks for the heads up.

[PATCH net-next 3/5] net: phy: allow Clause 45 access via mii ioctl

[Russell King]

Allow userspace to generate Clause 45 MII access cycles via phylib.
This is useful for tools such as mii-diag to be able to inspect Clause
45 PHYs.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3745220c5c98..6d279c2ac1f8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -386,6 +386,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
 	bool change_autoneg = false;
+	int prtad, devad;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -393,14 +394,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		/* fall through */
 
 	case SIOCGMIIREG:
-		mii_data->val_out = mdiobus_read(phydev->mdio.bus,
-						 mii_data->phy_id,
-						 mii_data->reg_num);
+		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+			prtad = mdio_phy_id_prtad(mii_data->phy_id);
+			devad = mdio_phy_id_devad(mii_data->phy_id);
+			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+		} else {
+			prtad = mii_data->phy_id;
+			devad = mii_data->reg_num;
+		}
+		mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
+						 devad);
 		return 0;
 
 	case SIOCSMIIREG:
-		if (mii_data->phy_id == phydev->mdio.addr) {
-			switch (mii_data->reg_num) {
+		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+			prtad = mdio_phy_id_prtad(mii_data->phy_id);
+			devad = mdio_phy_id_devad(mii_data->phy_id);
+			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+		} else {
+			prtad = mii_data->phy_id;
+			devad = mii_data->reg_num;
+		}
+		if (prtad == phydev->mdio.addr) {
+			switch (devad) {
 			case MII_BMCR:
 				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
 					if (phydev->autoneg == AUTONEG_ENABLE)
@@ -433,11 +449,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 			}
 		}
 
-		mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
-			      mii_data->reg_num, val);
+		mdiobus_write(phydev->mdio.bus, prtad, devad, val);
 
-		if (mii_data->phy_id == phydev->mdio.addr &&
-		    mii_data->reg_num == MII_BMCR &&
+		if (prtad == phydev->mdio.addr &&
+		    devad == MII_BMCR &&
 		    val & BMCR_RESET)
 			return phy_init_hw(phydev);
 
-- 
2.7.4

Re: [PATCH net-next 3/5] net: phy: allow Clause 45 access via mii ioctl

[Vladimir Oltean]

On Tue, 28 May 2019 at 12:58, Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Allow userspace to generate Clause 45 MII access cycles via phylib.
> This is useful for tools such as mii-diag to be able to inspect Clause
> 45 PHYs.
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3745220c5c98..6d279c2ac1f8 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -386,6 +386,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>         struct mii_ioctl_data *mii_data = if_mii(ifr);
>         u16 val = mii_data->val_in;
>         bool change_autoneg = false;
> +       int prtad, devad;
>
>         switch (cmd) {
>         case SIOCGMIIPHY:
> @@ -393,14 +394,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>                 /* fall through */
>
>         case SIOCGMIIREG:
> -               mii_data->val_out = mdiobus_read(phydev->mdio.bus,
> -                                                mii_data->phy_id,
> -                                                mii_data->reg_num);
> +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> +               } else {
> +                       prtad = mii_data->phy_id;
> +                       devad = mii_data->reg_num;
> +               }
> +               mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
> +                                                devad);
>                 return 0;
>
>         case SIOCSMIIREG:
> -               if (mii_data->phy_id == phydev->mdio.addr) {
> -                       switch (mii_data->reg_num) {
> +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> +               } else {
> +                       prtad = mii_data->phy_id;
> +                       devad = mii_data->reg_num;
> +               }
> +               if (prtad == phydev->mdio.addr) {
> +                       switch (devad) {
>                         case MII_BMCR:
>                                 if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
>                                         if (phydev->autoneg == AUTONEG_ENABLE)
> @@ -433,11 +449,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>                         }
>                 }
>
> -               mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
> -                             mii_data->reg_num, val);
> +               mdiobus_write(phydev->mdio.bus, prtad, devad, val);
>
> -               if (mii_data->phy_id == phydev->mdio.addr &&
> -                   mii_data->reg_num == MII_BMCR &&
> +               if (prtad == phydev->mdio.addr &&
> +                   devad == MII_BMCR &&
>                     val & BMCR_RESET)
>                         return phy_init_hw(phydev);
>
> --
> 2.7.4
>

Hi Russell,

I find the SIOCGMIIREG/SIOCGMIIPHY ioctls useful for C45 just as much
as they are for C22, but I think the way they work is a big hack and
for that reason they're less than useful when you need them most.
These ioctls work by hijacking the MDIO bus driver of a PHY that is
attached to a net_device. Hence they can be used to access at most a
PHY that lies on the same MDIO bus as one you already have a
phy-handle to.
If you have a PHY issue that makes of_phy_connect fail and the
net_device to fail to probe, basically you're SOL because you lose
that one handle that userspace had to the MDIO bus.
Similarly if you're doing a bring-up and all PHY interfaces are fixed-link.
Maybe it would be better to rethink this and expose some sysfs nodes
for raw MDIO access in the bus drivers.

-Vladimir

Re: [PATCH net-next 3/5] net: phy: allow Clause 45 access via mii ioctl

[Russell King - ARM Linux admin]

On Tue, May 28, 2019 at 03:54:31PM +0300, Vladimir Oltean wrote:
> On Tue, 28 May 2019 at 12:58, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Allow userspace to generate Clause 45 MII access cycles via phylib.
> > This is useful for tools such as mii-diag to be able to inspect Clause
> > 45 PHYs.
> >
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 3745220c5c98..6d279c2ac1f8 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -386,6 +386,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> >         struct mii_ioctl_data *mii_data = if_mii(ifr);
> >         u16 val = mii_data->val_in;
> >         bool change_autoneg = false;
> > +       int prtad, devad;
> >
> >         switch (cmd) {
> >         case SIOCGMIIPHY:
> > @@ -393,14 +394,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> >                 /* fall through */
> >
> >         case SIOCGMIIREG:
> > -               mii_data->val_out = mdiobus_read(phydev->mdio.bus,
> > -                                                mii_data->phy_id,
> > -                                                mii_data->reg_num);
> > +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> > +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> > +               } else {
> > +                       prtad = mii_data->phy_id;
> > +                       devad = mii_data->reg_num;
> > +               }
> > +               mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
> > +                                                devad);
> >                 return 0;
> >
> >         case SIOCSMIIREG:
> > -               if (mii_data->phy_id == phydev->mdio.addr) {
> > -                       switch (mii_data->reg_num) {
> > +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> > +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> > +               } else {
> > +                       prtad = mii_data->phy_id;
> > +                       devad = mii_data->reg_num;
> > +               }
> > +               if (prtad == phydev->mdio.addr) {
> > +                       switch (devad) {
> >                         case MII_BMCR:
> >                                 if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
> >                                         if (phydev->autoneg == AUTONEG_ENABLE)
> > @@ -433,11 +449,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> >                         }
> >                 }
> >
> > -               mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
> > -                             mii_data->reg_num, val);
> > +               mdiobus_write(phydev->mdio.bus, prtad, devad, val);
> >
> > -               if (mii_data->phy_id == phydev->mdio.addr &&
> > -                   mii_data->reg_num == MII_BMCR &&
> > +               if (prtad == phydev->mdio.addr &&
> > +                   devad == MII_BMCR &&
> >                     val & BMCR_RESET)
> >                         return phy_init_hw(phydev);
> >
> > --
> > 2.7.4
> >
> 
> Hi Russell,
> 
> I find the SIOCGMIIREG/SIOCGMIIPHY ioctls useful for C45 just as much
> as they are for C22, but I think the way they work is a big hack and
> for that reason they're less than useful when you need them most.
> These ioctls work by hijacking the MDIO bus driver of a PHY that is
> attached to a net_device. Hence they can be used to access at most a
> PHY that lies on the same MDIO bus as one you already have a
> phy-handle to.
> If you have a PHY issue that makes of_phy_connect fail and the
> net_device to fail to probe, basically you're SOL because you lose
> that one handle that userspace had to the MDIO bus.
> Similarly if you're doing a bring-up and all PHY interfaces are fixed-link.
> Maybe it would be better to rethink this and expose some sysfs nodes
> for raw MDIO access in the bus drivers.

I don't see how putting some attributes in sysfs helps - sysfs is
fine for exporting structured information, but with PHYs, it's not
that structured.  ioctls on sysfs attributes are certainly very
undesirable, and not supported.  So, I don't think sysfs would work.

debugfs is another option, that is more flexible, but that is also
based around the idea of exporting stuff in a relatively structured
way, and I don't think a MII bus with arbitary PHYs with an arbitary
number of layers would be exportable in a particularly nice way.
Consider that a Clause 45 PHY can have up to 32 layers, each with
64Ki of register space - that's a total of 2Mi of 16-bit registers.

What would be better would be for the MDIO layer to have /dev nodes
that userspace could use to access the bus independent of the PHY,
much the same as we have /dev/i2c-* - but I'm not sure if we really
want to invent a whole new interface to MDIO buses.

The MII interface already exists, works for the most part, and is
already used for Clause 45 PHYs for a number of NICs.  I do agree
that it is less than perfect though.

-- 
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-next 3/5] net: phy: allow Clause 45 access via mii ioctl

[Andrew Lunn]

> > Hi Russell,
> > 
> > I find the SIOCGMIIREG/SIOCGMIIPHY ioctls useful for C45 just as much
> > as they are for C22, but I think the way they work is a big hack and
> > for that reason they're less than useful when you need them most.
> > These ioctls work by hijacking the MDIO bus driver of a PHY that is
> > attached to a net_device. Hence they can be used to access at most a
> > PHY that lies on the same MDIO bus as one you already have a
> > phy-handle to.
> > If you have a PHY issue that makes of_phy_connect fail and the
> > net_device to fail to probe, basically you're SOL because you lose
> > that one handle that userspace had to the MDIO bus.
> > Similarly if you're doing a bring-up and all PHY interfaces are fixed-link.
> > Maybe it would be better to rethink this and expose some sysfs nodes
> > for raw MDIO access in the bus drivers.
> 
> I don't see how putting some attributes in sysfs helps

> What would be better would be for the MDIO layer to have /dev nodes
> that userspace could use to access the bus independent of the PHY,
> much the same as we have /dev/i2c-* - but I'm not sure if we really
> want to invent a whole new interface to MDIO buses.

There is work on re-implementing ethtool using a netlink socket.  The
proposed code supports the current MII interface. However, it should
be possible to extend it to pass some other identifier for the PHY,
rather than going via the network interface it is attached to.

Once ethtool netlink gets merged, i will take a look at this.

     Andrew

[PATCH net-next 4/5] net: sfp: add mandatory attach/detach methods for sfp buses

[Russell King]

Add attach and detach methods for SFP buses, which will allow us to get
rid of the netdev storage in sfp-bus.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 16 ++++++++++++++++
 drivers/net/phy/sfp-bus.c |  4 ++--
 include/linux/sfp.h       |  6 ++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 00cd0ed7ff3d..af02da9e9c1e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1595,6 +1595,20 @@ int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
 }
 EXPORT_SYMBOL_GPL(phylink_mii_ioctl);
 
+static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
+{
+	struct phylink *pl = upstream;
+
+	pl->netdev->sfp_bus = bus;
+}
+
+static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
+{
+	struct phylink *pl = upstream;
+
+	pl->netdev->sfp_bus = NULL;
+}
+
 static int phylink_sfp_module_insert(void *upstream,
 				     const struct sfp_eeprom_id *id)
 {
@@ -1713,6 +1727,8 @@ static void phylink_sfp_disconnect_phy(void *upstream)
 }
 
 static const struct sfp_upstream_ops sfp_phylink_ops = {
+	.attach = phylink_sfp_attach,
+	.detach = phylink_sfp_detach,
 	.module_insert = phylink_sfp_module_insert,
 	.link_up = phylink_sfp_link_up,
 	.link_down = phylink_sfp_link_down,
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index fef701bfad62..c664c905830a 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -350,7 +350,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
 	bus->socket_ops->attach(bus->sfp);
 	if (bus->started)
 		bus->socket_ops->start(bus->sfp);
-	bus->netdev->sfp_bus = bus;
+	bus->upstream_ops->attach(bus->upstream, bus);
 	bus->registered = true;
 	return 0;
 }
@@ -359,8 +359,8 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
 {
 	const struct sfp_upstream_ops *ops = bus->upstream_ops;
 
-	bus->netdev->sfp_bus = NULL;
 	if (bus->registered) {
+		bus->upstream_ops->detach(bus->upstream, bus);
 		if (bus->started)
 			bus->socket_ops->stop(bus->sfp);
 		bus->socket_ops->detach(bus->sfp);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index d9d9de3fcf8e..a3f0336dd703 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -469,6 +469,10 @@ struct sfp_bus;
 
 /**
  * struct sfp_upstream_ops - upstream operations structure
+ * @attach: called when the sfp socket driver is bound to the upstream
+ *   (mandatory).
+ * @detach: called when the sfp socket driver is unbound from the upstream
+ *   (mandatory).
  * @module_insert: called after a module has been detected to determine
  *   whether the module is supported for the upstream device.
  * @module_remove: called after the module has been removed.
@@ -481,6 +485,8 @@ struct sfp_bus;
  *   been removed.
  */
 struct sfp_upstream_ops {
+	void (*attach)(void *priv, struct sfp_bus *bus);
+	void (*detach)(void *priv, struct sfp_bus *bus);
 	int (*module_insert)(void *priv, const struct sfp_eeprom_id *id);
 	void (*module_remove)(void *priv);
 	void (*link_down)(void *priv);
-- 
2.7.4

Re: [PATCH net-next 4/5] net: sfp: add mandatory attach/detach methods for sfp buses

[Andrew Lunn]

On Tue, May 28, 2019 at 10:57:34AM +0100, Russell King wrote:
> Add attach and detach methods for SFP buses, which will allow us to get
> rid of the netdev storage in sfp-bus.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

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

    Andrew

[PATCH net-next 5/5] net: sfp: remove sfp-bus use of netdevs

[Russell King]

The sfp-bus code now no longer has any use for the network device
structure, so remove its use.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c |  3 +--
 drivers/net/phy/sfp-bus.c | 10 +++-------
 include/linux/sfp.h       |  6 ++----
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index af02da9e9c1e..70045c5e116e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -522,8 +522,7 @@ static int phylink_register_sfp(struct phylink *pl,
 		return ret;
 	}
 
-	pl->sfp_bus = sfp_register_upstream(ref.fwnode, pl->netdev, pl,
-					    &sfp_phylink_ops);
+	pl->sfp_bus = sfp_register_upstream(ref.fwnode, pl, &sfp_phylink_ops);
 	if (!pl->sfp_bus)
 		return -ENOMEM;
 
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index c664c905830a..5bc8099eaaf1 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -23,7 +23,6 @@ struct sfp_bus {
 
 	const struct sfp_upstream_ops *upstream_ops;
 	void *upstream;
-	struct net_device *netdev;
 	struct phy_device *phydev;
 
 	bool registered;
@@ -442,13 +441,11 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
 {
 	bus->upstream_ops = NULL;
 	bus->upstream = NULL;
-	bus->netdev = NULL;
 }
 
 /**
  * sfp_register_upstream() - Register the neighbouring device
  * @fwnode: firmware node for the SFP bus
- * @ndev: network device associated with the interface
  * @upstream: the upstream private data
  * @ops: the upstream's &struct sfp_upstream_ops
  *
@@ -459,7 +456,7 @@ static void sfp_upstream_clear(struct sfp_bus *bus)
  * On error, returns %NULL.
  */
 struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
-				      struct net_device *ndev, void *upstream,
+				      void *upstream,
 				      const struct sfp_upstream_ops *ops)
 {
 	struct sfp_bus *bus = sfp_bus_get(fwnode);
@@ -469,7 +466,6 @@ struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
 		rtnl_lock();
 		bus->upstream_ops = ops;
 		bus->upstream = upstream;
-		bus->netdev = ndev;
 
 		if (bus->sfp) {
 			ret = sfp_register_bus(bus);
@@ -591,7 +587,7 @@ struct sfp_bus *sfp_register_socket(struct device *dev, struct sfp *sfp,
 		bus->sfp = sfp;
 		bus->socket_ops = ops;
 
-		if (bus->netdev) {
+		if (bus->upstream_ops) {
 			ret = sfp_register_bus(bus);
 			if (ret)
 				sfp_socket_clear(bus);
@@ -611,7 +607,7 @@ EXPORT_SYMBOL_GPL(sfp_register_socket);
 void sfp_unregister_socket(struct sfp_bus *bus)
 {
 	rtnl_lock();
-	if (bus->netdev)
+	if (bus->upstream_ops)
 		sfp_unregister_bus(bus);
 	sfp_socket_clear(bus);
 	rtnl_unlock();
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index a3f0336dd703..1c35428e98bc 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -464,7 +464,6 @@ enum {
 struct fwnode_handle;
 struct ethtool_eeprom;
 struct ethtool_modinfo;
-struct net_device;
 struct sfp_bus;
 
 /**
@@ -510,7 +509,7 @@ int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
 void sfp_upstream_start(struct sfp_bus *bus);
 void sfp_upstream_stop(struct sfp_bus *bus);
 struct sfp_bus *sfp_register_upstream(struct fwnode_handle *fwnode,
-				      struct net_device *ndev, void *upstream,
+				      void *upstream,
 				      const struct sfp_upstream_ops *ops);
 void sfp_unregister_upstream(struct sfp_bus *bus);
 #else
@@ -555,8 +554,7 @@ static inline void sfp_upstream_stop(struct sfp_bus *bus)
 }
 
 static inline struct sfp_bus *sfp_register_upstream(
-	struct fwnode_handle *fwnode,
-	struct net_device *ndev, void *upstream,
+	struct fwnode_handle *fwnode, void *upstream,
 	const struct sfp_upstream_ops *ops)
 {
 	return (struct sfp_bus *)-1;
-- 
2.7.4

Re: [PATCH net-next 5/5] net: sfp: remove sfp-bus use of netdevs

[Andrew Lunn]

On Tue, May 28, 2019 at 10:57:39AM +0100, Russell King wrote:
> The sfp-bus code now no longer has any use for the network device
> structure, so remove its use.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

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

    Andrew

Re: [PATCH net-next 0/5] phylink/sfp updates

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Tue, 28 May 2019 10:56:39 +0100

> This is a series of updates to phylink and sfp:

Series applied.