Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752909AbdFQMBP convert rfc822-to-8bit (ORCPT ); Sat, 17 Jun 2017 08:01:15 -0400 Received: from szxga03-in.huawei.com ([45.249.212.189]:7879 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752826AbdFQMBN (ORCPT ); Sat, 17 Jun 2017 08:01:13 -0400 From: Salil Mehta To: "Mintz, Yuval" , "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 Thread-Topic: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC Thread-Index: AQHS4ZxY/dQq9iaau0S4JZktdJRDYKId6YiAgAMi5rCAAth1gIAFGZBg Date: Sat, 17 Jun 2017 12:00:47 +0000 Message-ID: References: <20170610034630.493852-1-salil.mehta@huawei.com> <20170610034630.493852-2-salil.mehta@huawei.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.203.181.160] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090204.59451A04.004C,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=169.254.2.25, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: b9817ccb430c32112e065c2770aee4c3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2416 Lines: 76 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 > > > > > +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 > > 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 > > > > Isn't mqprio going to override your priority2tc mapping with the > one > > > 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