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, 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
