-----Original Message----- From: Ilya Maximets <i.maxim...@samsung.com> Sent: 10 September 2019 11:44 To: Konieczny, TomaszX <tomaszx.koniec...@intel.com>; d...@openvswitch.org Cc: Stokes, Ian <ian.sto...@intel.com> Subject: Re: [ovs-dev] [PATCH v1] netdev-dpdk: Fix flow control configuration.
On 10.09.2019 12:29, Ilya Maximets wrote: > On 09.09.2019 14:39, Tomasz Konieczny wrote: >> Currently OVS is unable to change flow control configuration in DPDK >> because new settings are being overwritten by current settings with >> rte_eth_dev_flow_ctrl_get(). The fix restores correct order of >> operations and at the same time does not trigger error on devices >> without flow control support when flow control not requested. >> >> Fixes: d95a8d2b0bd6 ("netdev-dpdk: Fix flow control configuration.") >> Signed-off-by: Tomasz Konieczny <tomaszx.koniec...@intel.com> >> --- > > Hi Tomasz, > Thanks for the fix! > > I agree that current code in master doesn't make any sense. :) It > seems that no-one uses this functionality since it's broken for more > than a year already. > > Fixes tag should point to the commit from master branch, so it should > be: > Fixes: 7e1de65e8dfb ("netdev-dpdk: Fix failure to configure flow > control at netdev-init.") > > Regarding the fix itself: > Can we just move following two lines: > > dev->fc_conf.mode = fc_mode; > dev->fc_conf.autoneg = autoneg; > > below the rte_eth_dev_flow_ctrl_get() ? > Current version of the patch will re-setup flow control on each call > if it is not in initial state. One more nit is that it's, probably, better to re-name this patch to be more specific. Especially because we already have equally named patch on other branch. (The patch still needs to have v2 in a subject prefix with re-naming mentioned in a version history.) > > Best regards, Ilya Maximets. Hi Ilya, Thank you for your comments. I will adhere to tag and name issues. Regarding the fix itself comment: These lines need to be run separately from get() in order to allow disabling already enabled flow control - get() will not run in this case. Also, as far as I understand, these lines and re-setup will only run when there is some change in flow control settings. if (dev->fc_conf.mode != fc_mode || autoneg != dev->fc_conf.autoneg) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev