[PATCH v2 00/10] staging: fsl-dpaa2/ethsw: code cleanup

[PATCH v2 00/10] staging: fsl-dpaa2/ethsw: code cleanup

[Ioana Ciornei]

This patch set addresses some of the feedback received on the dpaa2-ethsw
driver: https://lore.kernel.org/patchwork/patch/1113335/.

There are no functional changes but rather just code cleanup.

Changes in v2:
 - added Reported-by and Suggested-by tags

Ioana Ciornei (10):
  staging: fsl-dpaa2/ethsw: remove IGMP default address
  staging: fsl-dpaa2/ethsw: enable switch ports only on dev_open
  staging: fsl-dpaa2/ethsw: add line terminator to all formats
  staging: fsl-dpaa2/ethsw: remove debug message
  staging: fsl-dpaa2/ethsw: use bool when encoding learning/flooding
    state
  staging: fsl-dpaa2/ethsw: remove unnecessary memset
  staging: fsl-dpaa2/ethsw: remove redundant VLAN check
  staging: fsl-dpaa2/ethsw: reword error message
  staging: fsl-dpaa2/ethsw: register_netdev only when ready
  staging: fsl-dpaa2/ethsw: do not force user to bring interface down

 drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c |  44 +++++-----
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c         | 110 ++++++------------------
 2 files changed, 50 insertions(+), 104 deletions(-)

-- 
1.9.1

[PATCH v2 01/10] staging: fsl-dpaa2/ethsw: remove IGMP default address

[Ioana Ciornei]

Do not add an IGMP multicast address by default since we do not support
Rx/Tx ar the moment.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index aac98ece2335..8032314d5cae 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -1506,7 +1506,6 @@ static int ethsw_init(struct fsl_mc_device *sw_dev)
 
 static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
 {
-	const char def_mcast[ETH_ALEN] = {0x01, 0x00, 0x5e, 0x00, 0x00, 0x01};
 	struct net_device *netdev = port_priv->netdev;
 	struct ethsw_core *ethsw = port_priv->ethsw_data;
 	struct dpsw_vlan_if_cfg vcfg;
@@ -1532,12 +1531,10 @@ static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
 
 	err = dpsw_vlan_remove_if(ethsw->mc_io, 0, ethsw->dpsw_handle,
 				  DEFAULT_VLAN_ID, &vcfg);
-	if (err) {
+	if (err)
 		netdev_err(netdev, "dpsw_vlan_remove_if err %d\n", err);
-		return err;
-	}
 
-	return ethsw_port_fdb_add_mc(port_priv, def_mcast);
+	return err;
 }
 
 static void ethsw_unregister_notifier(struct device *dev)
-- 
1.9.1

[PATCH v2 02/10] staging: fsl-dpaa2/ethsw: enable switch ports only on dev_open

[Ioana Ciornei]

At probe time, only the DPSW object should be enabled without the
associated ports, which will get enabled on dev_open. Remove the
ethsw_open() and ethsw_stop() functions and replace them only with
dpsw_enable()/_disable().

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 59 ++++-----------------------------
 1 file changed, 6 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 8032314d5cae..302842c3bdfe 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -1363,48 +1363,6 @@ static int ethsw_register_notifier(struct device *dev)
 	return err;
 }
 
-static int ethsw_open(struct ethsw_core *ethsw)
-{
-	struct ethsw_port_priv *port_priv = NULL;
-	int i, err;
-
-	err = dpsw_enable(ethsw->mc_io, 0, ethsw->dpsw_handle);
-	if (err) {
-		dev_err(ethsw->dev, "dpsw_enable err %d\n", err);
-		return err;
-	}
-
-	for (i = 0; i < ethsw->sw_attr.num_ifs; i++) {
-		port_priv = ethsw->ports[i];
-		err = dev_open(port_priv->netdev, NULL);
-		if (err) {
-			netdev_err(port_priv->netdev, "dev_open err %d\n", err);
-			return err;
-		}
-	}
-
-	return 0;
-}
-
-static int ethsw_stop(struct ethsw_core *ethsw)
-{
-	struct ethsw_port_priv *port_priv = NULL;
-	int i, err;
-
-	for (i = 0; i < ethsw->sw_attr.num_ifs; i++) {
-		port_priv = ethsw->ports[i];
-		dev_close(port_priv->netdev);
-	}
-
-	err = dpsw_disable(ethsw->mc_io, 0, ethsw->dpsw_handle);
-	if (err) {
-		dev_err(ethsw->dev, "dpsw_disable err %d\n", err);
-		return err;
-	}
-
-	return 0;
-}
-
 static int ethsw_init(struct fsl_mc_device *sw_dev)
 {
 	struct device *dev = &sw_dev->dev;
@@ -1586,9 +1544,7 @@ static int ethsw_remove(struct fsl_mc_device *sw_dev)
 
 	destroy_workqueue(ethsw_owq);
 
-	rtnl_lock();
-	ethsw_stop(ethsw);
-	rtnl_unlock();
+	dpsw_disable(ethsw->mc_io, 0, ethsw->dpsw_handle);
 
 	for (i = 0; i < ethsw->sw_attr.num_ifs; i++) {
 		port_priv = ethsw->ports[i];
@@ -1708,12 +1664,11 @@ static int ethsw_probe(struct fsl_mc_device *sw_dev)
 			goto err_free_ports;
 	}
 
-	/* Switch starts up enabled */
-	rtnl_lock();
-	err = ethsw_open(ethsw);
-	rtnl_unlock();
-	if (err)
+	err = dpsw_enable(ethsw->mc_io, 0, ethsw->dpsw_handle);
+	if (err) {
+		dev_err(ethsw->dev, "dpsw_enable err %d\n", err);
 		goto err_free_ports;
+	}
 
 	/* Setup IRQs */
 	err = ethsw_setup_irqs(sw_dev);
@@ -1724,9 +1679,7 @@ static int ethsw_probe(struct fsl_mc_device *sw_dev)
 	return 0;
 
 err_stop:
-	rtnl_lock();
-	ethsw_stop(ethsw);
-	rtnl_unlock();
+	dpsw_disable(ethsw->mc_io, 0, ethsw->dpsw_handle);
 
 err_free_ports:
 	/* Cleanup registered ports only */
-- 
1.9.1

[PATCH v2 03/10] staging: fsl-dpaa2/ethsw: add line terminator to all formats

[Ioana Ciornei]

Add the '\n' line terminator to the string formats missing it.

Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c |  2 +-
 drivers/staging/fsl-dpaa2/ethsw/ethsw.c         | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index 926a0c053e18..95e9f1096999 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -65,7 +65,7 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
 				     port_priv->idx,
 				     &state);
 	if (err) {
-		netdev_err(netdev, "ERROR %d getting link state", err);
+		netdev_err(netdev, "ERROR %d getting link state\n", err);
 		goto out;
 	}
 
diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 302842c3bdfe..9ade73928e60 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -722,12 +722,12 @@ static irqreturn_t ethsw_irq0_handler_thread(int irq_num, void *arg)
 	err = dpsw_get_irq_status(ethsw->mc_io, 0, ethsw->dpsw_handle,
 				  DPSW_IRQ_INDEX_IF, &status);
 	if (err) {
-		dev_err(dev, "Can't get irq status (err %d)", err);
+		dev_err(dev, "Can't get irq status (err %d)\n", err);
 
 		err = dpsw_clear_irq_status(ethsw->mc_io, 0, ethsw->dpsw_handle,
 					    DPSW_IRQ_INDEX_IF, 0xFFFFFFFF);
 		if (err)
-			dev_err(dev, "Can't clear irq status (err %d)", err);
+			dev_err(dev, "Can't clear irq status (err %d)\n", err);
 		goto out;
 	}
 
@@ -772,21 +772,21 @@ static int ethsw_setup_irqs(struct fsl_mc_device *sw_dev)
 					IRQF_NO_SUSPEND | IRQF_ONESHOT,
 					dev_name(dev), dev);
 	if (err) {
-		dev_err(dev, "devm_request_threaded_irq(): %d", err);
+		dev_err(dev, "devm_request_threaded_irq(): %d\n", err);
 		goto free_irq;
 	}
 
 	err = dpsw_set_irq_mask(ethsw->mc_io, 0, ethsw->dpsw_handle,
 				DPSW_IRQ_INDEX_IF, mask);
 	if (err) {
-		dev_err(dev, "dpsw_set_irq_mask(): %d", err);
+		dev_err(dev, "dpsw_set_irq_mask(): %d\n", err);
 		goto free_devm_irq;
 	}
 
 	err = dpsw_set_irq_enable(ethsw->mc_io, 0, ethsw->dpsw_handle,
 				  DPSW_IRQ_INDEX_IF, 1);
 	if (err) {
-		dev_err(dev, "dpsw_set_irq_enable(): %d", err);
+		dev_err(dev, "dpsw_set_irq_enable(): %d\n", err);
 		goto free_devm_irq;
 	}
 
-- 
1.9.1

[PATCH v2 04/10] staging: fsl-dpaa2/ethsw: remove debug message

[Ioana Ciornei]

Since ethtool will be loud enough if the .set_link_ksettings() callback
fails, remove the debug messages which do not add additional
information.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index 95e9f1096999..12e33a3a1bf1 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -91,8 +91,6 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
 	struct dpsw_link_cfg cfg = {0};
 	int err = 0;
 
-	netdev_dbg(netdev, "Setting link parameters...");
-
 	/* Due to a temporary MC limitation, the DPSW port must be down
 	 * in order to be able to change link settings. Taking steps to let
 	 * the user know that.
@@ -116,11 +114,6 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
 				   port_priv->ethsw_data->dpsw_handle,
 				   port_priv->idx,
 				   &cfg);
-	if (err)
-		/* ethtool will be loud enough if we return an error; no point
-		 * in putting our own error message on the console by default
-		 */
-		netdev_dbg(netdev, "ERROR %d setting link cfg", err);
 
 	return err;
 }
-- 
1.9.1

[PATCH v2 05/10] staging: fsl-dpaa2/ethsw: use bool when encoding learning/flooding state

[Ioana Ciornei]

Use a bool instead of an u8 in ethsw_set_learning() and
ethsw_port_set_flood() to encode an binary type property.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 9ade73928e60..20519af4d804 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -149,12 +149,12 @@ static int ethsw_port_add_vlan(struct ethsw_port_priv *port_priv,
 	return 0;
 }
 
-static int ethsw_set_learning(struct ethsw_core *ethsw, u8 flag)
+static int ethsw_set_learning(struct ethsw_core *ethsw, bool enable)
 {
 	enum dpsw_fdb_learning_mode learn_mode;
 	int err;
 
-	if (flag)
+	if (enable)
 		learn_mode = DPSW_FDB_LEARNING_MODE_HW;
 	else
 		learn_mode = DPSW_FDB_LEARNING_MODE_DIS;
@@ -165,24 +165,24 @@ static int ethsw_set_learning(struct ethsw_core *ethsw, u8 flag)
 		dev_err(ethsw->dev, "dpsw_fdb_set_learning_mode err %d\n", err);
 		return err;
 	}
-	ethsw->learning = !!flag;
+	ethsw->learning = enable;
 
 	return 0;
 }
 
-static int ethsw_port_set_flood(struct ethsw_port_priv *port_priv, u8 flag)
+static int ethsw_port_set_flood(struct ethsw_port_priv *port_priv, bool enable)
 {
 	int err;
 
 	err = dpsw_if_set_flooding(port_priv->ethsw_data->mc_io, 0,
 				   port_priv->ethsw_data->dpsw_handle,
-				   port_priv->idx, flag);
+				   port_priv->idx, enable);
 	if (err) {
 		netdev_err(port_priv->netdev,
 			   "dpsw_if_set_flooding err %d\n", err);
 		return err;
 	}
-	port_priv->flood = !!flag;
+	port_priv->flood = enable;
 
 	return 0;
 }
-- 
1.9.1

[PATCH v2 06/10] staging: fsl-dpaa2/ethsw: remove unnecessary memset

[Ioana Ciornei]

The ethtool core already zeroes the memory before calling
.get_ethtool_stats() thus making the memset unnecessary. Remove it.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index 12e33a3a1bf1..0f9f8345e534 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -149,9 +149,6 @@ static void ethsw_ethtool_get_stats(struct net_device *netdev,
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
 	int i, err;
 
-	memset(data, 0,
-	       sizeof(u64) * ETHSW_NUM_COUNTERS);
-
 	for (i = 0; i < ETHSW_NUM_COUNTERS; i++) {
 		err = dpsw_if_get_counter(port_priv->ethsw_data->mc_io, 0,
 					  port_priv->ethsw_data->dpsw_handle,
-- 
1.9.1

[PATCH v2 07/10] staging: fsl-dpaa2/ethsw: remove redundant VLAN check

[Ioana Ciornei]

The ethsw_add_vlan() function is already called only when the VLAN is
not yet configured on the switch. Remove the redundant check.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 20519af4d804..b69b2b7be972 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -34,11 +34,6 @@ static int ethsw_add_vlan(struct ethsw_core *ethsw, u16 vid)
 		.fdb_id = 0,
 	};
 
-	if (ethsw->vlans[vid]) {
-		dev_err(ethsw->dev, "VLAN already configured\n");
-		return -EEXIST;
-	}
-
 	err = dpsw_vlan_add(ethsw->mc_io, 0,
 			    ethsw->dpsw_handle, vid, &vcfg);
 	if (err) {
-- 
1.9.1

[PATCH v2 08/10] staging: fsl-dpaa2/ethsw: reword error message

[Ioana Ciornei]

In the current state, the dpaa2-ethsw driver supports only one bridge
per DPSW object. Reword the error message so that this information is
much more clear.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Suggested-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index b69b2b7be972..28da109aef5e 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -1119,8 +1119,7 @@ static int port_bridge_join(struct net_device *netdev,
 		if (ethsw->ports[i]->bridge_dev &&
 		    (ethsw->ports[i]->bridge_dev != upper_dev)) {
 			netdev_err(netdev,
-				   "Another switch port is connected to %s\n",
-				   ethsw->ports[i]->bridge_dev->name);
+				   "Only one bridge supported per DPSW object!\n");
 			return -EINVAL;
 		}
 
-- 
1.9.1

[PATCH v2 09/10] staging: fsl-dpaa2/ethsw: register_netdev only when ready

[Ioana Ciornei]

The register_netdev() call should be made only when ready to process any
user request on the interface. Move the call to be the last one issued
in the probe sequence.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 28da109aef5e..14a9eebf687e 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -1588,23 +1588,21 @@ static int ethsw_probe_port(struct ethsw_core *ethsw, u16 port_idx)
 	port_netdev->min_mtu = ETH_MIN_MTU;
 	port_netdev->max_mtu = ETHSW_MAX_FRAME_LENGTH;
 
+	err = ethsw_port_init(port_priv, port_idx);
+	if (err)
+		goto err_port_probe;
+
 	err = register_netdev(port_netdev);
 	if (err < 0) {
 		dev_err(dev, "register_netdev error %d\n", err);
-		goto err_register_netdev;
+		goto err_port_probe;
 	}
 
 	ethsw->ports[port_idx] = port_priv;
 
-	err = ethsw_port_init(port_priv, port_idx);
-	if (err)
-		goto err_ethsw_port_init;
-
 	return 0;
 
-err_ethsw_port_init:
-	unregister_netdev(port_netdev);
-err_register_netdev:
+err_port_probe:
 	free_netdev(port_netdev);
 
 	return err;
-- 
1.9.1

[PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

[Ioana Ciornei]

Link settings can be changed only when the interface is down. Disable
and re-enable the interface, if necessary, behind the scenes so that we do
not force users to an if down/up sequence.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - added Reported-by tag

 drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
index 0f9f8345e534..99d658fefa14 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
@@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
 			 const struct ethtool_link_ksettings *link_ksettings)
 {
 	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
+	struct ethsw_core *ethsw = port_priv->ethsw_data;
 	struct dpsw_link_cfg cfg = {0};
-	int err = 0;
-
-	/* Due to a temporary MC limitation, the DPSW port must be down
-	 * in order to be able to change link settings. Taking steps to let
-	 * the user know that.
-	 */
-	if (netif_running(netdev)) {
-		netdev_info(netdev, "Sorry, interface must be brought down first.\n");
-		return -EACCES;
+	bool if_running;
+	int err = 0, ret;
+
+	/* Interface needs to be down to change link settings */
+	if_running = netif_running(netdev);
+	if (if_running) {
+		err = dpsw_if_disable(ethsw->mc_io, 0,
+				      ethsw->dpsw_handle,
+				      port_priv->idx);
+		if (err) {
+			netdev_err(netdev, "dpsw_if_disable err %d\n", err);
+			return err;
+		}
 	}
 
 	cfg.rate = link_ksettings->base.speed;
@@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
 				   port_priv->idx,
 				   &cfg);
 
+	if (if_running) {
+		ret = dpsw_if_enable(ethsw->mc_io, 0,
+				     ethsw->dpsw_handle,
+				     port_priv->idx);
+		if (ret) {
+			return ret;
+			netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
+		}
+	}
 	return err;
 }
 
-- 
1.9.1

Re: [PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

[Razvan.Stefanescu]

On 13/08/2019 11:54, Ioana Ciornei wrote:
> Link settings can be changed only when the interface is down. Disable
> and re-enable the interface, if necessary, behind the scenes so that we do
> not force users to an if down/up sequence.
> 
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
>   - added Reported-by tag
> 
>   drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> index 0f9f8345e534..99d658fefa14 100644
> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
> @@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>   			 const struct ethtool_link_ksettings *link_ksettings)
>   {
>   	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
> +	struct ethsw_core *ethsw = port_priv->ethsw_data;
>   	struct dpsw_link_cfg cfg = {0};
> -	int err = 0;
> -
> -	/* Due to a temporary MC limitation, the DPSW port must be down
> -	 * in order to be able to change link settings. Taking steps to let
> -	 * the user know that.
> -	 */
> -	if (netif_running(netdev)) {
> -		netdev_info(netdev, "Sorry, interface must be brought down first.\n");
> -		return -EACCES;
> +	bool if_running;
> +	int err = 0, ret;
> +
> +	/* Interface needs to be down to change link settings */
> +	if_running = netif_running(netdev);
> +	if (if_running) {
> +		err = dpsw_if_disable(ethsw->mc_io, 0,
> +				      ethsw->dpsw_handle,
> +				      port_priv->idx);
> +		if (err) {
> +			netdev_err(netdev, "dpsw_if_disable err %d\n", err);
> +			return err;
> +		}
>   	}
>   
>   	cfg.rate = link_ksettings->base.speed;
> @@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>   				   port_priv->idx,
>   				   &cfg);
>   
> +	if (if_running) {
> +		ret = dpsw_if_enable(ethsw->mc_io, 0,
> +				     ethsw->dpsw_handle,
> +				     port_priv->idx);
> +		if (ret) {
> +			return ret;
> +			netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
Hello,

These last two lines need to be swapped.

Best regards,
Razvan

Re: [PATCH v2 10/10] staging: fsl-dpaa2/ethsw: do not force user to bring interface down

[Ioana Ciornei]

On 8/13/19 12:38 PM, Razvan.Stefanescu@microchip.com wrote:
> 
> 
> On 13/08/2019 11:54, Ioana Ciornei wrote:
>> Link settings can be changed only when the interface is down. Disable
>> and re-enable the interface, if necessary, behind the scenes so that we do
>> not force users to an if down/up sequence.
>>
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> ---
>> Changes in v2:
>>    - added Reported-by tag
>>
>>    drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c | 32 ++++++++++++++++++-------
>>    1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> index 0f9f8345e534..99d658fefa14 100644
>> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw-ethtool.c
>> @@ -88,16 +88,21 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>>    			 const struct ethtool_link_ksettings *link_ksettings)
>>    {
>>    	struct ethsw_port_priv *port_priv = netdev_priv(netdev);
>> +	struct ethsw_core *ethsw = port_priv->ethsw_data;
>>    	struct dpsw_link_cfg cfg = {0};
>> -	int err = 0;
>> -
>> -	/* Due to a temporary MC limitation, the DPSW port must be down
>> -	 * in order to be able to change link settings. Taking steps to let
>> -	 * the user know that.
>> -	 */
>> -	if (netif_running(netdev)) {
>> -		netdev_info(netdev, "Sorry, interface must be brought down first.\n");
>> -		return -EACCES;
>> +	bool if_running;
>> +	int err = 0, ret;
>> +
>> +	/* Interface needs to be down to change link settings */
>> +	if_running = netif_running(netdev);
>> +	if (if_running) {
>> +		err = dpsw_if_disable(ethsw->mc_io, 0,
>> +				      ethsw->dpsw_handle,
>> +				      port_priv->idx);
>> +		if (err) {
>> +			netdev_err(netdev, "dpsw_if_disable err %d\n", err);
>> +			return err;
>> +		}
>>    	}
>>    
>>    	cfg.rate = link_ksettings->base.speed;
>> @@ -115,6 +120,15 @@ static void ethsw_get_drvinfo(struct net_device *netdev,
>>    				   port_priv->idx,
>>    				   &cfg);
>>    
>> +	if (if_running) {
>> +		ret = dpsw_if_enable(ethsw->mc_io, 0,
>> +				     ethsw->dpsw_handle,
>> +				     port_priv->idx);
>> +		if (ret) {
>> +			return ret;
>> +			netdev_err(netdev, "dpsw_if_enable err %d\n", ret);
> Hello, >
> These last two lines need to be swapped.
> 
> Best regards,
> Razvan
> 

Oops, my bad. Will fix.

Thanks for pointing this out,
Ioana