>
> Hi Ciara,
> Please find my comments below,
>
>
> Regards
> _Sugesh
>
>
> > -----Original Message-----
> > From: Loftus, Ciara
> > Sent: Monday, August 15, 2016 10:01 AM
> > To: Chandran, Sugesh <sugesh.chand...@intel.com>;
> dev@openvswitch.org
> > Subject: RE: [ovs-dev] [PATCH] netdev-dpdk: Remove unnecessary 'if'
> > statement
> >
> > >
> > > Hi Ciara,
> > > Thank you for fixing this.
> > > Changes are looks fine for me.
> > > A minor comment as below.
> > > Acked!
> > >
> > >
> > > Regards
> > > _Sugesh
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ciara
> > > Loftus
> > > > Sent: Friday, August 12, 2016 5:17 PM
> > > > To: dev@openvswitch.org
> > > > Subject: [ovs-dev] [PATCH] netdev-dpdk: Remove unnecessary 'if'
> > > > statement
> > > >
> > > > Only devices of type "DPDK_DEV_ETH" use the
> netdev_dpdk_set_config
> > > > function, so no need to check for the device type within the function.
> > > >
> > > > Fixes: 9fd39370c12c ("netdev-dpdk: Add Flow Control support.")
> > > > Signed-off-by: Ciara Loftus <ciara.lof...@intel.com>
> > > > ---
> > > > lib/netdev-dpdk.c | 27 +++++++++++++--------------
> > > > 1 file changed, 13 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > > > 9a1f7cd..f772998
> > > > 100644
> > > > --- a/lib/netdev-dpdk.c
> > > > +++ b/lib/netdev-dpdk.c
> > > > @@ -1024,20 +1024,19 @@ netdev_dpdk_set_config(struct netdev
> > > *netdev,
> > > > const struct smap *args)
> > > > }
> > > >
> > > > /* Flow control configuration for DPDK Ethernet ports. */
> > > > - if (dev->type == DPDK_DEV_ETH) {
> > > > - bool rx_fc_en = false;
> > > > - bool tx_fc_en = false;
> > > > - enum rte_eth_fc_mode fc_mode_set[2][2] =
> > > > - {{RTE_FC_NONE,
> > > > RTE_FC_TX_PAUSE},
> > > > - {RTE_FC_RX_PAUSE,
> > > > RTE_FC_FULL}
> > > > - };
> > > > - rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
> > > > - tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
> > > > - dev->fc_conf.autoneg = smap_get_bool(args, "flow-ctrl-autoneg",
> > > > false);
> > > > - dev->fc_conf.mode = fc_mode_set[tx_fc_en][rx_fc_en];
> > > > -
> > > > - dpdk_eth_flow_ctrl_setup(dev);
> > > > - }
> > > [Sugesh] I would add a comment to say that the flow control is
> > > supported only on Eth/physical ports for better readability.
> >
> > Thanks for the review Sugesh, I'll improve the comment for the v2.
> > Out of interest, do 'dpdkr' ivshm ports support flow ctrl? At the moment
> > (with and without this patch) we attempt to initialise flow ctrl for these
> ports.
> > If that's not intended behaviour I'll fix that in v2 as well.
> [Sugesh] Good catch, the flow control can be enabled only on physical NIC
> ports.
> Ivshm ports doesn’t have flow control support. Can you please add that
> check as well,?
Thanks for clarifying. I've added this change in the v2.
I didn't add your Ack as the patch has changed quite a bit since, but will add
again if you're happy with the new patch.
Thanks,
Ciara
>
> Thanks,
> Sugesh
>
> >
> > Thanks,
> > Ciara
> >
> > > > + bool rx_fc_en = false;
> > > > + bool tx_fc_en = false;
> > > > + enum rte_eth_fc_mode fc_mode_set[2][2] =
> > > > + {{RTE_FC_NONE, RTE_FC_TX_PAUSE},
> > > > + {RTE_FC_RX_PAUSE, RTE_FC_FULL}
> > > > + };
> > > > + rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
> > > > + tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
> > > > + dev->fc_conf.autoneg = smap_get_bool(args, "flow-ctrl-autoneg",
> > > false);
> > > > + dev->fc_conf.mode = fc_mode_set[tx_fc_en][rx_fc_en];
> > > > +
> > > > + dpdk_eth_flow_ctrl_setup(dev);
> > > > +
> > > > ovs_mutex_unlock(&dev->mutex);
> > > >
> > > > return 0;
> > > > --
> > > > 2.4.3
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev@openvswitch.org
> > > > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
