[net v3 PATCH] net: ethernet: ti: cpsw_new: fix error handling in cpsw_ndo_vlan_rx_kill_vid()

[net v3 PATCH] net: ethernet: ti: cpsw_new: fix error handling in cpsw_ndo_vlan_rx_kill_vid()

[Murali Karicheri]

This patch fixes a bunch of issues in cpsw_ndo_vlan_rx_kill_vid()

 - pm_runtime_get_sync() returns non zero value. This results in
   non zero value return to caller which will be interpreted as error.
   So overwrite ret with zero.
 - If VID matches with port VLAN VID, then set error code.
 - Currently when VLAN interface is deleted, all of the VLAN mc addresses
   are removed from ALE table, however the return values from ale function
   calls are not checked. These functions can return error code -ENOENT.
   But that shouldn't happen in a normal case. So add error print to
   catch the situations so that these can be investigated and addressed.
   return zero in these cases as these are not real error case, but only
   serve to catch ALE table update related issues and help address the
   same in the driver.

Fixes: ed3525eda4c4 ("net: ethernet: ti: introduce cpsw switchdev based driver part 1 - dual-emac")
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 v3 - updated commit description to describe error check related to
      port vlan VID
 v2 - updated comments from Grygorii, also return error code if VID
 match with port_vlan vid.
 drivers/net/ethernet/ti/cpsw_new.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
index 8d0a2bc7128d..61fa5063d751 100644
--- a/drivers/net/ethernet/ti/cpsw_new.c
+++ b/drivers/net/ethernet/ti/cpsw_new.c
@@ -1032,19 +1032,35 @@ static int cpsw_ndo_vlan_rx_kill_vid(struct net_device *ndev,
 		return ret;
 	}
 
+	/* reset the return code as pm_runtime_get_sync() can return
+	 * non zero values as well.
+	 */
+	ret = 0;
 	for (i = 0; i < cpsw->data.slaves; i++) {
 		if (cpsw->slaves[i].ndev &&
-		    vid == cpsw->slaves[i].port_vlan)
+		    vid == cpsw->slaves[i].port_vlan) {
+			ret = -EINVAL;
 			goto err;
+		}
 	}
 
 	dev_dbg(priv->dev, "removing vlanid %d from vlan filter\n", vid);
-	cpsw_ale_del_vlan(cpsw->ale, vid, 0);
-	cpsw_ale_del_ucast(cpsw->ale, priv->mac_addr,
-			   HOST_PORT_NUM, ALE_VLAN, vid);
-	cpsw_ale_del_mcast(cpsw->ale, priv->ndev->broadcast,
-			   0, ALE_VLAN, vid);
+	ret = cpsw_ale_del_vlan(cpsw->ale, vid, 0);
+	if (ret)
+		dev_err(priv->dev, "%s: failed %d: ret %d\n",
+			__func__, __LINE__, ret);
+	ret = cpsw_ale_del_ucast(cpsw->ale, priv->mac_addr,
+				 HOST_PORT_NUM, ALE_VLAN, vid);
+	if (ret)
+		dev_err(priv->dev, "%s: failed %d: ret %d\n",
+			__func__, __LINE__, ret);
+	ret = cpsw_ale_del_mcast(cpsw->ale, priv->ndev->broadcast,
+				 0, ALE_VLAN, vid);
+	if (ret)
+		dev_err(priv->dev, "%s: failed %d: ret %d\n",
+			__func__, __LINE__, ret);
 	cpsw_ale_flush_multicast(cpsw->ale, ALE_PORT_HOST, vid);
+	ret = 0;
 err:
 	pm_runtime_put(cpsw->dev);
 	return ret;
-- 
2.17.1

Re: [net v3 PATCH] net: ethernet: ti: cpsw_new: fix error handling in cpsw_ndo_vlan_rx_kill_vid()

[David Miller]

From: Murali Karicheri <m-karicheri2@ti.com>
Date: Mon, 24 Aug 2020 13:01:00 -0400

> +	ret = cpsw_ale_del_vlan(cpsw->ale, vid, 0);
> +	if (ret)
> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
> +			__func__, __LINE__, ret);
> +	ret = cpsw_ale_del_ucast(cpsw->ale, priv->mac_addr,
> +				 HOST_PORT_NUM, ALE_VLAN, vid);
> +	if (ret)
> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
> +			__func__, __LINE__, ret);
> +	ret = cpsw_ale_del_mcast(cpsw->ale, priv->ndev->broadcast,
> +				 0, ALE_VLAN, vid);
> +	if (ret)
> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
> +			__func__, __LINE__, ret);
>  	cpsw_ale_flush_multicast(cpsw->ale, ALE_PORT_HOST, vid);

These error messages are extremely unhelpful.  You're calling three
different functions, yet emitting basically the same __func__ for
each of those cases.  No user can send you a useful bug report
immediately if they just have func and line.

Please get rid of the "__func__" and "__line__" stuff completely, it's
never advisable to ever use that in my opinion.  Instead, describe
which delete operation failed, optionally with the error return.

Re: [net v3 PATCH] net: ethernet: ti: cpsw_new: fix error handling in cpsw_ndo_vlan_rx_kill_vid()

[Murali Karicheri]

Hi Dave,

On 8/25/20 12:36 PM, David Miller wrote:
> From: Murali Karicheri <m-karicheri2@ti.com>
> Date: Mon, 24 Aug 2020 13:01:00 -0400
> 
>> +	ret = cpsw_ale_del_vlan(cpsw->ale, vid, 0);
>> +	if (ret)
>> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
>> +			__func__, __LINE__, ret);
>> +	ret = cpsw_ale_del_ucast(cpsw->ale, priv->mac_addr,
>> +				 HOST_PORT_NUM, ALE_VLAN, vid);
>> +	if (ret)
>> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
>> +			__func__, __LINE__, ret);
>> +	ret = cpsw_ale_del_mcast(cpsw->ale, priv->ndev->broadcast,
>> +				 0, ALE_VLAN, vid);
>> +	if (ret)
>> +		dev_err(priv->dev, "%s: failed %d: ret %d\n",
>> +			__func__, __LINE__, ret);
>>   	cpsw_ale_flush_multicast(cpsw->ale, ALE_PORT_HOST, vid);
> 
> These error messages are extremely unhelpful.  You're calling three
> different functions, yet emitting basically the same __func__ for
> each of those cases.  No user can send you a useful bug report
> immediately if they just have func and line.
> 
> Please get rid of the "__func__" and "__line__" stuff completely, it's
> never advisable to ever use that in my opinion.  Instead, describe
> which delete operation failed, optionally with the error return.
> 
OK. I had considered your suggestion, but thought having a line number
would be handy for a developer. Function name would be better. Will
re-send with changes as you have suggested.

-- 
Murali Karicheri
Texas Instruments