Hi Yuval,

-----Original Message-----
From: Mintz, Yuval [mailto:Yuval.Mintz@cavium.com]
Sent: Wednesday, June 14, 2017 9:04 AM
To: Salil Mehta; davem@davemloft.net
Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; Linuxarm
Subject: RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3
Ethernet Driver for hip08 SoC

quoted

quoted

quoted

+static void hns3_nic_net_down(struct net_device *ndev) {
+	struct hns3_nic_priv *priv = netdev_priv(ndev);
+	struct hnae3_ae_ops *ops;
+	int i;
+
+	netif_tx_stop_all_queues(ndev);
+	netif_carrier_off(ndev);
+	netif_tx_disable(ndev);
+
+	ops = priv->ae_handle->ae_algo->ops;
+
+	if (ops->stop)
+		ops->stop(priv->ae_handle);
+
+	netif_tx_stop_all_queues(ndev);

Looks a bit excessive. Why do you need all these
netif_tx_stop_all_queues()?

If we are disabling the netdev. We need to stop scheduling the queues
associated with that netdev for TX, so we need this code. Why do you

think

quoted

it is excessive?

Why do you need both netif_tx_disable() and netif_tx_stop_all_queues()?
And why would you need to re-do netif_tx_stop_all_queues() after
calling ops->stop()?

Oh yes! I missed this totally, In fact, netif_tx_diable is doing almost
the similar job what netif_tx_stop_all_queues() is doing with some
lock protection.

Thanks for identifying this. I will fix this in V3 patch. 

Thanks
Salil

quoted

quoted

Isn't mqprio going to override your priority2tc mapping with the

one

quoted

quoted

provided by user?

I guess you are referring to below code in the mqprio_init() - right?

static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
{
  [...]
	/* Always use supplied priority mappings */
	for (i = 0; i < TC_BITMASK + 1; i++)
		netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
  [...]
}

In this case yes, you are right below code seems to be redundant:

 +	/* Assign UP2TC map for the VSI */
 +	for (i = 0; i < HNAE3_MAX_TC; i++) {
 +		netdev_set_prio_tc_map(ndev,
 +				       kinfo->tc_info[i].up,
 +				       kinfo->tc_info[i].tc);

Hope I am not missing anything here?

You're not; That's what I meant.

Will remove this code in V3 patch.

Thanks
Salil