[RFC PATCH net-next 0/9] Decoupling PHYLINK from struct net_device

[RFC PATCH net-next 0/9] Decoupling PHYLINK from struct net_device

[Ioana Ciornei]

Following two separate discussion threads in:
  https://www.spinics.net/lists/netdev/msg569087.html
and:
  https://www.spinics.net/lists/netdev/msg570450.html

PHYLINK was reworked in order to add a new "raw" interface, alongside
the one based on struct net_device. The raw interface works by passing
structures to the owner of the phylink instance through a blocking
notifier call.

The event API exposed by the new notifier mechanism is a 1:1 mapping to
the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.

PHYLIB (which PHYLINK uses) was reworked to the extent that it does not
crash when connecting to a PHY and the net_device pointer is NULL.

Lastly, DSA has been reworked in its way that it handles PHYs for ports
that lack a net_device (CPU and DSA ports).  For these, it was
previously using PHYLIB and is now using the PHYLINK raw API.

The implication of the above is that DSA drivers that used to rely on
the .adjust_link to configure their CPU/DSA ports as fixed-link are now
broken.  Full explanation is found in patch 8/9, and a sample fix for
the SJA1105 driver is found in 9/9.  The drivers below are affected:

* b53: Uses .adjust_link for fixed-links on CPU port, as well as
  .phylink_mac_config.
  Migration to 100% PHYLINK does not appear trivial.

* ksz9477: rtl8366rb: mt7530: vsc73xx: Uses .adjust_link exclusively.
  Either the devicetree bindings document or code checks/comments reveal
  that the adjust_link is used for handling a fixed-link.
  Migration to PHYLINK appears trivial.

* qca8k: lan9303: Uses .adjust_link to configure fixed-link ports (skips
  the rest).
  Migration to PHYLINK appears trivial.

The patchset was tested on the NXP LS1021A-TSN board having the
following Ethernet layout:
  https://lkml.org/lkml/2019/5/5/279
The CPU port was moved from the internal RGMII fixed-link (enet2 ->
switch port 4) to an external loopback Cat5 cable between the enet1 port
and the front-facing swp2 SJA1105 port. In this mode, both the master
and the CPU port have an attached PHY which detects link change events:

[   42.785627] fsl-gianfar soc:ethernet@2d50000 eth1: Link is Down
[   43.025678] Broadcom BCM5464 mdio@2d24000:03: Link is Down
[   49.025793] fsl-gianfar soc:ethernet@2d50000 eth1: Link is Up - 1Gbps/Full - flow control off
[   49.266299] Broadcom BCM5464 mdio@2d24000:03: Link is Up - 1Gbps/Full - flow control off

Ioana Ciornei (7):
  net: phy: Guard against the presence of a netdev
  net: phy: Add phy_standalone sysfs entry
  net: phylink: Add phylink_mac_link_{up,down} wrapper functions
  net: phylink: Add phylink_create_raw
  net: phylink: Make fixed link notifier calls edge-triggered
  net: dsa: Move the phylink driver calls into port.c
  net: dsa: Use PHYLINK for the CPU/DSA ports

Vladimir Oltean (2):
  net: phy: Add phy_sysfs_create_links helper function
  net: dsa: sja1105: Fix broken fixed-link interfaces on user ports

 drivers/net/dsa/sja1105/sja1105_main.c |  11 +-
 drivers/net/phy/phy_device.c           |  88 +++++++---
 drivers/net/phy/phylink.c              | 224 ++++++++++++++++++++-----
 include/linux/phylink.h                |  21 +++
 include/net/dsa.h                      |   3 +
 net/dsa/dsa_priv.h                     |  19 +++
 net/dsa/port.c                         | 207 ++++++++++++++++-------
 net/dsa/slave.c                        |  49 +-----
 8 files changed, 441 insertions(+), 181 deletions(-)

-- 
2.21.0

[RFC PATCH net-next 1/9] net: phy: Add phy_sysfs_create_links helper function

[Ioana Ciornei]

From: Vladimir Oltean <olteanv@gmail.com>

This is a cosmetic patch that wraps the operation of creating sysfs
links between the netdev->phydev and the phydev->attached_dev.

This is needed to keep the indentation level in check in a follow-up
patch where this function will be guarded against the existence of a
phydev->attached_dev.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/phy/phy_device.c | 43 ++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index dcc93a873174..5cb01b9db7b5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1133,6 +1133,31 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
 }
 EXPORT_SYMBOL(phy_attached_print);
 
+static void phy_sysfs_create_links(struct phy_device *phydev)
+{
+	struct net_device *dev = phydev->attached_dev;
+	int err;
+
+	err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
+				"attached_dev");
+	if (err)
+		return;
+
+	err = sysfs_create_link_nowarn(&dev->dev.kobj,
+				       &phydev->mdio.dev.kobj,
+				       "phydev");
+	if (err) {
+		dev_err(&dev->dev, "could not add device link to %s err %d\n",
+			kobject_name(&phydev->mdio.dev.kobj),
+			err);
+		/* non-fatal - some net drivers can use one netdevice
+		 * with more then one phy
+		 */
+	}
+
+	phydev->sysfs_links = true;
+}
+
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
@@ -1216,23 +1241,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	 */
 	phydev->sysfs_links = false;
 
-	err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
-				"attached_dev");
-	if (!err) {
-		err = sysfs_create_link_nowarn(&dev->dev.kobj,
-					       &phydev->mdio.dev.kobj,
-					       "phydev");
-		if (err) {
-			dev_err(&dev->dev, "could not add device link to %s err %d\n",
-				kobject_name(&phydev->mdio.dev.kobj),
-				err);
-			/* non-fatal - some net drivers can use one netdevice
-			 * with more then one phy
-			 */
-		}
-
-		phydev->sysfs_links = true;
-	}
+	phy_sysfs_create_links(phydev);
 
 	phydev->dev_flags = flags;
 
-- 
2.21.0

Re: [RFC PATCH net-next 1/9] net: phy: Add phy_sysfs_create_links helper function

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> 
> This is a cosmetic patch that wraps the operation of creating sysfs
> links between the netdev->phydev and the phydev->attached_dev.
> 
> This is needed to keep the indentation level in check in a follow-up
> patch where this function will be guarded against the existence of a
> phydev->attached_dev.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

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

[RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Ioana Ciornei]

A prerequisite for PHYLIB to work in the absence of a struct net_device
is to not access pointers to it.

Changes are needed in the following areas:

 - Printing: In some places netdev_err was replaced with phydev_err.

 - Incrementing reference count to the parent MDIO bus driver: If there
   is no net device, then the reference count should definitely be
   incremented since there is no chance that it was an Ethernet driver
   who registered the MDIO bus.

 - Sysfs links are not created in case there is no attached_dev.

 - No netif_carrier_off is done if there is no attached_dev.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5cb01b9db7b5..25cc7c33f8dd 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1138,6 +1138,9 @@ static void phy_sysfs_create_links(struct phy_device *phydev)
 	struct net_device *dev = phydev->attached_dev;
 	int err;
 
+	if (!dev)
+		return;
+
 	err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
 				"attached_dev");
 	if (err)
@@ -1176,9 +1179,9 @@ static void phy_sysfs_create_links(struct phy_device *phydev)
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface)
 {
-	struct module *ndev_owner = dev->dev.parent->driver->owner;
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct device *d = &phydev->mdio.dev;
+	struct module *ndev_owner = NULL;
 	bool using_genphy = false;
 	int err;
 
@@ -1187,8 +1190,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	 * our own module->refcnt here, otherwise we would not be able to
 	 * unload later on.
 	 */
+	if (dev)
+		ndev_owner = dev->dev.parent->driver->owner;
 	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
-		dev_err(&dev->dev, "failed to get the bus module\n");
+		phydev_err(phydev, "failed to get the bus module\n");
 		return -EIO;
 	}
 
@@ -1207,7 +1212,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	}
 
 	if (!try_module_get(d->driver->owner)) {
-		dev_err(&dev->dev, "failed to get the device driver module\n");
+		phydev_err(phydev, "failed to get the device driver module\n");
 		err = -EIO;
 		goto error_put_device;
 	}
@@ -1228,8 +1233,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	}
 
 	phydev->phy_link_change = phy_link_change;
-	phydev->attached_dev = dev;
-	dev->phydev = phydev;
+	if (dev) {
+		phydev->attached_dev = dev;
+		dev->phydev = phydev;
+	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
 	 * calling register_netdevice() -> netdev_register_kobject() and
@@ -1252,7 +1259,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	/* Initial carrier state is off as the phy is about to be
 	 * (re)initialized.
 	 */
-	netif_carrier_off(phydev->attached_dev);
+	if (dev)
+		netif_carrier_off(phydev->attached_dev);
 
 	/* Do initial configuration here, now that
 	 * we have certain key parameters
@@ -1358,16 +1366,19 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
 void phy_detach(struct phy_device *phydev)
 {
 	struct net_device *dev = phydev->attached_dev;
-	struct module *ndev_owner = dev->dev.parent->driver->owner;
+	struct module *ndev_owner = NULL;
 	struct mii_bus *bus;
 
 	if (phydev->sysfs_links) {
-		sysfs_remove_link(&dev->dev.kobj, "phydev");
+		if (dev)
+			sysfs_remove_link(&dev->dev.kobj, "phydev");
 		sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev");
 	}
 	phy_suspend(phydev);
-	phydev->attached_dev->phydev = NULL;
-	phydev->attached_dev = NULL;
+	if (dev) {
+		phydev->attached_dev->phydev = NULL;
+		phydev->attached_dev = NULL;
+	}
 	phydev->phylink = NULL;
 
 	phy_led_triggers_unregister(phydev);
@@ -1390,6 +1401,8 @@ void phy_detach(struct phy_device *phydev)
 	bus = phydev->mdio.bus;
 
 	put_device(&phydev->mdio.dev);
+	if (dev)
+		ndev_owner = dev->dev.parent->driver->owner;
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);
 
-- 
2.21.0

Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> A prerequisite for PHYLIB to work in the absence of a struct net_device
> is to not access pointers to it.
> 
> Changes are needed in the following areas:
> 
>  - Printing: In some places netdev_err was replaced with phydev_err.
> 
>  - Incrementing reference count to the parent MDIO bus driver: If there
>    is no net device, then the reference count should definitely be
>    incremented since there is no chance that it was an Ethernet driver
>    who registered the MDIO bus.
> 
>  - Sysfs links are not created in case there is no attached_dev.
> 
>  - No netif_carrier_off is done if there is no attached_dev.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

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

Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Andrew Lunn]

On Thu, May 23, 2019 at 01:20:38AM +0000, Ioana Ciornei wrote:
> A prerequisite for PHYLIB to work in the absence of a struct net_device
> is to not access pointers to it.
> 
> Changes are needed in the following areas:
> 
>  - Printing: In some places netdev_err was replaced with phydev_err.
> 
>  - Incrementing reference count to the parent MDIO bus driver: If there
>    is no net device, then the reference count should definitely be
>    incremented since there is no chance that it was an Ethernet driver
>    who registered the MDIO bus.
> 
>  - Sysfs links are not created in case there is no attached_dev.
> 
>  - No netif_carrier_off is done if there is no attached_dev.

Hi Ioana

Looking at the functions changed here, they seem to be related to
phy_attach(), phy_connect(), and phy_detach() etc. Is the intention
you can call these functions and pass a NULL pointer for the
net_device?

	Andrew

RE: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Ioana Ciornei]

> Subject: Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a
> netdev
> 
> On Thu, May 23, 2019 at 01:20:38AM +0000, Ioana Ciornei wrote:
> > A prerequisite for PHYLIB to work in the absence of a struct
> > net_device is to not access pointers to it.
> >
> > Changes are needed in the following areas:
> >
> >  - Printing: In some places netdev_err was replaced with phydev_err.
> >
> >  - Incrementing reference count to the parent MDIO bus driver: If there
> >    is no net device, then the reference count should definitely be
> >    incremented since there is no chance that it was an Ethernet driver
> >    who registered the MDIO bus.
> >
> >  - Sysfs links are not created in case there is no attached_dev.
> >
> >  - No netif_carrier_off is done if there is no attached_dev.
> 
> Hi Ioana
> 
> Looking at the functions changed here, they seem to be related to phy_attach(),
> phy_connect(), and phy_detach() etc. Is the intention you can call these
> functions and pass a NULL pointer for the net_device?
> 
> 	Andrew

Hi Andrew,

Yes, the intention is exactly to pass a NULL pointer for the  net_device from PHYLINK.
The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add phylink_create_raw".

--
Ioana

Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Andrew Lunn]

> > Hi Ioana
> > 
> > Looking at the functions changed here, they seem to be related to phy_attach(),
> > phy_connect(), and phy_detach() etc. Is the intention you can call these
> > functions and pass a NULL pointer for the net_device?
> > 
> > 	Andrew
> 
> Hi Andrew,
> 
> Yes, the intention is exactly to pass a NULL pointer for the  net_device from PHYLINK.
> The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add phylink_create_raw".

Hi Ioana

I think in general, we don't want MAC drivers doing this.

We should enforce that the general APIs get a netdev. PHYLINK uses
phy_attach_direct() which is the lowest level of these attach() and
connect() calls. And there is only one MAC driver using
phy_attach_direct(). So please add checks for the netdev and return
-EINVAL in these higher level callers to phy_attach_direct().

Thanks
	Andrew

RE: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a netdev

[Ioana Ciornei]

> Subject: Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a
> netdev
> 
> > > Hi Ioana
> > >
> > > Looking at the functions changed here, they seem to be related to
> > > phy_attach(), phy_connect(), and phy_detach() etc. Is the intention
> > > you can call these functions and pass a NULL pointer for the net_device?
> > >
> > > 	Andrew
> >
> > Hi Andrew,
> >
> > Yes, the intention is exactly to pass a NULL pointer for the  net_device from
> PHYLINK.
> > The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add
> phylink_create_raw".
> 
> Hi Ioana
> 
> I think in general, we don't want MAC drivers doing this.
> 

Agreed.

> We should enforce that the general APIs get a netdev. PHYLINK uses
> phy_attach_direct() which is the lowest level of these attach() and
> connect() calls. And there is only one MAC driver using phy_attach_direct(). So
> please add checks for the netdev and return -EINVAL in these higher level callers
> to phy_attach_direct().
> 

Will add the checks in v2.

--
Ioana

[RFC PATCH net-next 3/9] net: phy: Add phy_standalone sysfs entry

[Ioana Ciornei]

Export a phy_standalone device attribute that is meant to give the
indication that this PHY lacks an attached_dev and its corresponding
sysfs link.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phy_device.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 25cc7c33f8dd..30e0e73d5f86 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -537,10 +537,22 @@ phy_has_fixups_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_has_fixups);
 
+static ssize_t phy_standalone_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	return sprintf(buf, "%d\n", !phydev->attached_dev);
+}
+
+static DEVICE_ATTR_RO(phy_standalone);
+
 static struct attribute *phy_dev_attrs[] = {
 	&dev_attr_phy_id.attr,
 	&dev_attr_phy_interface.attr,
 	&dev_attr_phy_has_fixups.attr,
+	&dev_attr_phy_standalone.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(phy_dev);
-- 
2.21.0

Re: [RFC PATCH net-next 3/9] net: phy: Add phy_standalone sysfs entry

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> Export a phy_standalone device attribute that is meant to give the
> indication that this PHY lacks an attached_dev and its corresponding
> sysfs link.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>

I would rather have that attribute be conditionally visible/created upon
a PHY device being associated with a NULL net_device and not have it for
"non-standalone" PHY devices, that would be confusing.

> ---
>  drivers/net/phy/phy_device.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 25cc7c33f8dd..30e0e73d5f86 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -537,10 +537,22 @@ phy_has_fixups_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(phy_has_fixups);
>  
> +static ssize_t phy_standalone_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct phy_device *phydev = to_phy_device(dev);
> +
> +	return sprintf(buf, "%d\n", !phydev->attached_dev);
> +}
> +
> +static DEVICE_ATTR_RO(phy_standalone);
> +
>  static struct attribute *phy_dev_attrs[] = {
>  	&dev_attr_phy_id.attr,
>  	&dev_attr_phy_interface.attr,
>  	&dev_attr_phy_has_fixups.attr,
> +	&dev_attr_phy_standalone.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(phy_dev);
> 

-- 
Florian

RE: [RFC PATCH net-next 3/9] net: phy: Add phy_standalone sysfs entry

[Ioana Ciornei]

> Subject: Re: [RFC PATCH net-next 3/9] net: phy: Add phy_standalone sysfs entry
> 
> 
> 
> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> > Export a phy_standalone device attribute that is meant to give the
> > indication that this PHY lacks an attached_dev and its corresponding
> > sysfs link.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> I would rather have that attribute be conditionally visible/created upon a PHY
> device being associated with a NULL net_device and not have it for "non-
> standalone" PHY devices, that would be confusing.
> 

Ok, I understand the reasoning and will change it in v2.

--
Ioana

> > ---
> >  drivers/net/phy/phy_device.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c
> > b/drivers/net/phy/phy_device.c index 25cc7c33f8dd..30e0e73d5f86 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -537,10 +537,22 @@ phy_has_fixups_show(struct device *dev, struct
> > device_attribute *attr,  }  static DEVICE_ATTR_RO(phy_has_fixups);
> >
> > +static ssize_t phy_standalone_show(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   char *buf)
> > +{
> > +	struct phy_device *phydev = to_phy_device(dev);
> > +
> > +	return sprintf(buf, "%d\n", !phydev->attached_dev); }
> > +
> > +static DEVICE_ATTR_RO(phy_standalone);
> > +
> >  static struct attribute *phy_dev_attrs[] = {
> >  	&dev_attr_phy_id.attr,
> >  	&dev_attr_phy_interface.attr,
> >  	&dev_attr_phy_has_fixups.attr,
> > +	&dev_attr_phy_standalone.attr,
> >  	NULL,
> >  };
> >  ATTRIBUTE_GROUPS(phy_dev);
> >
> 
> --
> Florian

[RFC PATCH net-next 4/9] net: phylink: Add phylink_mac_link_{up,down} wrapper functions

[Ioana Ciornei]

This is a cosmetic patch that reduces the clutter in phylink_resolve
around calling the .mac_link_up/.mac_link_down driver callbacks.  In a
further patch this logic will be extended to emit notifications in case
a net device does not exist.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 50 +++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 89750c7dfd6f..68cfe31240cc 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -395,6 +395,34 @@ static const char *phylink_pause_to_str(int pause)
 	}
 }
 
+static void phylink_mac_link_up(struct phylink *pl,
+				struct phylink_link_state link_state)
+{
+	struct net_device *ndev = pl->netdev;
+
+	pl->ops->mac_link_up(ndev, pl->link_an_mode,
+			     pl->phy_state.interface,
+			     pl->phydev);
+
+	netif_carrier_on(ndev);
+
+	netdev_info(ndev,
+		    "Link is Up - %s/%s - flow control %s\n",
+		    phy_speed_to_str(link_state.speed),
+		    phy_duplex_to_str(link_state.duplex),
+		    phylink_pause_to_str(link_state.pause));
+}
+
+static void phylink_mac_link_down(struct phylink *pl)
+{
+	struct net_device *ndev = pl->netdev;
+
+	netif_carrier_off(ndev);
+	pl->ops->mac_link_down(ndev, pl->link_an_mode,
+			       pl->phy_state.interface);
+	netdev_info(ndev, "Link is Down\n");
+}
+
 static void phylink_resolve(struct work_struct *w)
 {
 	struct phylink *pl = container_of(w, struct phylink, resolve);
@@ -450,24 +478,10 @@ static void phylink_resolve(struct work_struct *w)
 	}
 
 	if (link_state.link != netif_carrier_ok(ndev)) {
-		if (!link_state.link) {
-			netif_carrier_off(ndev);
-			pl->ops->mac_link_down(ndev, pl->link_an_mode,
-					       pl->phy_state.interface);
-			netdev_info(ndev, "Link is Down\n");
-		} else {
-			pl->ops->mac_link_up(ndev, pl->link_an_mode,
-					     pl->phy_state.interface,
-					     pl->phydev);
-
-			netif_carrier_on(ndev);
-
-			netdev_info(ndev,
-				    "Link is Up - %s/%s - flow control %s\n",
-				    phy_speed_to_str(link_state.speed),
-				    phy_duplex_to_str(link_state.duplex),
-				    phylink_pause_to_str(link_state.pause));
-		}
+		if (!link_state.link)
+			phylink_mac_link_down(pl);
+		else
+			phylink_mac_link_up(pl, link_state);
 	}
 	if (!link_state.link && pl->mac_link_dropped) {
 		pl->mac_link_dropped = false;
-- 
2.21.0

Re: [RFC PATCH net-next 4/9] net: phylink: Add phylink_mac_link_{up,down} wrapper functions

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> This is a cosmetic patch that reduces the clutter in phylink_resolve
> around calling the .mac_link_up/.mac_link_down driver callbacks.  In a
> further patch this logic will be extended to emit notifications in case
> a net device does not exist.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>

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

[RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Ioana Ciornei]

This adds a new entry point to PHYLINK that does not require a
net_device structure.

The main intended use are DSA ports that do not have net devices
registered for them (mainly because doing so would be redundant - see
Documentation/networking/dsa/dsa.rst for details). So far DSA has been
using PHYLIB fixed PHYs for these ports, driven manually with genphy
instead of starting a full PHY state machine, but this does not scale
well when there are actual PHYs that need a driver on those ports, or
when a fixed-link is requested in DT that has a speed unsupported by the
fixed PHY C22 emulation (such as SGMII-2500).

The proposed solution comes in the form of a notifier chain owned by the
PHYLINK instance, and the passing of phylink_notifier_info structures
back to the driver through a blocking notifier call.

The event API exposed by the new notifier mechanism is a 1:1 mapping to
the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.

Both the standard phylink_create() function, as well as its raw variant,
call the same underlying function which initializes either the netdev
field or the notifier block of the PHYLINK instance.

All PHYLINK driver callbacks have been extended to call the notifier
chain in case the instance is a raw one.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/phy/phylink.c | 189 ++++++++++++++++++++++++++++++--------
 include/linux/phylink.h   |  21 +++++
 2 files changed, 174 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 68cfe31240cc..7b6b233c1a07 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <linux/notifier.h>
 
 #include "sfp.h"
 #include "swphy.h"
@@ -41,6 +42,8 @@ struct phylink {
 	/* private: */
 	struct net_device *netdev;
 	const struct phylink_mac_ops *ops;
+	struct notifier_block *nb;
+	struct blocking_notifier_head notifier_chain;
 
 	unsigned long phylink_disable_state; /* bitmask of disables */
 	struct phy_device *phydev;
@@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode)
 static int phylink_validate(struct phylink *pl, unsigned long *supported,
 			    struct phylink_link_state *state)
 {
-	pl->ops->validate(pl->netdev, supported, state);
+	struct phylink_notifier_info info = {
+		.supported = supported,
+		.state = state,
+	};
+
+	if (pl->ops)
+		pl->ops->validate(pl->netdev, supported, state);
+	else
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_VALIDATE, &info);
 
 	return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
 }
@@ -290,6 +302,12 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
 static void phylink_mac_config(struct phylink *pl,
 			       const struct phylink_link_state *state)
 {
+	struct phylink_notifier_info info = {
+		.link_an_mode = pl->link_an_mode,
+		/* Discard const pointer */
+		.state = (struct phylink_link_state *)state,
+	};
+
 	netdev_dbg(pl->netdev,
 		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
 		   __func__, phylink_an_mode_str(pl->link_an_mode),
@@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
 		   state->pause, state->link, state->an_enabled);
 
-	pl->ops->mac_config(pl->netdev, pl->link_an_mode, state);
+	if (pl->ops)
+		pl->ops->mac_config(pl->netdev, pl->link_an_mode, state);
+	else
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_MAC_CONFIG, &info);
+
 }
 
 static void phylink_mac_config_up(struct phylink *pl,
@@ -311,13 +334,22 @@ static void phylink_mac_config_up(struct phylink *pl,
 
 static void phylink_mac_an_restart(struct phylink *pl)
 {
-	if (pl->link_config.an_enabled &&
-	    phy_interface_mode_is_8023z(pl->link_config.interface))
+	if (!pl->link_config.an_enabled &&
+	    !phy_interface_mode_is_8023z(pl->link_config.interface))
+		return;
+
+	if (pl->ops)
 		pl->ops->mac_an_restart(pl->netdev);
+	else
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_MAC_AN_RESTART, NULL);
 }
 
 static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state *state)
 {
+	struct phylink_notifier_info info = {
+		.state = state,
+	};
 	struct net_device *ndev = pl->netdev;
 
 	linkmode_copy(state->advertising, pl->link_config.advertising);
@@ -330,7 +362,12 @@ static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state *
 	state->an_complete = 0;
 	state->link = 1;
 
-	return pl->ops->mac_link_state(ndev, state);
+	if (pl->ops)
+		return pl->ops->mac_link_state(ndev, state);
+	else
+		return blocking_notifier_call_chain(&pl->notifier_chain,
+						    PHYLINK_MAC_LINK_STATE,
+						    &info);
 }
 
 /* The fixed state is... fixed except for the link state,
@@ -338,9 +375,17 @@ static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state *
  */
 static void phylink_get_fixed_state(struct phylink *pl, struct phylink_link_state *state)
 {
+	struct phylink_notifier_info info = {
+		.state = state,
+	};
+
 	*state = pl->link_config;
 	if (pl->get_fixed_state)
 		pl->get_fixed_state(pl->netdev, state);
+	else if (pl->nb)
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_GET_FIXED_STATE,
+					     &info);
 	else if (pl->link_gpio)
 		state->link = !!gpiod_get_value_cansleep(pl->link_gpio);
 }
@@ -399,28 +444,54 @@ static void phylink_mac_link_up(struct phylink *pl,
 				struct phylink_link_state link_state)
 {
 	struct net_device *ndev = pl->netdev;
+	struct phylink_notifier_info info = {
+		.link_an_mode = pl->link_an_mode,
+		.interface = pl->phy_state.interface,
+		.phydev = pl->phydev,
+	};
 
-	pl->ops->mac_link_up(ndev, pl->link_an_mode,
+	if (pl->ops) {
+		pl->ops->mac_link_up(ndev, pl->link_an_mode,
 			     pl->phy_state.interface,
 			     pl->phydev);
 
-	netif_carrier_on(ndev);
+		netif_carrier_on(ndev);
 
-	netdev_info(ndev,
-		    "Link is Up - %s/%s - flow control %s\n",
-		    phy_speed_to_str(link_state.speed),
-		    phy_duplex_to_str(link_state.duplex),
-		    phylink_pause_to_str(link_state.pause));
+		netdev_info(ndev,
+			    "Link is Up - %s/%s - flow control %s\n",
+			    phy_speed_to_str(link_state.speed),
+			    phy_duplex_to_str(link_state.duplex),
+			    phylink_pause_to_str(link_state.pause));
+	} else {
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_MAC_LINK_UP, &info);
+		phydev_info(pl->phydev,
+			    "Link is Up - %s/%s - flow control %s\n",
+			    phy_speed_to_str(link_state.speed),
+			    phy_duplex_to_str(link_state.duplex),
+			    phylink_pause_to_str(link_state.pause));
+	}
 }
 
 static void phylink_mac_link_down(struct phylink *pl)
 {
 	struct net_device *ndev = pl->netdev;
+	struct phylink_notifier_info info = {
+		.link_an_mode = pl->link_an_mode,
+		.interface = pl->phy_state.interface,
+		.phydev = pl->phydev,
+	};
 
-	netif_carrier_off(ndev);
-	pl->ops->mac_link_down(ndev, pl->link_an_mode,
+	if (pl->ops) {
+		netif_carrier_off(ndev);
+		pl->ops->mac_link_down(ndev, pl->link_an_mode,
 			       pl->phy_state.interface);
-	netdev_info(ndev, "Link is Down\n");
+		netdev_info(ndev, "Link is Down\n");
+	} else {
+		blocking_notifier_call_chain(&pl->notifier_chain,
+					     PHYLINK_MAC_LINK_DOWN, &info);
+		phydev_info(pl->phydev, "Link is Down\n");
+	}
 }
 
 static void phylink_resolve(struct work_struct *w)
@@ -477,7 +548,10 @@ static void phylink_resolve(struct work_struct *w)
 		}
 	}
 
-	if (link_state.link != netif_carrier_ok(ndev)) {
+	/* Take the branch without checking the carrier status
+	 * if there is no netdevice.
+	 */
+	if (!pl->ops || link_state.link != netif_carrier_ok(ndev)) {
 		if (!link_state.link)
 			phylink_mac_link_down(pl);
 		else
@@ -546,24 +620,12 @@ static int phylink_register_sfp(struct phylink *pl,
 	return 0;
 }
 
-/**
- * phylink_create() - create a phylink instance
- * @ndev: a pointer to the &struct net_device
- * @fwnode: a pointer to a &struct fwnode_handle describing the network
- *	interface
- * @iface: the desired link mode defined by &typedef phy_interface_t
- * @ops: a pointer to a &struct phylink_mac_ops for the MAC.
- *
- * Create a new phylink instance, and parse the link parameters found in @np.
- * This will parse in-band modes, fixed-link or SFP configuration.
- *
- * Returns a pointer to a &struct phylink, or an error-pointer value. Users
- * must use IS_ERR() to check for errors from this function.
- */
-struct phylink *phylink_create(struct net_device *ndev,
-			       struct fwnode_handle *fwnode,
-			       phy_interface_t iface,
-			       const struct phylink_mac_ops *ops)
+static inline struct phylink *
+__phylink_create_raw(struct net_device *ndev,
+		     struct notifier_block *nb,
+		     struct fwnode_handle *fwnode,
+		     phy_interface_t iface,
+		     const struct phylink_mac_ops *ops)
 {
 	struct phylink *pl;
 	int ret;
@@ -574,7 +636,17 @@ struct phylink *phylink_create(struct net_device *ndev,
 
 	mutex_init(&pl->state_mutex);
 	INIT_WORK(&pl->resolve, phylink_resolve);
-	pl->netdev = ndev;
+
+	if (ndev) {
+		pl->netdev = ndev;
+	} else if (nb) {
+		BLOCKING_INIT_NOTIFIER_HEAD(&pl->notifier_chain);
+		blocking_notifier_chain_register(&pl->notifier_chain, nb);
+		pl->nb = nb;
+	} else {
+		return ERR_PTR(-EINVAL);
+	}
+
 	pl->phy_state.interface = iface;
 	pl->link_interface = iface;
 	if (iface == PHY_INTERFACE_MODE_MOCA)
@@ -616,8 +688,52 @@ struct phylink *phylink_create(struct net_device *ndev,
 
 	return pl;
 }
+
+/**
+ * phylink_create() - create a phylink instance
+ * @ndev: a pointer to the &struct net_device
+ * @fwnode: a pointer to a &struct fwnode_handle describing the network
+ *	interface
+ * @iface: the desired link mode defined by &typedef phy_interface_t
+ * @ops: a pointer to a &struct phylink_mac_ops for the MAC.
+ *
+ * Create a new phylink instance, and parse the link parameters found in @np.
+ * This will parse in-band modes, fixed-link or SFP configuration.
+ *
+ * Returns a pointer to a &struct phylink, or an error-pointer value. Users
+ * must use IS_ERR() to check for errors from this function.
+ */
+struct phylink *phylink_create(struct net_device *ndev,
+			       struct fwnode_handle *fwnode,
+			       phy_interface_t iface,
+			       const struct phylink_mac_ops *ops)
+{
+	return __phylink_create_raw(ndev, NULL, fwnode, iface, ops);
+}
 EXPORT_SYMBOL_GPL(phylink_create);
 
+/**
+ * phylink_create_raw() - create a raw phylink instance
+ * @nb: a pointer to a struct notifier_block which which will be called on
+ *	on phylink events
+ * @fwnode: a pointer to a &struct fwnode_handle describing the network
+ *	interface
+ * @iface: the desired link mode defined by &typedef phy_interface_t
+ *
+ * Create a new raw phylink instance, and parse the link parameters found in
+ * @np.  This will parse in-band modes, fixed-link or SFP configuration.
+ *
+ * Returns a pointer to a &struct phylink, or an error-pointer value. Users
+ * must use IS_ERR() to check for errors from this function.
+ */
+struct phylink *phylink_create_raw(struct notifier_block *nb,
+				   struct fwnode_handle *fwnode,
+				   phy_interface_t iface)
+{
+	return __phylink_create_raw(NULL, nb, fwnode, iface, NULL);
+}
+EXPORT_SYMBOL_GPL(phylink_create_raw);
+
 /**
  * phylink_destroy() - cleanup and destroy the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -909,7 +1025,8 @@ void phylink_start(struct phylink *pl)
 		    phy_modes(pl->link_config.interface));
 
 	/* Always set the carrier off */
-	netif_carrier_off(pl->netdev);
+	if (pl->netdev)
+		netif_carrier_off(pl->netdev);
 
 	/* Apply the link configuration to the MAC when starting. This allows
 	 * a fixed-link to start with the correct parameters, and also
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6411c624f63a..d171156eab4e 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -54,6 +54,24 @@ struct phylink_link_state {
 	unsigned int an_complete:1;
 };
 
+enum phylink_cmd {
+	PHYLINK_VALIDATE = 1,
+	PHYLINK_MAC_LINK_STATE,
+	PHYLINK_MAC_AN_RESTART,
+	PHYLINK_MAC_CONFIG,
+	PHYLINK_MAC_LINK_DOWN,
+	PHYLINK_MAC_LINK_UP,
+	PHYLINK_GET_FIXED_STATE,
+};
+
+struct phylink_notifier_info {
+	struct phylink_link_state *state;
+	unsigned long *supported;
+	u8 link_an_mode;
+	phy_interface_t interface;
+	struct phy_device *phydev;
+};
+
 /**
  * struct phylink_mac_ops - MAC operations structure.
  * @validate: Validate and update the link configuration.
@@ -200,6 +218,9 @@ void mac_link_up(struct net_device *ndev, unsigned int mode,
 
 struct phylink *phylink_create(struct net_device *, struct fwnode_handle *,
 	phy_interface_t iface, const struct phylink_mac_ops *ops);
+struct phylink *phylink_create_raw(struct notifier_block *nb,
+				   struct fwnode_handle *fwnode,
+				   phy_interface_t iface);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
-- 
2.21.0

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> This adds a new entry point to PHYLINK that does not require a
> net_device structure.
> 
> The main intended use are DSA ports that do not have net devices
> registered for them (mainly because doing so would be redundant - see
> Documentation/networking/dsa/dsa.rst for details). So far DSA has been
> using PHYLIB fixed PHYs for these ports, driven manually with genphy
> instead of starting a full PHY state machine, but this does not scale
> well when there are actual PHYs that need a driver on those ports, or
> when a fixed-link is requested in DT that has a speed unsupported by the
> fixed PHY C22 emulation (such as SGMII-2500).
> 
> The proposed solution comes in the form of a notifier chain owned by the
> PHYLINK instance, and the passing of phylink_notifier_info structures
> back to the driver through a blocking notifier call.
> 
> The event API exposed by the new notifier mechanism is a 1:1 mapping to
> the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
> 
> Both the standard phylink_create() function, as well as its raw variant,
> call the same underlying function which initializes either the netdev
> field or the notifier block of the PHYLINK instance.
> 
> All PHYLINK driver callbacks have been extended to call the notifier
> chain in case the instance is a raw one.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---

[snip]

> +	struct phylink_notifier_info info = {
> +		.link_an_mode = pl->link_an_mode,
> +		/* Discard const pointer */
> +		.state = (struct phylink_link_state *)state,
> +	};
> +
>  	netdev_dbg(pl->netdev,
>  		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>  		   __func__, phylink_an_mode_str(pl->link_an_mode),
> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
>  		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>  		   state->pause, state->link, state->an_enabled);

Don't you need to guard that netdev_dbg() with an if (pl->ops) to avoid
de-referencing a NULL net_device?

Another possibility could be to change the signature of the
phylink_mac_ops to take an opaque pointer and in the case where we
called phylink_create() and passed down a net_device pointer, we somehow
remember that for doing any operation that requires a net_device
(printing, setting carrier). We lose strict typing in doing that, but
we'd have fewer places to patch for a blocking notifier call.
-- 
Florian

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Florian Fainelli]

On 5/22/2019 7:25 PM, Florian Fainelli wrote:
> 
> 
> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
>> This adds a new entry point to PHYLINK that does not require a
>> net_device structure.
>>
>> The main intended use are DSA ports that do not have net devices
>> registered for them (mainly because doing so would be redundant - see
>> Documentation/networking/dsa/dsa.rst for details). So far DSA has been
>> using PHYLIB fixed PHYs for these ports, driven manually with genphy
>> instead of starting a full PHY state machine, but this does not scale
>> well when there are actual PHYs that need a driver on those ports, or
>> when a fixed-link is requested in DT that has a speed unsupported by the
>> fixed PHY C22 emulation (such as SGMII-2500).
>>
>> The proposed solution comes in the form of a notifier chain owned by the
>> PHYLINK instance, and the passing of phylink_notifier_info structures
>> back to the driver through a blocking notifier call.
>>
>> The event API exposed by the new notifier mechanism is a 1:1 mapping to
>> the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
>>
>> Both the standard phylink_create() function, as well as its raw variant,
>> call the same underlying function which initializes either the netdev
>> field or the notifier block of the PHYLINK instance.
>>
>> All PHYLINK driver callbacks have been extended to call the notifier
>> chain in case the instance is a raw one.
>>
>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>> ---
> 
> [snip]
> 
>> +	struct phylink_notifier_info info = {
>> +		.link_an_mode = pl->link_an_mode,
>> +		/* Discard const pointer */
>> +		.state = (struct phylink_link_state *)state,
>> +	};
>> +
>>  	netdev_dbg(pl->netdev,
>>  		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>>  		   __func__, phylink_an_mode_str(pl->link_an_mode),
>> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
>>  		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>>  		   state->pause, state->link, state->an_enabled);
> 
> Don't you need to guard that netdev_dbg() with an if (pl->ops) to avoid
> de-referencing a NULL net_device?
> 
> Another possibility could be to change the signature of the
> phylink_mac_ops to take an opaque pointer and in the case where we
> called phylink_create() and passed down a net_device pointer, we somehow
> remember that for doing any operation that requires a net_device
> (printing, setting carrier). We lose strict typing in doing that, but
> we'd have fewer places to patch for a blocking notifier call.
> 

Or even make those functions part of phylink_mac_ops such that the
caller could pass an .carrier_ok callback which is netif_carrier_ok()
for a net_device, else it's NULL, same with printing functions if desired...
-- 
Florian

RE: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Ioana Ciornei]

> Subject: Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw
> 
> 
> 
> On 5/22/2019 7:25 PM, Florian Fainelli wrote:
> >
> >
> > On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> >> This adds a new entry point to PHYLINK that does not require a
> >> net_device structure.
> >>
> >> The main intended use are DSA ports that do not have net devices
> >> registered for them (mainly because doing so would be redundant - see
> >> Documentation/networking/dsa/dsa.rst for details). So far DSA has
> >> been using PHYLIB fixed PHYs for these ports, driven manually with
> >> genphy instead of starting a full PHY state machine, but this does
> >> not scale well when there are actual PHYs that need a driver on those
> >> ports, or when a fixed-link is requested in DT that has a speed
> >> unsupported by the fixed PHY C22 emulation (such as SGMII-2500).
> >>
> >> The proposed solution comes in the form of a notifier chain owned by
> >> the PHYLINK instance, and the passing of phylink_notifier_info
> >> structures back to the driver through a blocking notifier call.
> >>
> >> The event API exposed by the new notifier mechanism is a 1:1 mapping
> >> to the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
> >>
> >> Both the standard phylink_create() function, as well as its raw
> >> variant, call the same underlying function which initializes either
> >> the netdev field or the notifier block of the PHYLINK instance.
> >>
> >> All PHYLINK driver callbacks have been extended to call the notifier
> >> chain in case the instance is a raw one.
> >>
> >> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> >> ---
> >
> > [snip]
> >
> >> +	struct phylink_notifier_info info = {
> >> +		.link_an_mode = pl->link_an_mode,
> >> +		/* Discard const pointer */
> >> +		.state = (struct phylink_link_state *)state,
> >> +	};
> >> +
> >>  	netdev_dbg(pl->netdev,
> >>  		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u
> an=%u\n",
> >>  		   __func__, phylink_an_mode_str(pl->link_an_mode),
> >> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
> >>  		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
> >>  		   state->pause, state->link, state->an_enabled);
> >
> > Don't you need to guard that netdev_dbg() with an if (pl->ops) to
> > avoid de-referencing a NULL net_device?
> >


The netdev_* print will not dereference a NULL net_device since it has explicit checks agains this.
Instead it will just print (net/core/dev.c, __netdev_printk):

	printk("%s(NULL net_device): %pV", level, vaf);


> > Another possibility could be to change the signature of the
> > phylink_mac_ops to take an opaque pointer and in the case where we
> > called phylink_create() and passed down a net_device pointer, we
> > somehow remember that for doing any operation that requires a
> > net_device (printing, setting carrier). We lose strict typing in doing
> > that, but we'd have fewer places to patch for a blocking notifier call.
> >
> 
> Or even make those functions part of phylink_mac_ops such that the caller
> could pass an .carrier_ok callback which is netif_carrier_ok() for a net_device,
> else it's NULL, same with printing functions if desired...
> --
> Florian


Let me see if I understood this correctly. I presume that any API that we add should not break any current PHYLINK users.

You suggest to change the prototype of the phylink_mac_ops from

	void (*validate)(struct net_device *ndev, unsigned long *supported,
			 struct phylink_link_state *state);

to something that takes a void pointer:

	void (*validate)(void *dev, unsigned long *supported,
			 struct phylink_link_state *state);

This would imply that the any function in PHYLINK would have to somehow differentiate if the dev provided is indeed a net_device or another structure in order to make the decision if netif_carrier_off should be called or not (this is so we do not break any drivers using PHYLINK). I cannot see how this judgement can be made.

--
Ioana

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Florian Fainelli]

On 5/23/2019 5:10 AM, Ioana Ciornei wrote:
> 
>> Subject: Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw
>>
>>
>>
>> On 5/22/2019 7:25 PM, Florian Fainelli wrote:
>>>
>>>
>>> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
>>>> This adds a new entry point to PHYLINK that does not require a
>>>> net_device structure.
>>>>
>>>> The main intended use are DSA ports that do not have net devices
>>>> registered for them (mainly because doing so would be redundant - see
>>>> Documentation/networking/dsa/dsa.rst for details). So far DSA has
>>>> been using PHYLIB fixed PHYs for these ports, driven manually with
>>>> genphy instead of starting a full PHY state machine, but this does
>>>> not scale well when there are actual PHYs that need a driver on those
>>>> ports, or when a fixed-link is requested in DT that has a speed
>>>> unsupported by the fixed PHY C22 emulation (such as SGMII-2500).
>>>>
>>>> The proposed solution comes in the form of a notifier chain owned by
>>>> the PHYLINK instance, and the passing of phylink_notifier_info
>>>> structures back to the driver through a blocking notifier call.
>>>>
>>>> The event API exposed by the new notifier mechanism is a 1:1 mapping
>>>> to the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
>>>>
>>>> Both the standard phylink_create() function, as well as its raw
>>>> variant, call the same underlying function which initializes either
>>>> the netdev field or the notifier block of the PHYLINK instance.
>>>>
>>>> All PHYLINK driver callbacks have been extended to call the notifier
>>>> chain in case the instance is a raw one.
>>>>
>>>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>>>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +	struct phylink_notifier_info info = {
>>>> +		.link_an_mode = pl->link_an_mode,
>>>> +		/* Discard const pointer */
>>>> +		.state = (struct phylink_link_state *)state,
>>>> +	};
>>>> +
>>>>  	netdev_dbg(pl->netdev,
>>>>  		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u
>> an=%u\n",
>>>>  		   __func__, phylink_an_mode_str(pl->link_an_mode),
>>>> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
>>>>  		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>>>>  		   state->pause, state->link, state->an_enabled);
>>>
>>> Don't you need to guard that netdev_dbg() with an if (pl->ops) to
>>> avoid de-referencing a NULL net_device?
>>>
> 
> 
> The netdev_* print will not dereference a NULL net_device since it has explicit checks agains this.
> Instead it will just print (net/core/dev.c, __netdev_printk):
> 
> 	printk("%s(NULL net_device): %pV", level, vaf);
> 
> 
>>> Another possibility could be to change the signature of the
>>> phylink_mac_ops to take an opaque pointer and in the case where we
>>> called phylink_create() and passed down a net_device pointer, we
>>> somehow remember that for doing any operation that requires a
>>> net_device (printing, setting carrier). We lose strict typing in doing
>>> that, but we'd have fewer places to patch for a blocking notifier call.
>>>
>>
>> Or even make those functions part of phylink_mac_ops such that the caller
>> could pass an .carrier_ok callback which is netif_carrier_ok() for a net_device,
>> else it's NULL, same with printing functions if desired...
>> --
>> Florian
> 
> 
> Let me see if I understood this correctly. I presume that any API that we add should not break any current PHYLINK users.
> 
> You suggest to change the prototype of the phylink_mac_ops from
> 
> 	void (*validate)(struct net_device *ndev, unsigned long *supported,
> 			 struct phylink_link_state *state);
> 
> to something that takes a void pointer:
> 
> 	void (*validate)(void *dev, unsigned long *supported,
> 			 struct phylink_link_state *state);

That is what I am suggesting, but I am also suggesting passing all
netdev specific calls that must be made as callbacks as well, so
something like:

	bool (*carrier_ok)(const void *dev)
	void (*carrier_set)(const void *dev, bool on)
	void (*print)(const void *dev, const char *fmt)

as new members of phylink_mac_ops.

> 
> This would imply that the any function in PHYLINK would have to somehow differentiate if the dev provided is indeed a net_device or another structure in order to make the decision if netif_carrier_off should be called or not (this is so we do not break any drivers using PHYLINK). I cannot see how this judgement can be made.

You don't have to make the judgement you can just do:

if (pl->ops->carrier_set)
	pl->ops->carrier_set(dev,

where dev was this opaque pointer passed to phylink_create() the first
time it was created. Like I wrote, we lose strong typing doing that, but
we don't have to update all code paths for if (pl->ops) else notifier.
-- 
Florian

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Vladimir Oltean]

On 5/23/19 5:32 PM, Florian Fainelli wrote:
> 
> On 5/23/2019 5:10 AM, Ioana Ciornei wrote:
>>
>>> Subject: Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw
>>>
>>>
>>>
>>> On 5/22/2019 7:25 PM, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
>>>>> This adds a new entry point to PHYLINK that does not require a
>>>>> net_device structure.
>>>>>
>>>>> The main intended use are DSA ports that do not have net devices
>>>>> registered for them (mainly because doing so would be redundant - see
>>>>> Documentation/networking/dsa/dsa.rst for details). So far DSA has
>>>>> been using PHYLIB fixed PHYs for these ports, driven manually with
>>>>> genphy instead of starting a full PHY state machine, but this does
>>>>> not scale well when there are actual PHYs that need a driver on those
>>>>> ports, or when a fixed-link is requested in DT that has a speed
>>>>> unsupported by the fixed PHY C22 emulation (such as SGMII-2500).
>>>>>
>>>>> The proposed solution comes in the form of a notifier chain owned by
>>>>> the PHYLINK instance, and the passing of phylink_notifier_info
>>>>> structures back to the driver through a blocking notifier call.
>>>>>
>>>>> The event API exposed by the new notifier mechanism is a 1:1 mapping
>>>>> to the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
>>>>>
>>>>> Both the standard phylink_create() function, as well as its raw
>>>>> variant, call the same underlying function which initializes either
>>>>> the netdev field or the notifier block of the PHYLINK instance.
>>>>>
>>>>> All PHYLINK driver callbacks have been extended to call the notifier
>>>>> chain in case the instance is a raw one.
>>>>>
>>>>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>>>>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +	struct phylink_notifier_info info = {
>>>>> +		.link_an_mode = pl->link_an_mode,
>>>>> +		/* Discard const pointer */
>>>>> +		.state = (struct phylink_link_state *)state,
>>>>> +	};
>>>>> +
>>>>>   	netdev_dbg(pl->netdev,
>>>>>   		   "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u
>>> an=%u\n",
>>>>>   		   __func__, phylink_an_mode_str(pl->link_an_mode),
>>>>> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
>>>>>   		   __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>>>>>   		   state->pause, state->link, state->an_enabled);
>>>>
>>>> Don't you need to guard that netdev_dbg() with an if (pl->ops) to
>>>> avoid de-referencing a NULL net_device?
>>>>
>>
>>
>> The netdev_* print will not dereference a NULL net_device since it has explicit checks agains this.
>> Instead it will just print (net/core/dev.c, __netdev_printk):
>>
>> 	printk("%s(NULL net_device): %pV", level, vaf);
>>
>>
>>>> Another possibility could be to change the signature of the
>>>> phylink_mac_ops to take an opaque pointer and in the case where we
>>>> called phylink_create() and passed down a net_device pointer, we
>>>> somehow remember that for doing any operation that requires a
>>>> net_device (printing, setting carrier). We lose strict typing in doing
>>>> that, but we'd have fewer places to patch for a blocking notifier call.
>>>>
>>>
>>> Or even make those functions part of phylink_mac_ops such that the caller
>>> could pass an .carrier_ok callback which is netif_carrier_ok() for a net_device,
>>> else it's NULL, same with printing functions if desired...
>>> --
>>> Florian
>>
>>
>> Let me see if I understood this correctly. I presume that any API that we add should not break any current PHYLINK users.
>>
>> You suggest to change the prototype of the phylink_mac_ops from
>>
>> 	void (*validate)(struct net_device *ndev, unsigned long *supported,
>> 			 struct phylink_link_state *state);
>>
>> to something that takes a void pointer:
>>
>> 	void (*validate)(void *dev, unsigned long *supported,
>> 			 struct phylink_link_state *state);
> 
> That is what I am suggesting, but I am also suggesting passing all
> netdev specific calls that must be made as callbacks as well, so
> something like:
> 
> 	bool (*carrier_ok)(const void *dev)
> 	void (*carrier_set)(const void *dev, bool on)
> 	void (*print)(const void *dev, const char *fmt)
> 
> as new members of phylink_mac_ops.
> 
>>
>> This would imply that the any function in PHYLINK would have to somehow differentiate if the dev provided is indeed a net_device or another structure in order to make the decision if netif_carrier_off should be called or not (this is so we do not break any drivers using PHYLINK). I cannot see how this judgement can be made.
> 
> You don't have to make the judgement you can just do:
> 
> if (pl->ops->carrier_set)
> 	pl->ops->carrier_set(dev,
> 
> where dev was this opaque pointer passed to phylink_create() the first
> time it was created. Like I wrote, we lose strong typing doing that, but
> we don't have to update all code paths for if (pl->ops) else notifier.
> 

Hi Florian,

Have you thought this through?
What about the totally random stuff, such as this portion from 2/9:

> @@ -1187,8 +1190,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	 * our own module->refcnt here, otherwise we would not be able to
>  	 * unload later on.
>  	 */
> +	if (dev)
> +		ndev_owner = dev->dev.parent->driver->owner;
>  	if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
> -		dev_err(&dev->dev, "failed to get the bus module\n");
> +		phydev_err(phydev, "failed to get the bus module\n");
>  		return -EIO;
>  	}

Which is in PHYLIB by the way.
Do you just add a pl->ops->owns_mdio_bus() callback? What if that code 
goes away in the future? Do you remove it? This is code that all users 
of phylink_create_raw will have to implement.

IMO the whole point is to change as little as possible from PHYLINK's 
surface, and nothing from PHYLIB's. What you're suggesting is to change 
everything, *including* phylib. And PHYLINK's print callback can't be 
used in PHYLIB unless struct phylink is made public.

And if you want to replace "struct net_device *ndev" with "const void 
*dev", how will you even assign the phydev->attached_dev->phydev 
backlink? Another callback?

As for carrier state - realistically I don't see how any raw PHYLINK 
user would implement it any other way except keep a variable for it. 
Hence just let PHYLINK do it once.

I fail to see how this is cleaner.

-Vladimir

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Florian Fainelli]

On 5/23/19 1:32 PM, Vladimir Oltean wrote:
> On 5/23/19 5:32 PM, Florian Fainelli wrote:
>>
>> On 5/23/2019 5:10 AM, Ioana Ciornei wrote:
>>>
>>>> Subject: Re: [RFC PATCH net-next 5/9] net: phylink: Add
>>>> phylink_create_raw
>>>>
>>>>
>>>>
>>>> On 5/22/2019 7:25 PM, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
>>>>>> This adds a new entry point to PHYLINK that does not require a
>>>>>> net_device structure.
>>>>>>
>>>>>> The main intended use are DSA ports that do not have net devices
>>>>>> registered for them (mainly because doing so would be redundant - see
>>>>>> Documentation/networking/dsa/dsa.rst for details). So far DSA has
>>>>>> been using PHYLIB fixed PHYs for these ports, driven manually with
>>>>>> genphy instead of starting a full PHY state machine, but this does
>>>>>> not scale well when there are actual PHYs that need a driver on those
>>>>>> ports, or when a fixed-link is requested in DT that has a speed
>>>>>> unsupported by the fixed PHY C22 emulation (such as SGMII-2500).
>>>>>>
>>>>>> The proposed solution comes in the form of a notifier chain owned by
>>>>>> the PHYLINK instance, and the passing of phylink_notifier_info
>>>>>> structures back to the driver through a blocking notifier call.
>>>>>>
>>>>>> The event API exposed by the new notifier mechanism is a 1:1 mapping
>>>>>> to the existing PHYLINK mac_ops, plus the PHYLINK fixed-link
>>>>>> callback.
>>>>>>
>>>>>> Both the standard phylink_create() function, as well as its raw
>>>>>> variant, call the same underlying function which initializes either
>>>>>> the netdev field or the notifier block of the PHYLINK instance.
>>>>>>
>>>>>> All PHYLINK driver callbacks have been extended to call the notifier
>>>>>> chain in case the instance is a raw one.
>>>>>>
>>>>>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>>>>>> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>>>>>> ---
>>>>>
>>>>> [snip]
>>>>>
>>>>>> +    struct phylink_notifier_info info = {
>>>>>> +        .link_an_mode = pl->link_an_mode,
>>>>>> +        /* Discard const pointer */
>>>>>> +        .state = (struct phylink_link_state *)state,
>>>>>> +    };
>>>>>> +
>>>>>>       netdev_dbg(pl->netdev,
>>>>>>              "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u
>>>> an=%u\n",
>>>>>>              __func__, phylink_an_mode_str(pl->link_an_mode),
>>>>>> @@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink
>>>>>> *pl,
>>>>>>              __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>>>>>>              state->pause, state->link, state->an_enabled);
>>>>>
>>>>> Don't you need to guard that netdev_dbg() with an if (pl->ops) to
>>>>> avoid de-referencing a NULL net_device?
>>>>>
>>>
>>>
>>> The netdev_* print will not dereference a NULL net_device since it
>>> has explicit checks agains this.
>>> Instead it will just print (net/core/dev.c, __netdev_printk):
>>>
>>>     printk("%s(NULL net_device): %pV", level, vaf);
>>>
>>>
>>>>> Another possibility could be to change the signature of the
>>>>> phylink_mac_ops to take an opaque pointer and in the case where we
>>>>> called phylink_create() and passed down a net_device pointer, we
>>>>> somehow remember that for doing any operation that requires a
>>>>> net_device (printing, setting carrier). We lose strict typing in doing
>>>>> that, but we'd have fewer places to patch for a blocking notifier
>>>>> call.
>>>>>
>>>>
>>>> Or even make those functions part of phylink_mac_ops such that the
>>>> caller
>>>> could pass an .carrier_ok callback which is netif_carrier_ok() for a
>>>> net_device,
>>>> else it's NULL, same with printing functions if desired...
>>>> -- 
>>>> Florian
>>>
>>>
>>> Let me see if I understood this correctly. I presume that any API
>>> that we add should not break any current PHYLINK users.
>>>
>>> You suggest to change the prototype of the phylink_mac_ops from
>>>
>>>     void (*validate)(struct net_device *ndev, unsigned long *supported,
>>>              struct phylink_link_state *state);
>>>
>>> to something that takes a void pointer:
>>>
>>>     void (*validate)(void *dev, unsigned long *supported,
>>>              struct phylink_link_state *state);
>>
>> That is what I am suggesting, but I am also suggesting passing all
>> netdev specific calls that must be made as callbacks as well, so
>> something like:
>>
>>     bool (*carrier_ok)(const void *dev)
>>     void (*carrier_set)(const void *dev, bool on)
>>     void (*print)(const void *dev, const char *fmt)
>>
>> as new members of phylink_mac_ops.
>>
>>>
>>> This would imply that the any function in PHYLINK would have to
>>> somehow differentiate if the dev provided is indeed a net_device or
>>> another structure in order to make the decision if netif_carrier_off
>>> should be called or not (this is so we do not break any drivers using
>>> PHYLINK). I cannot see how this judgement can be made.
>>
>> You don't have to make the judgement you can just do:
>>
>> if (pl->ops->carrier_set)
>>     pl->ops->carrier_set(dev,
>>
>> where dev was this opaque pointer passed to phylink_create() the first
>> time it was created. Like I wrote, we lose strong typing doing that, but
>> we don't have to update all code paths for if (pl->ops) else notifier.
>>
> 
> Hi Florian,
> 
> Have you thought this through?

Not to the point of seeing the problems you are highlighting.

> What about the totally random stuff, such as this portion from 2/9:
> 
>> @@ -1187,8 +1190,10 @@ int phy_attach_direct(struct net_device *dev,
>> struct phy_device *phydev,
>>       * our own module->refcnt here, otherwise we would not be able to
>>       * unload later on.
>>       */
>> +    if (dev)
>> +        ndev_owner = dev->dev.parent->driver->owner;
>>      if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
>> -        dev_err(&dev->dev, "failed to get the bus module\n");
>> +        phydev_err(phydev, "failed to get the bus module\n");
>>          return -EIO;
>>      }
> 
> Which is in PHYLIB by the way.
> Do you just add a pl->ops->owns_mdio_bus() callback? What if that code
> goes away in the future? Do you remove it? This is code that all users
> of phylink_create_raw will have to implement.
> 
> IMO the whole point is to change as little as possible from PHYLINK's
> surface, and nothing from PHYLIB's. What you're suggesting is to change
> everything, *including* phylib. And PHYLINK's print callback can't be
> used in PHYLIB unless struct phylink is made public.
> 
> And if you want to replace "struct net_device *ndev" with "const void
> *dev", how will you even assign the phydev->attached_dev->phydev
> backlink? Another callback?
> 
> As for carrier state - realistically I don't see how any raw PHYLINK
> user would implement it any other way except keep a variable for it.
> Hence just let PHYLINK do it once.
> 
> I fail to see how this is cleaner.
Fine, it's not.
-- 
Florian

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Russell King - ARM Linux admin]

On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote:
> @@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode)
>  static int phylink_validate(struct phylink *pl, unsigned long *supported,
>  			    struct phylink_link_state *state)
>  {
> -	pl->ops->validate(pl->netdev, supported, state);
> +	struct phylink_notifier_info info = {
> +		.supported = supported,
> +		.state = state,
> +	};
> +
> +	if (pl->ops)
> +		pl->ops->validate(pl->netdev, supported, state);
> +	else
> +		blocking_notifier_call_chain(&pl->notifier_chain,
> +					     PHYLINK_VALIDATE, &info);

I don't like this use of notifiers for several reasons:

1. It becomes harder to grep for users of this.
2. We lose documentation about what is passed for each method.
3. We lose const-ness for parameters, which then allows users to
   modify phylink-internal data structures inappropriately from
   these notifier calls.

Please find another way.

-- 
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: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Vladimir Oltean]

On Fri, 24 May 2019 at 00:28, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote:
> > @@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode)
> >  static int phylink_validate(struct phylink *pl, unsigned long *supported,
> >                           struct phylink_link_state *state)
> >  {
> > -     pl->ops->validate(pl->netdev, supported, state);
> > +     struct phylink_notifier_info info = {
> > +             .supported = supported,
> > +             .state = state,
> > +     };
> > +
> > +     if (pl->ops)
> > +             pl->ops->validate(pl->netdev, supported, state);
> > +     else
> > +             blocking_notifier_call_chain(&pl->notifier_chain,
> > +                                          PHYLINK_VALIDATE, &info);
>
> I don't like this use of notifiers for several reasons:
>
> 1. It becomes harder to grep for users of this.
> 2. We lose documentation about what is passed for each method.
> 3. We lose const-ness for parameters, which then allows users to
>    modify phylink-internal data structures inappropriately from
>    these notifier calls.
>
> Please find another way.
>
> --
> 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

Hi Russell,

Items 2 and 3 can be addressed by creating an union of structures in
struct phylink_notifier_info, just like switchdev does.
For 1 (grep), you mean that the notifiers are upper-case and the
regular callbacks are lower-case?

-Vladimir

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Russell King - ARM Linux admin]

On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote:
> +	if (pl->ops) {
> +		pl->ops->mac_link_up(ndev, pl->link_an_mode,
>  			     pl->phy_state.interface,
>  			     pl->phydev);
>  
> +		netif_carrier_on(ndev);
>  
> +		netdev_info(ndev,
> +			    "Link is Up - %s/%s - flow control %s\n",
> +			    phy_speed_to_str(link_state.speed),
> +			    phy_duplex_to_str(link_state.duplex),
> +			    phylink_pause_to_str(link_state.pause));
> +	} else {
> +		blocking_notifier_call_chain(&pl->notifier_chain,
> +					     PHYLINK_MAC_LINK_UP, &info);
> +		phydev_info(pl->phydev,
> +			    "Link is Up - %s/%s - flow control %s\n",
> +			    phy_speed_to_str(link_state.speed),
> +			    phy_duplex_to_str(link_state.duplex),
> +			    phylink_pause_to_str(link_state.pause));
> +	}

So if we don't have pl->ops, what happens when we call phydev_info()
with a NULL phydev, which is a very real possibility: one of phylink's
whole points is to support dynamic presence of a PHY.

What will happen in that case is this will oops, due to dereferencing
an offset NULL pointer via:

#define phydev_info(_phydev, format, args...)	\
	dev_info(&_phydev->mdio.dev, format, ##args)

You can't just decide that if there's no netdev, we will be guaranteed
a phy.

-- 
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: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Vladimir Oltean]

On Fri, 24 May 2019 at 00:55, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote:
> > +     if (pl->ops) {
> > +             pl->ops->mac_link_up(ndev, pl->link_an_mode,
> >                            pl->phy_state.interface,
> >                            pl->phydev);
> >
> > +             netif_carrier_on(ndev);
> >
> > +             netdev_info(ndev,
> > +                         "Link is Up - %s/%s - flow control %s\n",
> > +                         phy_speed_to_str(link_state.speed),
> > +                         phy_duplex_to_str(link_state.duplex),
> > +                         phylink_pause_to_str(link_state.pause));
> > +     } else {
> > +             blocking_notifier_call_chain(&pl->notifier_chain,
> > +                                          PHYLINK_MAC_LINK_UP, &info);
> > +             phydev_info(pl->phydev,
> > +                         "Link is Up - %s/%s - flow control %s\n",
> > +                         phy_speed_to_str(link_state.speed),
> > +                         phy_duplex_to_str(link_state.duplex),
> > +                         phylink_pause_to_str(link_state.pause));
> > +     }
>
> So if we don't have pl->ops, what happens when we call phydev_info()
> with a NULL phydev, which is a very real possibility: one of phylink's
> whole points is to support dynamic presence of a PHY.
>
> What will happen in that case is this will oops, due to dereferencing
> an offset NULL pointer via:
>
> #define phydev_info(_phydev, format, args...)   \
>         dev_info(&_phydev->mdio.dev, format, ##args)
>
> You can't just decide that if there's no netdev, we will be guaranteed
> a phy.
>
> --
> 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

True, however it does not crash:

[    2.539949] (NULL device *): Link is Up - 1Gbps/Full - flow control off

I agree that a better printing system has to be established though.

-Vladimir

Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw

[Russell King - ARM Linux admin]

On Fri, May 24, 2019 at 01:04:01AM +0300, Vladimir Oltean wrote:
> On Fri, 24 May 2019 at 00:55, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote:
> > > +     if (pl->ops) {
> > > +             pl->ops->mac_link_up(ndev, pl->link_an_mode,
> > >                            pl->phy_state.interface,
> > >                            pl->phydev);
> > >
> > > +             netif_carrier_on(ndev);
> > >
> > > +             netdev_info(ndev,
> > > +                         "Link is Up - %s/%s - flow control %s\n",
> > > +                         phy_speed_to_str(link_state.speed),
> > > +                         phy_duplex_to_str(link_state.duplex),
> > > +                         phylink_pause_to_str(link_state.pause));
> > > +     } else {
> > > +             blocking_notifier_call_chain(&pl->notifier_chain,
> > > +                                          PHYLINK_MAC_LINK_UP, &info);
> > > +             phydev_info(pl->phydev,
> > > +                         "Link is Up - %s/%s - flow control %s\n",
> > > +                         phy_speed_to_str(link_state.speed),
> > > +                         phy_duplex_to_str(link_state.duplex),
> > > +                         phylink_pause_to_str(link_state.pause));
> > > +     }
> >
> > So if we don't have pl->ops, what happens when we call phydev_info()
> > with a NULL phydev, which is a very real possibility: one of phylink's
> > whole points is to support dynamic presence of a PHY.
> >
> > What will happen in that case is this will oops, due to dereferencing
> > an offset NULL pointer via:
> >
> > #define phydev_info(_phydev, format, args...)   \
> >         dev_info(&_phydev->mdio.dev, format, ##args)
> >
> > You can't just decide that if there's no netdev, we will be guaranteed
> > a phy.
> >
> > --
> > 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
> 
> True, however it does not crash:
> 
> [    2.539949] (NULL device *): Link is Up - 1Gbps/Full - flow control off
> 
> I agree that a better printing system has to be established though.

The only reason that happens is because struct mdio_device is at the
start of struct phy_device, and struct device is at the start of
struct mdio_device.

Should either of these move, that breaks and we get an oops.  Sorry,
that's way too fragile.

Plus, of course, do we think that printing "(NULL device *):" is
really acceptable?  We completely lose any information about _what_
link came up or went down.

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

[RFC PATCH net-next 7/9] net: dsa: Move the phylink driver calls into port.c

[Ioana Ciornei]

In order to have a common handling of PHYLINK for the slave and non-user
ports, the DSA core glue logic (between PHYLINK and the driver) must use
an API that does not rely on a struct net_device.

These will also be called by the CPU-port-handling code in a further
patch.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/dsa_priv.h | 19 +++++++++
 net/dsa/port.c     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 net/dsa/slave.c    | 49 ++++-------------------
 3 files changed, 122 insertions(+), 42 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 8f1222324646..da70da65bdc5 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -161,6 +161,25 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
 int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags);
 int dsa_port_vid_del(struct dsa_port *dp, u16 vid);
+void dsa_port_phylink_validate(struct dsa_port *dp,
+			       unsigned long *supported,
+			       struct phylink_link_state *state);
+int dsa_port_phylink_mac_link_state(struct dsa_port *dp,
+				    struct phylink_link_state *state);
+void dsa_port_phylink_mac_config(struct dsa_port *dp,
+				 unsigned int mode,
+				 const struct phylink_link_state *state);
+void dsa_port_phylink_mac_an_restart(struct dsa_port *dp);
+void dsa_port_phylink_mac_link_down(struct dsa_port *dp,
+				    unsigned int mode,
+				    phy_interface_t interface,
+				    struct phy_device *phydev);
+void dsa_port_phylink_mac_link_up(struct dsa_port *dp,
+				  unsigned int mode,
+				  phy_interface_t interface,
+				  struct phy_device *phydev);
+void dsa_port_phylink_fixed_state(struct dsa_port *dp,
+				  struct phylink_link_state *state);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed8ba9daa3ba..d0f955e8b731 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -422,6 +422,102 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
 	return phydev;
 }
 
+void dsa_port_phylink_validate(struct dsa_port *dp,
+			       unsigned long *supported,
+			       struct phylink_link_state *state)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->phylink_validate)
+		return;
+
+	ds->ops->phylink_validate(ds, dp->index, supported, state);
+}
+EXPORT_SYMBOL(dsa_port_phylink_validate);
+
+int dsa_port_phylink_mac_link_state(struct dsa_port *dp,
+				    struct phylink_link_state *state)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	/* Only called for SGMII and 802.3z */
+	if (!ds->ops->phylink_mac_link_state)
+		return -EOPNOTSUPP;
+
+	return ds->ops->phylink_mac_link_state(ds, dp->index, state);
+}
+EXPORT_SYMBOL(dsa_port_phylink_mac_link_state);
+
+void dsa_port_phylink_mac_config(struct dsa_port *dp,
+				 unsigned int mode,
+				 const struct phylink_link_state *state)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->phylink_mac_config)
+		return;
+
+	ds->ops->phylink_mac_config(ds, dp->index, mode, state);
+}
+EXPORT_SYMBOL(dsa_port_phylink_mac_config);
+
+void dsa_port_phylink_mac_an_restart(struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->phylink_mac_an_restart)
+		return;
+
+	ds->ops->phylink_mac_an_restart(ds, dp->index);
+}
+EXPORT_SYMBOL(dsa_port_phylink_mac_an_restart);
+
+void dsa_port_phylink_mac_link_down(struct dsa_port *dp,
+				    unsigned int mode,
+				    phy_interface_t interface,
+				    struct phy_device *phydev)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->phylink_mac_link_down) {
+		if (ds->ops->adjust_link && phydev)
+			ds->ops->adjust_link(ds, dp->index, phydev);
+		return;
+	}
+
+	ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface);
+}
+EXPORT_SYMBOL(dsa_port_phylink_mac_link_down);
+
+void dsa_port_phylink_mac_link_up(struct dsa_port *dp,
+				  unsigned int mode,
+				  phy_interface_t interface,
+				  struct phy_device *phydev)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->phylink_mac_link_up) {
+		if (ds->ops->adjust_link && phydev)
+			ds->ops->adjust_link(ds, dp->index, phydev);
+		return;
+	}
+
+	ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev);
+}
+EXPORT_SYMBOL(dsa_port_phylink_mac_link_up);
+
+void dsa_port_phylink_fixed_state(struct dsa_port *dp,
+				  struct phylink_link_state *state)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	/* No need to check that this operation is valid, the callback would
+	 * not be called if it was not.
+	 */
+	ds->ops->phylink_fixed_state(ds, dp->index, state);
+}
+EXPORT_SYMBOL(dsa_port_phylink_fixed_state);
+
 static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
 {
 	struct dsa_switch *ds = dp->ds;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9892ca1f6859..308066da8a0f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1169,25 +1169,16 @@ static void dsa_slave_phylink_validate(struct net_device *dev,
 				       struct phylink_link_state *state)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_validate)
-		return;
 
-	ds->ops->phylink_validate(ds, dp->index, supported, state);
+	dsa_port_phylink_validate(dp, supported, state);
 }
 
 static int dsa_slave_phylink_mac_link_state(struct net_device *dev,
 					    struct phylink_link_state *state)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
-
-	/* Only called for SGMII and 802.3z */
-	if (!ds->ops->phylink_mac_link_state)
-		return -EOPNOTSUPP;
 
-	return ds->ops->phylink_mac_link_state(ds, dp->index, state);
+	return dsa_port_phylink_mac_link_state(dp, state);
 }
 
 static void dsa_slave_phylink_mac_config(struct net_device *dev,
@@ -1195,23 +1186,15 @@ static void dsa_slave_phylink_mac_config(struct net_device *dev,
 					 const struct phylink_link_state *state)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_config)
-		return;
 
-	ds->ops->phylink_mac_config(ds, dp->index, mode, state);
+	dsa_port_phylink_mac_config(dp, mode, state);
 }
 
 static void dsa_slave_phylink_mac_an_restart(struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_an_restart)
-		return;
 
-	ds->ops->phylink_mac_an_restart(ds, dp->index);
+	dsa_port_phylink_mac_an_restart(dp);
 }
 
 static void dsa_slave_phylink_mac_link_down(struct net_device *dev,
@@ -1219,15 +1202,8 @@ static void dsa_slave_phylink_mac_link_down(struct net_device *dev,
 					    phy_interface_t interface)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!ds->ops->phylink_mac_link_down) {
-		if (ds->ops->adjust_link && dev->phydev)
-			ds->ops->adjust_link(ds, dp->index, dev->phydev);
-		return;
-	}
 
-	ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface);
+	dsa_port_phylink_mac_link_down(dp, mode, interface, dev->phydev);
 }
 
 static void dsa_slave_phylink_mac_link_up(struct net_device *dev,
@@ -1236,15 +1212,8 @@ static void dsa_slave_phylink_mac_link_up(struct net_device *dev,
 					  struct phy_device *phydev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
 
-	if (!ds->ops->phylink_mac_link_up) {
-		if (ds->ops->adjust_link && dev->phydev)
-			ds->ops->adjust_link(ds, dp->index, dev->phydev);
-		return;
-	}
-
-	ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev);
+	dsa_port_phylink_mac_link_up(dp, mode, interface, phydev);
 }
 
 static const struct phylink_mac_ops dsa_slave_phylink_mac_ops = {
@@ -1268,12 +1237,8 @@ static void dsa_slave_phylink_fixed_state(struct net_device *dev,
 					  struct phylink_link_state *state)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct dsa_switch *ds = dp->ds;
 
-	/* No need to check that this operation is valid, the callback would
-	 * not be called if it was not.
-	 */
-	ds->ops->phylink_fixed_state(ds, dp->index, state);
+	dsa_port_phylink_fixed_state(dp, state);
 }
 
 /* slave device setup *******************************************************/
-- 
2.21.0

Re: [RFC PATCH net-next 7/9] net: dsa: Move the phylink driver calls into port.c

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> In order to have a common handling of PHYLINK for the slave and non-user
> ports, the DSA core glue logic (between PHYLINK and the driver) must use
> an API that does not rely on a struct net_device.
> 
> These will also be called by the CPU-port-handling code in a further
> patch.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> ---

[snip]

> +void dsa_port_phylink_validate(struct dsa_port *dp,
> +			       unsigned long *supported,
> +			       struct phylink_link_state *state)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_validate)
> +		return;
> +
> +	ds->ops->phylink_validate(ds, dp->index, supported, state);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_validate);

Those exports should probably be _GPL to follow the PHYLINK exports but
other than that:

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

Re: [RFC PATCH net-next 7/9] net: dsa: Move the phylink driver calls into port.c

[Russell King - ARM Linux admin]

On Thu, May 23, 2019 at 01:20:41AM +0000, Ioana Ciornei wrote:
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index ed8ba9daa3ba..d0f955e8b731 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -422,6 +422,102 @@ static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
>  	return phydev;
>  }
>  
> +void dsa_port_phylink_validate(struct dsa_port *dp,
> +			       unsigned long *supported,
> +			       struct phylink_link_state *state)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_validate)
> +		return;

No, really not acceptable.  If there's no phylink_validate function,
then simply returning is going to produce what I'd term "unpredictable"
results.  This callback will be called with various modes in
"supported", and if you simply return without any modification,
you're basically saying that you support _anything_ that the supported
mask throws at you.

> +
> +	ds->ops->phylink_validate(ds, dp->index, supported, state);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_validate);
> +
> +int dsa_port_phylink_mac_link_state(struct dsa_port *dp,
> +				    struct phylink_link_state *state)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	/* Only called for SGMII and 802.3z */
> +	if (!ds->ops->phylink_mac_link_state)
> +		return -EOPNOTSUPP;
> +
> +	return ds->ops->phylink_mac_link_state(ds, dp->index, state);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_mac_link_state);
> +
> +void dsa_port_phylink_mac_config(struct dsa_port *dp,
> +				 unsigned int mode,
> +				 const struct phylink_link_state *state)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_mac_config)
> +		return;

If you don't implement this, what's the point?

> +
> +	ds->ops->phylink_mac_config(ds, dp->index, mode, state);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_mac_config);
> +
> +void dsa_port_phylink_mac_an_restart(struct dsa_port *dp)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_mac_an_restart)
> +		return;
> +
> +	ds->ops->phylink_mac_an_restart(ds, dp->index);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_mac_an_restart);
> +
> +void dsa_port_phylink_mac_link_down(struct dsa_port *dp,
> +				    unsigned int mode,
> +				    phy_interface_t interface,
> +				    struct phy_device *phydev)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_mac_link_down) {
> +		if (ds->ops->adjust_link && phydev)
> +			ds->ops->adjust_link(ds, dp->index, phydev);
> +		return;
> +	}
> +
> +	ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_mac_link_down);
> +
> +void dsa_port_phylink_mac_link_up(struct dsa_port *dp,
> +				  unsigned int mode,
> +				  phy_interface_t interface,
> +				  struct phy_device *phydev)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	if (!ds->ops->phylink_mac_link_up) {
> +		if (ds->ops->adjust_link && phydev)
> +			ds->ops->adjust_link(ds, dp->index, phydev);
> +		return;
> +	}
> +
> +	ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_mac_link_up);
> +
> +void dsa_port_phylink_fixed_state(struct dsa_port *dp,
> +				  struct phylink_link_state *state)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +
> +	/* No need to check that this operation is valid, the callback would
> +	 * not be called if it was not.
> +	 */
> +	ds->ops->phylink_fixed_state(ds, dp->index, state);
> +}
> +EXPORT_SYMBOL(dsa_port_phylink_fixed_state);
> +
>  static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
>  {
>  	struct dsa_switch *ds = dp->ds;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 9892ca1f6859..308066da8a0f 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1169,25 +1169,16 @@ static void dsa_slave_phylink_validate(struct net_device *dev,
>  				       struct phylink_link_state *state)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	if (!ds->ops->phylink_validate)
> -		return;

Ah, this is where it came from - but still wrong for the reasons I
stated above, though it makes it not a bug you're introducing, but
one that pre-exists.

>  
> -	ds->ops->phylink_validate(ds, dp->index, supported, state);
> +	dsa_port_phylink_validate(dp, supported, state);
>  }
>  
>  static int dsa_slave_phylink_mac_link_state(struct net_device *dev,
>  					    struct phylink_link_state *state)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	/* Only called for SGMII and 802.3z */
> -	if (!ds->ops->phylink_mac_link_state)
> -		return -EOPNOTSUPP;
>  
> -	return ds->ops->phylink_mac_link_state(ds, dp->index, state);
> +	return dsa_port_phylink_mac_link_state(dp, state);
>  }
>  
>  static void dsa_slave_phylink_mac_config(struct net_device *dev,
> @@ -1195,23 +1186,15 @@ static void dsa_slave_phylink_mac_config(struct net_device *dev,
>  					 const struct phylink_link_state *state)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	if (!ds->ops->phylink_mac_config)
> -		return;
>  
> -	ds->ops->phylink_mac_config(ds, dp->index, mode, state);
> +	dsa_port_phylink_mac_config(dp, mode, state);
>  }
>  
>  static void dsa_slave_phylink_mac_an_restart(struct net_device *dev)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	if (!ds->ops->phylink_mac_an_restart)
> -		return;
>  
> -	ds->ops->phylink_mac_an_restart(ds, dp->index);
> +	dsa_port_phylink_mac_an_restart(dp);
>  }
>  
>  static void dsa_slave_phylink_mac_link_down(struct net_device *dev,
> @@ -1219,15 +1202,8 @@ static void dsa_slave_phylink_mac_link_down(struct net_device *dev,
>  					    phy_interface_t interface)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
> -
> -	if (!ds->ops->phylink_mac_link_down) {
> -		if (ds->ops->adjust_link && dev->phydev)
> -			ds->ops->adjust_link(ds, dp->index, dev->phydev);
> -		return;
> -	}
>  
> -	ds->ops->phylink_mac_link_down(ds, dp->index, mode, interface);
> +	dsa_port_phylink_mac_link_down(dp, mode, interface, dev->phydev);
>  }
>  
>  static void dsa_slave_phylink_mac_link_up(struct net_device *dev,
> @@ -1236,15 +1212,8 @@ static void dsa_slave_phylink_mac_link_up(struct net_device *dev,
>  					  struct phy_device *phydev)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
>  
> -	if (!ds->ops->phylink_mac_link_up) {
> -		if (ds->ops->adjust_link && dev->phydev)
> -			ds->ops->adjust_link(ds, dp->index, dev->phydev);
> -		return;
> -	}
> -
> -	ds->ops->phylink_mac_link_up(ds, dp->index, mode, interface, phydev);
> +	dsa_port_phylink_mac_link_up(dp, mode, interface, phydev);
>  }
>  
>  static const struct phylink_mac_ops dsa_slave_phylink_mac_ops = {
> @@ -1268,12 +1237,8 @@ static void dsa_slave_phylink_fixed_state(struct net_device *dev,
>  					  struct phylink_link_state *state)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	struct dsa_switch *ds = dp->ds;
>  
> -	/* No need to check that this operation is valid, the callback would
> -	 * not be called if it was not.
> -	 */
> -	ds->ops->phylink_fixed_state(ds, dp->index, state);
> +	dsa_port_phylink_fixed_state(dp, state);
>  }
>  
>  /* slave device setup *******************************************************/
> -- 
> 2.21.0
> 
> 

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

[RFC PATCH net-next 6/9] net: phylink: Make fixed link notifier calls edge-triggered

[Ioana Ciornei]

Otherwise .mac_link_up and .mac_link_down on fixed-link interfaces are
called every second.

On a raw phylink instance we cannot call netdev_carrier_ok for an
indication of the last state of the link, so keep track and update this
in a new field of the phylink structure.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7b6b233c1a07..3bc91b249990 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -44,6 +44,7 @@ struct phylink {
 	const struct phylink_mac_ops *ops;
 	struct notifier_block *nb;
 	struct blocking_notifier_head notifier_chain;
+	unsigned int old_link_state:1;
 
 	unsigned long phylink_disable_state; /* bitmask of disables */
 	struct phy_device *phydev;
@@ -499,6 +500,7 @@ static void phylink_resolve(struct work_struct *w)
 	struct phylink *pl = container_of(w, struct phylink, resolve);
 	struct phylink_link_state link_state;
 	struct net_device *ndev = pl->netdev;
+	int link_changed;
 
 	mutex_lock(&pl->state_mutex);
 	if (pl->phylink_disable_state) {
@@ -548,10 +550,13 @@ static void phylink_resolve(struct work_struct *w)
 		}
 	}
 
-	/* Take the branch without checking the carrier status
-	 * if there is no netdevice.
-	 */
-	if (!pl->ops || link_state.link != netif_carrier_ok(ndev)) {
+	if (pl->ops)
+		link_changed = (link_state.link != netif_carrier_ok(ndev));
+	else
+		link_changed = (link_state.link != pl->old_link_state);
+
+	if (link_changed) {
+		pl->old_link_state = link_state.link;
 		if (!link_state.link)
 			phylink_mac_link_down(pl);
 		else
-- 
2.21.0

[RFC PATCH net-next 8/9] net: dsa: Use PHYLINK for the CPU/DSA ports

[Ioana Ciornei]

This completely removes the usage of PHYLIB from DSA, namely for the
aforementioned switch ports which used to drive a software PHY manually
using genphy operations.

For these ports, the newly introduced phylink_create_raw API must be
used, and the callbacks are received through a notifier block registered
per dsa_port, but otherwise the implementation is fairly
straightforward, and the handling of the regular vs raw PHYLINK
instances is common from the perspective of the driver.

What changes for drivers:

The .adjust_link callback is no longer called for the fixed-link CPU/DSA
ports and drivers must migrate to standard PHYLINK operations (e.g.
.phylink_mac_config).  The reason why we can't do anything for them is
because PHYLINK does not wrap the fixed link state behind a phydev
object, so we cannot wrap .phylink_mac_config into .adjust_link unless
we fabricate a phy_device structure.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 include/net/dsa.h |   3 +
 net/dsa/port.c    | 137 +++++++++++++++++++++-------------------------
 2 files changed, 64 insertions(+), 76 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 685294817712..87616ff00919 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -212,6 +212,9 @@ struct dsa_port {
 	 * Original copy of the master netdev net_device_ops
 	 */
 	const struct net_device_ops *orig_ndo_ops;
+
+	/* Listener for phylink events on ports with no netdev */
+	struct notifier_block	nb;
 };
 
 struct dsa_switch {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index d0f955e8b731..31bd07dd42db 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -13,6 +13,7 @@
 #include <linux/if_bridge.h>
 #include <linux/notifier.h>
 #include <linux/of_mdio.h>
+#include <linux/phylink.h>
 #include <linux/of_net.h>
 
 #include "dsa_priv.h"
@@ -511,104 +512,88 @@ void dsa_port_phylink_fixed_state(struct dsa_port *dp,
 {
 	struct dsa_switch *ds = dp->ds;
 
-	/* No need to check that this operation is valid, the callback would
-	 * not be called if it was not.
+	/* We need to check that the callback exists because phylink raw will
+	 * send PHYLINK_GET_FIXED_STATE events without an explicit register.
 	 */
-	ds->ops->phylink_fixed_state(ds, dp->index, state);
+	if (ds->ops->phylink_fixed_state)
+		ds->ops->phylink_fixed_state(ds, dp->index, state);
 }
 EXPORT_SYMBOL(dsa_port_phylink_fixed_state);
 
-static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
-{
-	struct dsa_switch *ds = dp->ds;
-	struct phy_device *phydev;
-	int port = dp->index;
-	int err = 0;
-
-	phydev = dsa_port_get_phy_device(dp);
-	if (!phydev)
-		return 0;
-
-	if (IS_ERR(phydev))
-		return PTR_ERR(phydev);
-
-	if (enable) {
-		err = genphy_config_init(phydev);
-		if (err < 0)
-			goto err_put_dev;
-
-		err = genphy_resume(phydev);
-		if (err < 0)
-			goto err_put_dev;
-
-		err = genphy_read_status(phydev);
-		if (err < 0)
-			goto err_put_dev;
-	} else {
-		err = genphy_suspend(phydev);
-		if (err < 0)
-			goto err_put_dev;
+static int dsa_cpu_port_event(struct notifier_block *nb,
+			      unsigned long event, void *ptr)
+{
+	struct phylink_notifier_info *info = ptr;
+	struct dsa_port *dp = container_of(nb, struct dsa_port, nb);
+
+	switch (event) {
+	case PHYLINK_VALIDATE:
+		dsa_port_phylink_validate(dp, info->supported, info->state);
+		break;
+	case PHYLINK_MAC_AN_RESTART:
+		dsa_port_phylink_mac_an_restart(dp);
+		break;
+	case PHYLINK_MAC_CONFIG:
+		dsa_port_phylink_mac_config(dp, info->link_an_mode,
+					    info->state);
+		break;
+	case PHYLINK_MAC_LINK_DOWN:
+		dsa_port_phylink_mac_link_down(dp, info->link_an_mode,
+					       info->interface, info->phydev);
+		break;
+	case PHYLINK_MAC_LINK_UP:
+		dsa_port_phylink_mac_link_up(dp, info->link_an_mode,
+					     info->interface, info->phydev);
+		break;
+	case PHYLINK_GET_FIXED_STATE:
+		dsa_port_phylink_fixed_state(dp, info->state);
+		break;
+	default:
+		return NOTIFY_OK;
 	}
 
-	if (ds->ops->adjust_link)
-		ds->ops->adjust_link(ds, port, phydev);
-
-	dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev));
-
-err_put_dev:
-	put_device(&phydev->mdio.dev);
-	return err;
+	return NOTIFY_DONE;
 }
 
-static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
+int dsa_port_link_register_of(struct dsa_port *dp)
 {
-	struct device_node *dn = dp->dn;
-	struct dsa_switch *ds = dp->ds;
-	struct phy_device *phydev;
-	int port = dp->index;
-	int mode;
-	int err;
+	struct device_node *port_dn = dp->dn;
+	int mode, err;
 
-	err = of_phy_register_fixed_link(dn);
-	if (err) {
-		dev_err(ds->dev,
-			"failed to register the fixed PHY of port %d\n",
-			port);
-		return err;
-	}
-
-	phydev = of_phy_find_device(dn);
-
-	mode = of_get_phy_mode(dn);
+	mode = of_get_phy_mode(port_dn);
 	if (mode < 0)
 		mode = PHY_INTERFACE_MODE_NA;
-	phydev->interface = mode;
 
-	genphy_config_init(phydev);
-	genphy_read_status(phydev);
+	dp->nb.notifier_call = dsa_cpu_port_event;
+	dp->pl = phylink_create_raw(&dp->nb, of_fwnode_handle(port_dn), mode);
+	if (IS_ERR(dp->pl)) {
+		pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl));
+		return PTR_ERR(dp->pl);
+	}
 
-	if (ds->ops->adjust_link)
-		ds->ops->adjust_link(ds, port, phydev);
+	err = phylink_of_phy_connect(dp->pl, port_dn, 0);
+	if (err) {
+		pr_err("could not attach to PHY: %d\n", err);
+		goto err_phy_connect;
+	}
 
-	put_device(&phydev->mdio.dev);
+	rtnl_lock();
+	phylink_start(dp->pl);
+	rtnl_unlock();
 
 	return 0;
-}
 
-int dsa_port_link_register_of(struct dsa_port *dp)
-{
-	if (of_phy_is_fixed_link(dp->dn))
-		return dsa_port_fixed_link_register_of(dp);
-	else
-		return dsa_port_setup_phy_of(dp, true);
+err_phy_connect:
+	phylink_destroy(dp->pl);
+	return err;
 }
 
 void dsa_port_link_unregister_of(struct dsa_port *dp)
 {
-	if (of_phy_is_fixed_link(dp->dn))
-		of_phy_deregister_fixed_link(dp->dn);
-	else
-		dsa_port_setup_phy_of(dp, false);
+	rtnl_lock();
+	phylink_disconnect_phy(dp->pl);
+	rtnl_unlock();
+	phylink_destroy(dp->pl);
 }
 
 int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data)
-- 
2.21.0

Re: [RFC PATCH net-next 8/9] net: dsa: Use PHYLINK for the CPU/DSA ports

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> This completely removes the usage of PHYLIB from DSA, namely for the
> aforementioned switch ports which used to drive a software PHY manually
> using genphy operations.
> 
> For these ports, the newly introduced phylink_create_raw API must be
> used, and the callbacks are received through a notifier block registered
> per dsa_port, but otherwise the implementation is fairly
> straightforward, and the handling of the regular vs raw PHYLINK
> instances is common from the perspective of the driver.
> 
> What changes for drivers:
> 
> The .adjust_link callback is no longer called for the fixed-link CPU/DSA
> ports and drivers must migrate to standard PHYLINK operations (e.g.
> .phylink_mac_config).  The reason why we can't do anything for them is
> because PHYLINK does not wrap the fixed link state behind a phydev
> object, so we cannot wrap .phylink_mac_config into .adjust_link unless
> we fabricate a phy_device structure.

Can't we offer a slightly nicer transition period for DSA switch drivers:

- if adjust_link and phylink_mac_ops are both supported, prefer
phylink_mac_ops
- if phylink_mac_ops is defined alone use it, we're good
- if adjust_link alone is defined, keep using it with the existing code
but print a warning inviting to migrate?

The changes look fine but the transition path needs to be a little more
gentle IMHO.
-- 
Florian

Re: [RFC PATCH net-next 8/9] net: dsa: Use PHYLINK for the CPU/DSA ports

[Vladimir Oltean]

On Thu, 23 May 2019 at 05:17, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> > This completely removes the usage of PHYLIB from DSA, namely for the
> > aforementioned switch ports which used to drive a software PHY manually
> > using genphy operations.
> >
> > For these ports, the newly introduced phylink_create_raw API must be
> > used, and the callbacks are received through a notifier block registered
> > per dsa_port, but otherwise the implementation is fairly
> > straightforward, and the handling of the regular vs raw PHYLINK
> > instances is common from the perspective of the driver.
> >
> > What changes for drivers:
> >
> > The .adjust_link callback is no longer called for the fixed-link CPU/DSA
> > ports and drivers must migrate to standard PHYLINK operations (e.g.
> > .phylink_mac_config).  The reason why we can't do anything for them is
> > because PHYLINK does not wrap the fixed link state behind a phydev
> > object, so we cannot wrap .phylink_mac_config into .adjust_link unless
> > we fabricate a phy_device structure.
>
> Can't we offer a slightly nicer transition period for DSA switch drivers:
>
> - if adjust_link and phylink_mac_ops are both supported, prefer
> phylink_mac_ops
> - if phylink_mac_ops is defined alone use it, we're good
> - if adjust_link alone is defined, keep using it with the existing code
> but print a warning inviting to migrate?
>
> The changes look fine but the transition path needs to be a little more
> gentle IMHO.
> --
> Florian

Hi Florian,

Yes we could, but since most of the adjust_link -> phylink_mac_ops
changes appear trivial, and we have the knowledge behind b53 right
here, can't we just migrate everything in the next patchset and remove
adjust_link altogether from DSA?
...So why does b53 configure the SGMII SerDes in phylink_mac_config,
and RGMII delays for the CPU port in adjust_link? Why are pause frames
only configured for the CPU port?
A migration from my side would be rather mechanical, so could you
please share some suggestions?

-Vladimir

Re: [RFC PATCH net-next 8/9] net: dsa: Use PHYLINK for the CPU/DSA ports

[Andrew Lunn]

> Hi Florian,
> 
> Yes we could, but since most of the adjust_link -> phylink_mac_ops
> changes appear trivial, and we have the knowledge behind b53 right
> here, can't we just migrate everything in the next patchset and remove
> adjust_link altogether from DSA?

I agree with Florian, we either need to support both, or their needs
to be another patchset which comes first and converts all DSA drivers
to PHYLINK. And it is this conversion patchset which is likely to
break things, so it would be good to sit in net-next for a week or two
to allow testing, before the second patchset is applied.

   Andrew

Re: [RFC PATCH net-next 8/9] net: dsa: Use PHYLINK for the CPU/DSA ports

[Vladimir Oltean]

On Fri, 24 May 2019 at 16:19, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Hi Florian,
> >
> > Yes we could, but since most of the adjust_link -> phylink_mac_ops
> > changes appear trivial, and we have the knowledge behind b53 right
> > here, can't we just migrate everything in the next patchset and remove
> > adjust_link altogether from DSA?
>
> I agree with Florian, we either need to support both, or their needs
> to be another patchset which comes first and converts all DSA drivers
> to PHYLINK. And it is this conversion patchset which is likely to
> break things, so it would be good to sit in net-next for a week or two
> to allow testing, before the second patchset is applied.
>
>    Andrew

Hi Andrew,

I think that converting drivers to PHYLINK in a separate patchset is
going to introduce useless work, since the complete migration to
PHYLINK is necessarily going to take 2 steps. As of now, the CPU and
DSA ports still use the PHYLIB adjust_link callback exclusively.
So let's see what the drivers need to do in adjust_link now and how to
map that over phylink_mac_config, and in v2 we can just remove the
adjust_link wrappers completely from DSA.

-Vladimir

[RFC PATCH net-next 9/9] net: dsa: sja1105: Fix broken fixed-link interfaces on user ports

[Ioana Ciornei]

From: Vladimir Oltean <olteanv@gmail.com>

PHYLIB and PHYLINK handle fixed-link interfaces differently. PHYLIB
wraps them in a software PHY ("pseudo fixed link") phydev construct such
that .adjust_link driver callbacks see an unified API. Whereas PHYLINK
simply creates a phylink_link_state structure and passes it to
.mac_config.

At the time the driver was introduced, DSA was using PHYLIB for the
CPU/cascade ports (the ones with no net devices) and PHYLINK for
everything else.

As explained below:

commit aab9c4067d2389d0adfc9c53806437df7b0fe3d5
Author: Florian Fainelli <f.fainelli@gmail.com>
Date:   Thu May 10 13:17:36 2018 -0700

  net: dsa: Plug in PHYLINK support

  Drivers that utilize fixed links for user-facing ports (e.g: bcm_sf2)
  will need to implement phylink_mac_ops from now on to preserve
  functionality, since PHYLINK *does not* create a phy_device instance
  for fixed links.

In the above patch, DSA guards the .phylink_mac_config callback against
a NULL phydev pointer.  Therefore, .adjust_link is not called in case of
a fixed-link user port.

This patch fixes the situation by converting the driver from using
.adjust_link to .phylink_mac_config.  This can be done now in a unified
fashion for both slave and CPU/cascade ports because DSA now uses
PHYLINK for all ports.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 0663b78a2f6c..cfdefd9f1905 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -734,15 +734,16 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
 	return sja1105_clocking_setup_port(priv, port);
 }
 
-static void sja1105_adjust_link(struct dsa_switch *ds, int port,
-				struct phy_device *phydev)
+static void sja1105_mac_config(struct dsa_switch *ds, int port,
+			       unsigned int link_an_mode,
+			       const struct phylink_link_state *state)
 {
 	struct sja1105_private *priv = ds->priv;
 
-	if (!phydev->link)
+	if (!state->link)
 		sja1105_adjust_port_config(priv, port, 0, false);
 	else
-		sja1105_adjust_port_config(priv, port, phydev->speed, true);
+		sja1105_adjust_port_config(priv, port, state->speed, true);
 }
 
 static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
@@ -1515,9 +1516,9 @@ static int sja1105_set_ageing_time(struct dsa_switch *ds,
 static const struct dsa_switch_ops sja1105_switch_ops = {
 	.get_tag_protocol	= sja1105_get_tag_protocol,
 	.setup			= sja1105_setup,
-	.adjust_link		= sja1105_adjust_link,
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.phylink_validate	= sja1105_phylink_validate,
+	.phylink_mac_config	= sja1105_mac_config,
 	.get_strings		= sja1105_get_strings,
 	.get_ethtool_stats	= sja1105_get_ethtool_stats,
 	.get_sset_count		= sja1105_get_sset_count,
-- 
2.21.0

Re: [RFC PATCH net-next 9/9] net: dsa: sja1105: Fix broken fixed-link interfaces on user ports

[Florian Fainelli]

On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> 
> PHYLIB and PHYLINK handle fixed-link interfaces differently. PHYLIB
> wraps them in a software PHY ("pseudo fixed link") phydev construct such
> that .adjust_link driver callbacks see an unified API. Whereas PHYLINK
> simply creates a phylink_link_state structure and passes it to
> .mac_config.
> 
> At the time the driver was introduced, DSA was using PHYLIB for the
> CPU/cascade ports (the ones with no net devices) and PHYLINK for
> everything else.
> 
> As explained below:
> 
> commit aab9c4067d2389d0adfc9c53806437df7b0fe3d5
> Author: Florian Fainelli <f.fainelli@gmail.com>
> Date:   Thu May 10 13:17:36 2018 -0700
> 
>   net: dsa: Plug in PHYLINK support
> 
>   Drivers that utilize fixed links for user-facing ports (e.g: bcm_sf2)
>   will need to implement phylink_mac_ops from now on to preserve
>   functionality, since PHYLINK *does not* create a phy_device instance
>   for fixed links.
> 
> In the above patch, DSA guards the .phylink_mac_config callback against
> a NULL phydev pointer.  Therefore, .adjust_link is not called in case of
> a fixed-link user port.
> 
> This patch fixes the situation by converting the driver from using
> .adjust_link to .phylink_mac_config.  This can be done now in a unified
> fashion for both slave and CPU/cascade ports because DSA now uses
> PHYLINK for all ports.
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>

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

Re: [RFC PATCH net-next 0/9] Decoupling PHYLINK from struct net_device

[Maxime Chevallier]

Hello Ioana,

On Thu, 23 May 2019 01:20:36 +0000
Ioana Ciornei <ioana.ciornei@nxp.com> wrote:

>Following two separate discussion threads in:
>  https://www.spinics.net/lists/netdev/msg569087.html
>and:
>  https://www.spinics.net/lists/netdev/msg570450.html
>
>PHYLINK was reworked in order to add a new "raw" interface, alongside
>the one based on struct net_device. The raw interface works by passing
>structures to the owner of the phylink instance through a blocking
>notifier call.
>
>The event API exposed by the new notifier mechanism is a 1:1 mapping to
>the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
>
>PHYLIB (which PHYLINK uses) was reworked to the extent that it does not
>crash when connecting to a PHY and the net_device pointer is NULL.
>
>Lastly, DSA has been reworked in its way that it handles PHYs for ports
>that lack a net_device (CPU and DSA ports).  For these, it was
>previously using PHYLIB and is now using the PHYLINK raw API.

Thanks for this series, I've tested it and it does address the
particular issue I having, with the DSA fixed-link CPU port. I've been
able to use 2500 and 10000 Mbps links with your series, on a board
using a 88e6390X (so, the mv88e6xxx driver).

I've also tested it on boards using PPv2, which uses phylink, and
didn't spot any issue, so this looks fine.

I'll be happy to test future versions,

Thanks,

Maxime