diff mbox series

[net,4/9] net: stmmac: selftests: Must remove UC/MC addresses to prevent false positives

Message ID 36d9af9080068c4e38cf50e80b6f2a5eafc9ed99.1572355609.git.Jose.Abreu@synopsys.com
State Changes Requested
Delegated to: David Miller
Headers show
Series net: stmmac: Fixes for -net | expand

Commit Message

Jose Abreu Oct. 29, 2019, 2:14 p.m. UTC 
In L2 tests that filter packets by destination MAC address we need to
remove all addresses that are currently in the device so that we don't
get false positives when running the tests.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 .../net/ethernet/stmicro/stmmac/stmmac_selftests.c | 104 +++++++++++++++------
 1 file changed, 76 insertions(+), 28 deletions(-)

Comments

David Miller Oct. 30, 2019, 9:52 p.m. UTC | #1 
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Tue, 29 Oct 2019 15:14:48 +0100

> @@ -499,9 +501,18 @@ static int stmmac_test_hfilt(struct stmmac_priv *priv)
>  	if (netdev_mc_count(priv->dev) >= priv->hw->multicast_filter_bins)
>  		return -EOPNOTSUPP;

This test above...

> +	dummy_dev = alloc_etherdev(0);
> +	if (!dummy_dev)
> +		return -ENOMEM;
> +
> +	/* Remove all MC addresses */
> +	netdev_for_each_mc_addr(ha, priv->dev)
> +		dev_mc_add(dummy_dev, ha->addr);
> +	dev_mc_flush(priv->dev);

No longer makes any sense now that you're removing all of the MC
addresses.

Also I know it seems that it should be guaranteed that re-adding all of
the previously configured MC addresses should succeed.  But I am always
wary when I see error codes ignored like this.

This test makes destructure changes to the device's configuration,
perhaps in a non-restorable fashion if errors occur re-adding the MC
list entries.

Running a test should never even remotely introduce a change in the
device state like that.

I really don't like this, to be honest.  I'd hate to be the user who
had this somehow trigger on them and then have to diagnose it. :-/
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 0b5db52149bc..bcc902f44318 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -490,6 +490,8 @@  static int stmmac_test_hfilt(struct stmmac_priv *priv)
 	unsigned char gd_addr[ETH_ALEN] = {0x01, 0xee, 0xdd, 0xcc, 0xbb, 0xaa};
 	unsigned char bd_addr[ETH_ALEN] = {0x01, 0x01, 0x02, 0x03, 0x04, 0x05};
 	struct stmmac_packet_attrs attr = { };
+	struct net_device *dummy_dev;
+	struct netdev_hw_addr *ha;
 	int ret;
 
 	ret = stmmac_filter_check(priv);
@@ -499,9 +501,18 @@  static int stmmac_test_hfilt(struct stmmac_priv *priv)
 	if (netdev_mc_count(priv->dev) >= priv->hw->multicast_filter_bins)
 		return -EOPNOTSUPP;
 
+	dummy_dev = alloc_etherdev(0);
+	if (!dummy_dev)
+		return -ENOMEM;
+
+	/* Remove all MC addresses */
+	netdev_for_each_mc_addr(ha, priv->dev)
+		dev_mc_add(dummy_dev, ha->addr);
+	dev_mc_flush(priv->dev);
+
 	ret = dev_mc_add(priv->dev, gd_addr);
 	if (ret)
-		return ret;
+		goto sync;
 
 	attr.dst = gd_addr;
 
@@ -518,6 +529,11 @@  static int stmmac_test_hfilt(struct stmmac_priv *priv)
 
 cleanup:
 	dev_mc_del(priv->dev, gd_addr);
+sync:
+	/* Restore MC addresses */
+	netdev_for_each_mc_addr(ha, dummy_dev)
+		dev_mc_add(priv->dev, ha->addr);
+	free_netdev(dummy_dev);
 	return ret;
 }
 
@@ -526,14 +542,25 @@  static int stmmac_test_pfilt(struct stmmac_priv *priv)
 	unsigned char gd_addr[ETH_ALEN] = {0x00, 0x01, 0x44, 0x55, 0x66, 0x77};
 	unsigned char bd_addr[ETH_ALEN] = {0x08, 0x00, 0x22, 0x33, 0x44, 0x55};
 	struct stmmac_packet_attrs attr = { };
+	struct net_device *dummy_dev;
+	struct netdev_hw_addr *ha;
 	int ret;
 
 	if (stmmac_filter_check(priv))
 		return -EOPNOTSUPP;
 
+	dummy_dev = alloc_etherdev(0);
+	if (!dummy_dev)
+		return -ENOMEM;
+
+	/* Remove all UC addresses */
+	netdev_for_each_uc_addr(ha, priv->dev)
+		dev_uc_add(dummy_dev, ha->addr);
+	dev_uc_flush(priv->dev);
+
 	ret = dev_uc_add(priv->dev, gd_addr);
 	if (ret)
-		return ret;
+		goto sync;
 
 	attr.dst = gd_addr;
 
@@ -550,28 +577,21 @@  static int stmmac_test_pfilt(struct stmmac_priv *priv)
 
 cleanup:
 	dev_uc_del(priv->dev, gd_addr);
+sync:
+	/* Restore UC addresses */
+	netdev_for_each_uc_addr(ha, dummy_dev)
+		dev_uc_add(priv->dev, ha->addr);
+	free_netdev(dummy_dev);
 	return ret;
 }
 
-static int stmmac_dummy_sync(struct net_device *netdev, const u8 *addr)
-{
-	return 0;
-}
-
-static void stmmac_test_set_rx_mode(struct net_device *netdev)
-{
-	/* As we are in test mode of ethtool we already own the rtnl lock
-	 * so no address will change from user. We can just call the
-	 * ndo_set_rx_mode() callback directly */
-	if (netdev->netdev_ops->ndo_set_rx_mode)
-		netdev->netdev_ops->ndo_set_rx_mode(netdev);
-}
-
 static int stmmac_test_mcfilt(struct stmmac_priv *priv)
 {
 	unsigned char uc_addr[ETH_ALEN] = {0x00, 0x01, 0x44, 0x55, 0x66, 0x77};
 	unsigned char mc_addr[ETH_ALEN] = {0x01, 0x01, 0x44, 0x55, 0x66, 0x77};
 	struct stmmac_packet_attrs attr = { };
+	struct net_device *dummy_dev;
+	struct netdev_hw_addr *ha;
 	int ret;
 
 	if (stmmac_filter_check(priv))
@@ -579,13 +599,21 @@  static int stmmac_test_mcfilt(struct stmmac_priv *priv)
 	if (!priv->hw->multicast_filter_bins)
 		return -EOPNOTSUPP;
 
-	/* Remove all MC addresses */
-	__dev_mc_unsync(priv->dev, NULL);
-	stmmac_test_set_rx_mode(priv->dev);
+	dummy_dev = alloc_etherdev(0);
+	if (!dummy_dev)
+		return -ENOMEM;
+
+	/* Remove all UC and MC addresses */
+	netdev_for_each_uc_addr(ha, priv->dev)
+		dev_uc_add(dummy_dev, ha->addr);
+	netdev_for_each_mc_addr(ha, priv->dev)
+		dev_mc_add(dummy_dev, ha->addr);
+	dev_uc_flush(priv->dev);
+	dev_mc_flush(priv->dev);
 
 	ret = dev_uc_add(priv->dev, uc_addr);
 	if (ret)
-		goto cleanup;
+		goto sync;
 
 	attr.dst = uc_addr;
 
@@ -602,8 +630,13 @@  static int stmmac_test_mcfilt(struct stmmac_priv *priv)
 
 cleanup:
 	dev_uc_del(priv->dev, uc_addr);
-	__dev_mc_sync(priv->dev, stmmac_dummy_sync, NULL);
-	stmmac_test_set_rx_mode(priv->dev);
+sync:
+	/* Restore UC and MC addresses */
+	netdev_for_each_uc_addr(ha, dummy_dev)
+		dev_uc_add(priv->dev, ha->addr);
+	netdev_for_each_mc_addr(ha, dummy_dev)
+		dev_mc_add(priv->dev, ha->addr);
+	free_netdev(dummy_dev);
 	return ret;
 }
 
@@ -612,6 +645,8 @@  static int stmmac_test_ucfilt(struct stmmac_priv *priv)
 	unsigned char uc_addr[ETH_ALEN] = {0x00, 0x01, 0x44, 0x55, 0x66, 0x77};
 	unsigned char mc_addr[ETH_ALEN] = {0x01, 0x01, 0x44, 0x55, 0x66, 0x77};
 	struct stmmac_packet_attrs attr = { };
+	struct net_device *dummy_dev;
+	struct netdev_hw_addr *ha;
 	int ret;
 
 	if (stmmac_filter_check(priv))
@@ -619,13 +654,21 @@  static int stmmac_test_ucfilt(struct stmmac_priv *priv)
 	if (!priv->hw->multicast_filter_bins)
 		return -EOPNOTSUPP;
 
-	/* Remove all UC addresses */
-	__dev_uc_unsync(priv->dev, NULL);
-	stmmac_test_set_rx_mode(priv->dev);
+	dummy_dev = alloc_etherdev(0);
+	if (!dummy_dev)
+		return -ENOMEM;
+
+	/* Remove all UC and MC addresses */
+	netdev_for_each_uc_addr(ha, priv->dev)
+		dev_uc_add(dummy_dev, ha->addr);
+	netdev_for_each_mc_addr(ha, priv->dev)
+		dev_mc_add(dummy_dev, ha->addr);
+	dev_uc_flush(priv->dev);
+	dev_mc_flush(priv->dev);
 
 	ret = dev_mc_add(priv->dev, mc_addr);
 	if (ret)
-		goto cleanup;
+		goto sync;
 
 	attr.dst = mc_addr;
 
@@ -642,8 +685,13 @@  static int stmmac_test_ucfilt(struct stmmac_priv *priv)
 
 cleanup:
 	dev_mc_del(priv->dev, mc_addr);
-	__dev_uc_sync(priv->dev, stmmac_dummy_sync, NULL);
-	stmmac_test_set_rx_mode(priv->dev);
+sync:
+	/* Restore UC and MC addresses */
+	netdev_for_each_uc_addr(ha, dummy_dev)
+		dev_uc_add(priv->dev, ha->addr);
+	netdev_for_each_mc_addr(ha, dummy_dev)
+		dev_mc_add(priv->dev, ha->addr);
+	free_netdev(dummy_dev);
 	return ret;
 }