From mboxrd@z Thu Jan  1 00:00:00 1970
From: Rami Rosen <roszenrami@gmail.com>
Subject: Re: [PATCH 03/19] net/ice: support device and queue ops
Date: Mon, 3 Dec 2018 17:43:57 +0200
Message-ID: <CAKoUArmG1nLtuTFBrtVw4zqDn2yK9_B7F3-exWGUUcZ3-NEXqA@mail.gmail.com>
References: <1542956179-80951-1-git-send-email-wenzhuo.lu@intel.com>
 <1542956179-80951-4-git-send-email-wenzhuo.lu@intel.com>
 <CAKoUArkb_-se-2saBL5Yx_2zksUQVdAEm45mOf6C5jRY_8z72g@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: dev@dpdk.org, qiming.yang@intel.com, xiaoyun.li@intel.com,
 jingjing.wu@intel.com
To: wenzhuo.lu@intel.com
Return-path: <dev-bounces@dpdk.org>
Received: from mail-ot1-f66.google.com (mail-ot1-f66.google.com
 [209.85.210.66]) by dpdk.org (Postfix) with ESMTP id 15DDD1B416
 for <dev@dpdk.org>; Mon,  3 Dec 2018 16:44:09 +0100 (CET)
Received: by mail-ot1-f66.google.com with SMTP id s5so11977454oth.7
 for <dev@dpdk.org>; Mon, 03 Dec 2018 07:44:08 -0800 (PST)
In-Reply-To: <CAKoUArkb_-se-2saBL5Yx_2zksUQVdAEm45mOf6C5jRY_8z72g@mail.gmail.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi,

The same comment refers also to V2 of the patch, [PATCH v2 03/20]
net/ice: support device and queue ops

Regards,
Rami Rosen

On Mon, 3 Dec 2018 at 17:24, Rami Rosen <roszenrami@gmail.com> wrote:
>
> Hi, Wenzhuo,
>
> > +static int
> > +ice_dev_start(struct rte_eth_dev *dev)
> > +{
> > +       struct rte_eth_dev_data *data = dev->data;
> > +       struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +       struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> > +       uint16_t nb_rxq = 0;
> > +       uint16_t nb_txq, i;
> > +       int ret;
> > +
> > +       if (rte_eal_process_type() == RTE_PROC_SECONDARY)
> > +               return -E_RTE_SECONDARY;
> > +
> [Rami Rosen] Suppose start of a TX queue failes in the loop below. You
> go to **tx_err** label, where you stop
> all **RX** queues (which actually were not started at all, since they
> are started only later in this method; and then you
> return -EIO and the ice_dev_start() method is terminated, without
> actually stopping any TX queues which were already started;
> So maybe it is better to call   ice_tx_queue_stop() in tx_err and
> ice_rx_queue_stop() in rx_err.
> Apart from it, there is a typo:  "Tx queues' contex" should be =>Tx
> queues' context"
>
> > +       /* program Tx queues' contex in hardware */
> > +       for (nb_txq = 0; nb_txq < data->nb_tx_queues; nb_txq++) {
> > +               ret = ice_tx_queue_start(dev, nb_txq);
> > +               if (ret) {
> > +                       PMD_DRV_LOG(ERR, "fail to start Tx queue %u", nb_txq);
> > +                       goto tx_err;
> > +               }
> > +       }
> > +
>
> > +       /* program Rx queues' context in hardware*/
> > +       for (nb_rxq = 0; nb_rxq < data->nb_rx_queues; nb_rxq++) {
> > +               ret = ice_rx_queue_start(dev, nb_rxq);
> > +               if (ret) {
> > +                       PMD_DRV_LOG(ERR, "fail to start Rx queue %u", nb_rxq);
> > +                       goto rx_err;
> > +               }
> > +       }
> ,,,
>
> > +       /* stop the started queues if failed to start all queues */
> > +rx_err:
> > +       for (i = 0; i < nb_txq; i++)
> > +               ice_tx_queue_stop(dev, i);
> > +tx_err:
> > +       for (i = 0; i < nb_rxq; i++)
> > +               ice_rx_queue_stop(dev, i);
> > +
> > +       return -EIO;
> > +}
> > +
>
> Regards,
> Rami Rosen