From: Dapeng Yu <dapengx.yu@intel.com> Currently max frame size is set at queue level, which makes the values of the following counters wrong when a jumbo frame is received. The expected value: rx_good_bytes: 0 rx_errors: 1 rx_oversize_errors: 1 The actual value: rx_good_bytes: 1626 rx_errors: 0 rx_oversize_errors: 0 This patch enables setting max frame size at port level, and makes the values above right. Cc: stable@dpdk.org Signed-off-by: Dapeng Yu <dapengx.yu@intel.com> --- drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index c0bfff43ee..ef9b2b2414 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr); static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); static int i40e_ethertype_filter_convert( const struct rte_eth_ethertype_filter *input, @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused) */ i40e_add_tx_flow_control_drop_filter(pf); - /* Set the max frame size to 0x2600 by default, - * in case other drivers changed the default value. - */ - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL); - /* initialize RSS rule list */ TAILQ_INIT(&pf->rss_config_list); @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev) uint32_t intr_vector = 0; struct i40e_vsi *vsi; uint16_t nb_rxq, nb_txq; + uint16_t max_frame_size; hw->adapter_stopped = 0; @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev) "please call hierarchy_commit() " "before starting the port"); + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; + i40e_set_mac_max_frame(dev, max_frame_size); + return I40E_SUCCESS; tx_err: @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) return i40e_phy_conf_link(hw, abilities, speed, false); } +#define CHECK_INTERVAL 100 /* 100ms */ +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ + static __rte_always_inline void update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@ -2914,8 +2917,6 @@ static __rte_always_inline void update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, bool enable_lse, int wait_to_complete) { -#define CHECK_INTERVAL 100 /* 100ms */ -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ uint32_t rep_cnt = MAX_REPEAT_TIME; struct i40e_link_status link_status; int status; @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) if (!ret) rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL); + break; default: PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) return ret; } +static void +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) +{ + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); + uint32_t rep_cnt = MAX_REPEAT_TIME; + struct rte_eth_link link; + enum i40e_status_code status; + + do { + update_link_reg(hw, &link); + if (link.link_status) + break; + + rte_delay_ms(CHECK_INTERVAL); + } while (--rep_cnt); + + if (link.link_status) { + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL); + if (status != I40E_SUCCESS) + PMD_DRV_LOG(ERR, "Failed to set max frame size at port level"); + } else { + PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down"); + } +} + RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE); RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE); #ifdef RTE_ETHDEV_DEBUG_RX -- 2.27.0
> -----Original Message-----
> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> Sent: Tuesday, December 7, 2021 5:00 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: enable max frame size at port level
>
> From: Dapeng Yu <dapengx.yu@intel.com>
>
> Currently max frame size is set at queue level, which makes the values of the
> following counters wrong when a jumbo frame is received.
>
> The expected value:
> rx_good_bytes: 0
> rx_errors: 1
> rx_oversize_errors: 1
>
> The actual value:
> rx_good_bytes: 1626
> rx_errors: 0
> rx_oversize_errors: 0
>
> This patch enables setting max frame size at port level, and makes the values
> above right.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> --
Tested-by: Peng Zhang <peng1x.zhang@intel.com>
> -----Original Message-----
> From: Zhang, Peng1X <peng1x.zhang@intel.com>
> Sent: Monday, February 21, 2022 6:43 PM
> To: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/i40e: enable max frame size at port level
>
> > -----Original Message-----
> > From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> > Sent: Tuesday, December 7, 2021 5:00 PM
> > To: Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> > Subject: [PATCH] net/i40e: enable max frame size at port level
> >
> > From: Dapeng Yu <dapengx.yu@intel.com>
> >
> > Currently max frame size is set at queue level, which makes the values
> > of the following counters wrong when a jumbo frame is received.
> >
> > The expected value:
> > rx_good_bytes: 0
> > rx_errors: 1
> > rx_oversize_errors: 1
> >
> > The actual value:
> > rx_good_bytes: 1626
> > rx_errors: 0
> > rx_oversize_errors: 0
> >
> > This patch enables setting max frame size at port level, and makes the
> > values above right.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> > --
> Tested-by: Peng Zhang <peng1x.zhang@intel.com>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel.
Thanks
Qi
On 21/02/2022 11:22, Zhang, Qi Z wrote: > > >> -----Original Message----- >> From: Zhang, Peng1X <peng1x.zhang@intel.com> >> Sent: Monday, February 21, 2022 6:43 PM >> To: dev@dpdk.org; stable@dpdk.org >> Subject: RE: [PATCH] net/i40e: enable max frame size at port level >> >>> -----Original Message----- >>> From: dapengx.yu@intel.com <dapengx.yu@intel.com> >>> Sent: Tuesday, December 7, 2021 5:00 PM >>> To: Xing, Beilei <beilei.xing@intel.com> >>> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org >>> Subject: [PATCH] net/i40e: enable max frame size at port level >>> >>> From: Dapeng Yu <dapengx.yu@intel.com> >>> >>> Currently max frame size is set at queue level, which makes the values >>> of the following counters wrong when a jumbo frame is received. >>> >>> The expected value: >>> rx_good_bytes: 0 >>> rx_errors: 1 >>> rx_oversize_errors: 1 >>> >>> The actual value: >>> rx_good_bytes: 1626 >>> rx_errors: 0 >>> rx_oversize_errors: 0 >>> >>> This patch enables setting max frame size at port level, and makes the >>> values above right. >>> >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com> >>> -- >> Tested-by: Peng Zhang <peng1x.zhang@intel.com> > > Reviewed-by: Qi Zhang <qi.z.zhang@intel.com> > > Applied to dpdk-next-net-intel. > Please add a Fixes: tag so we can know which LTS it is relevant for. > Thanks > Qi >
On Tue, Dec 7, 2021 at 10:02 AM <dapengx.yu@intel.com> wrote: > > From: Dapeng Yu <dapengx.yu@intel.com> > > Currently max frame size is set at queue level, which makes the values > of the following counters wrong when a jumbo frame is received. > > The expected value: > rx_good_bytes: 0 > rx_errors: 1 > rx_oversize_errors: 1 > > The actual value: > rx_good_bytes: 1626 > rx_errors: 0 > rx_oversize_errors: 0 > > This patch enables setting max frame size at port level, and makes the > values above right. > > Cc: stable@dpdk.org > > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com> I am looking again at this patch that triggered regressions in stable branches, when used with OVS. > --- > drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------ > 1 file changed, 34 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index c0bfff43ee..ef9b2b2414 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev, > struct rte_ether_addr *mac_addr); > > static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); > +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); > > static int i40e_ethertype_filter_convert( > const struct rte_eth_ethertype_filter *input, > @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused) > */ > i40e_add_tx_flow_control_drop_filter(pf); > > - /* Set the max frame size to 0x2600 by default, > - * in case other drivers changed the default value. > - */ > - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL); > - If I understand correctly, the max framesize (9728) was always applied in dev_init, before this change. > /* initialize RSS rule list */ > TAILQ_INIT(&pf->rss_config_list); > > @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > uint32_t intr_vector = 0; > struct i40e_vsi *vsi; > uint16_t nb_rxq, nb_txq; > + uint16_t max_frame_size; > > hw->adapter_stopped = 0; > > @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev) > "please call hierarchy_commit() " > "before starting the port"); > > + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; > + i40e_set_mac_max_frame(dev, max_frame_size); > + And now this change tries to configure, at dev_start step, the max frame size that the application actually requested. Which seems ok, afaiu. > return I40E_SUCCESS; > > tx_err: > @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) > return i40e_phy_conf_link(hw, abilities, speed, false); > } > > +#define CHECK_INTERVAL 100 /* 100ms */ > +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > + > static __rte_always_inline void > update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) > { > @@ -2914,8 +2917,6 @@ static __rte_always_inline void > update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, > bool enable_lse, int wait_to_complete) > { > -#define CHECK_INTERVAL 100 /* 100ms */ > -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > uint32_t rep_cnt = MAX_REPEAT_TIME; > struct i40e_link_status link_status; > int status; > @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) > if (!ret) > rte_eth_dev_callback_process(dev, > RTE_ETH_EVENT_INTR_LSC, NULL); > + > break; > default: > PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", > @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) > return ret; > } > > +static void > +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) > +{ > + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + uint32_t rep_cnt = MAX_REPEAT_TIME; > + struct rte_eth_link link; > + enum i40e_status_code status; > + > + do { > + update_link_reg(hw, &link); > + if (link.link_status) > + break; > + > + rte_delay_ms(CHECK_INTERVAL); > + } while (--rep_cnt); > + > + if (link.link_status) { > + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL); > + if (status != I40E_SUCCESS) > + PMD_DRV_LOG(ERR, "Failed to set max frame size at port level"); > + } else { > + PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down"); > + } But here, applying the configuration when the link is up only, looks very wrong to me. If the link is down, then this configuration will never get applied. And what is worse, the application has no indication of it since no error code is propagated. So the application thinks the port started, but it will never get "big" packets. Like for example: - unplug cable, - start testpmd and configure mtu to 9000, # dpdk-testpmd -l 0,2,3 --in-memory -a 0000:05:00.1 -- -i --disable-link-check ... Configuring Port 0 (socket 0) i40e_set_mac_max_frame(): Set max frame size at port level not applicable on link down Port 0: 3C:FD:FE:A0:A4:C5 Done testpmd> port stop 0 Stopping ports... Done testpmd> port config mtu 0 9000 testpmd> port start 0 i40e_set_mac_max_frame(): Set max frame size at port level not applicable on link down Port 0: 3C:FD:FE:A0:A4:C5 Done - plug back cable, and send "big" packets, like for example ping with -s 8000 from other side of the cable, - no "big" packet is received on the dpdk port, and its rx_errors xstats keeps increasing, Looking at the original change, why did you not ignore link status state? I would just move "i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL);" in dev_start. -- David Marchand
On Mon, Dec 12, 2022 at 2:53 PM David Marchand <david.marchand@redhat.com> wrote: > On Tue, Dec 7, 2021 at 10:02 AM <dapengx.yu@intel.com> wrote: > > > > From: Dapeng Yu <dapengx.yu@intel.com> I got a bounce on his mail address, so adding people who submitted/reviewed later fixes for this change. Please guys, have a look. > > > > Currently max frame size is set at queue level, which makes the values > > of the following counters wrong when a jumbo frame is received. > > > > The expected value: > > rx_good_bytes: 0 > > rx_errors: 1 > > rx_oversize_errors: 1 > > > > The actual value: > > rx_good_bytes: 1626 > > rx_errors: 0 > > rx_oversize_errors: 0 > > > > This patch enables setting max frame size at port level, and makes the > > values above right. > > > > Cc: stable@dpdk.org > > > > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com> > > I am looking again at this patch that triggered regressions in stable > branches, when used with OVS. > > > > --- > > drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------ > > 1 file changed, 34 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > > index c0bfff43ee..ef9b2b2414 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev, > > struct rte_ether_addr *mac_addr); > > > > static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); > > +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); > > > > static int i40e_ethertype_filter_convert( > > const struct rte_eth_ethertype_filter *input, > > @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused) > > */ > > i40e_add_tx_flow_control_drop_filter(pf); > > > > - /* Set the max frame size to 0x2600 by default, > > - * in case other drivers changed the default value. > > - */ > > - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL); > > - > > If I understand correctly, the max framesize (9728) was always applied > in dev_init, before this change. > > > > /* initialize RSS rule list */ > > TAILQ_INIT(&pf->rss_config_list); > > > > @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev) > > uint32_t intr_vector = 0; > > struct i40e_vsi *vsi; > > uint16_t nb_rxq, nb_txq; > > + uint16_t max_frame_size; > > > > hw->adapter_stopped = 0; > > > > @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev) > > "please call hierarchy_commit() " > > "before starting the port"); > > > > + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; > > + i40e_set_mac_max_frame(dev, max_frame_size); > > + > > And now this change tries to configure, at dev_start step, the max > frame size that the application actually requested. > Which seems ok, afaiu. > > > > return I40E_SUCCESS; > > > > tx_err: > > @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) > > return i40e_phy_conf_link(hw, abilities, speed, false); > > } > > > > +#define CHECK_INTERVAL 100 /* 100ms */ > > +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > + > > static __rte_always_inline void > > update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) > > { > > @@ -2914,8 +2917,6 @@ static __rte_always_inline void > > update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, > > bool enable_lse, int wait_to_complete) > > { > > -#define CHECK_INTERVAL 100 /* 100ms */ > > -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > uint32_t rep_cnt = MAX_REPEAT_TIME; > > struct i40e_link_status link_status; > > int status; > > @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) > > if (!ret) > > rte_eth_dev_callback_process(dev, > > RTE_ETH_EVENT_INTR_LSC, NULL); > > + > > break; > > default: > > PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", > > @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) > > return ret; > > } > > > > +static void > > +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) > > +{ > > + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > + uint32_t rep_cnt = MAX_REPEAT_TIME; > > + struct rte_eth_link link; > > + enum i40e_status_code status; > > + > > + do { > > + update_link_reg(hw, &link); > > + if (link.link_status) > > + break; > > + > > + rte_delay_ms(CHECK_INTERVAL); > > + } while (--rep_cnt); > > + > > + if (link.link_status) { > > + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL); > > + if (status != I40E_SUCCESS) > > + PMD_DRV_LOG(ERR, "Failed to set max frame size at port level"); > > + } else { > > + PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down"); > > + } > > But here, applying the configuration when the link is up only, looks > very wrong to me. > > If the link is down, then this configuration will never get applied. > And what is worse, the application has no indication of it since no > error code is propagated. > > So the application thinks the port started, but it will never get "big" packets. > > > Like for example: > - unplug cable, > - start testpmd and configure mtu to 9000, > # dpdk-testpmd -l 0,2,3 --in-memory -a 0000:05:00.1 -- -i --disable-link-check > ... > Configuring Port 0 (socket 0) > i40e_set_mac_max_frame(): Set max frame size at port level not > applicable on link down > Port 0: 3C:FD:FE:A0:A4:C5 > Done > > testpmd> port stop 0 > Stopping ports... > Done > > testpmd> port config mtu 0 9000 > > testpmd> port start 0 > i40e_set_mac_max_frame(): Set max frame size at port level not > applicable on link down > Port 0: 3C:FD:FE:A0:A4:C5 > Done > > - plug back cable, and send "big" packets, like for example ping with > -s 8000 from other side of the cable, > - no "big" packet is received on the dpdk port, and its rx_errors > xstats keeps increasing, > > > Looking at the original change, why did you not ignore link status state? > I would just move "i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, > TRUE, false, 0, NULL);" in dev_start. > > -- David Marchand
This is a tentative fix, reverting mentionned changes below and only moving max frame size configuration to dev_start. Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level") Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level") Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722") Cc: stable@dpdk.org Signed-off-by: David Marchand <david.marchand@redhat.com> --- drivers/net/i40e/i40e_ethdev.c | 46 +++------------------------------- 1 file changed, 4 insertions(+), 42 deletions(-) diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 7726a89d99..5635dd03cf 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -387,7 +387,6 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr); static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); static int i40e_ethertype_filter_convert( const struct rte_eth_ethertype_filter *input, @@ -2328,7 +2327,6 @@ i40e_dev_start(struct rte_eth_dev *dev) uint32_t intr_vector = 0; struct i40e_vsi *vsi; uint16_t nb_rxq, nb_txq; - uint16_t max_frame_size; hw->adapter_stopped = 0; @@ -2467,8 +2465,8 @@ i40e_dev_start(struct rte_eth_dev *dev) "please call hierarchy_commit() " "before starting the port"); - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; - i40e_set_mac_max_frame(dev, max_frame_size); + i40e_aq_set_mac_config(hw, dev->data->mtu + I40E_ETH_OVERHEAD, TRUE, + false, 0, NULL); return I40E_SUCCESS; @@ -2809,9 +2807,6 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) return i40e_phy_conf_link(hw, abilities, speed, false); } -#define CHECK_INTERVAL 100 /* 100ms */ -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ - static __rte_always_inline void update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@ -2878,6 +2873,8 @@ static __rte_always_inline void update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, bool enable_lse, int wait_to_complete) { +#define CHECK_INTERVAL 100 /* 100ms */ +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ uint32_t rep_cnt = MAX_REPEAT_TIME; struct i40e_link_status link_status; int status; @@ -6738,7 +6735,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) if (!ret) rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL); - break; default: PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", @@ -12123,40 +12119,6 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) return ret; } -static void -i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) -{ - struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); - uint32_t rep_cnt = MAX_REPEAT_TIME; - struct rte_eth_link link; - enum i40e_status_code status; - bool can_be_set = true; - - /* - * I40E_MEDIA_TYPE_BASET link up can be ignored - * I40E_MEDIA_TYPE_BASET link down that hw->phy.media_type - * is I40E_MEDIA_TYPE_UNKNOWN - */ - if (hw->phy.media_type != I40E_MEDIA_TYPE_BASET && - hw->phy.media_type != I40E_MEDIA_TYPE_UNKNOWN) { - do { - update_link_reg(hw, &link); - if (link.link_status) - break; - rte_delay_ms(CHECK_INTERVAL); - } while (--rep_cnt); - can_be_set = !!link.link_status; - } - - if (can_be_set) { - status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL); - if (status != I40E_SUCCESS) - PMD_DRV_LOG(ERR, "Failed to set max frame size at port level"); - } else { - PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down"); - } -} - RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE); RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE); #ifdef RTE_ETHDEV_DEBUG_RX -- 2.38.1
The mentioned changes broke existing applications when the link status of i40e ports is down at the time the port is started. Revert those changes, the original issue will need a different fix. Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level") Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level") Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722") Cc: stable@dpdk.org Signed-off-by: David Marchand <david.marchand@redhat.com> --- Changes since v1: - since the CI reports a failure on v1, simplified the fix by only reverting commits, --- drivers/net/i40e/i40e_ethdev.c | 50 +++++----------------------------- 1 file changed, 7 insertions(+), 43 deletions(-) diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 7726a89d99..a982e42264 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -387,7 +387,6 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr); static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size); static int i40e_ethertype_filter_convert( const struct rte_eth_ethertype_filter *input, @@ -1711,6 +1710,11 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused) */ i40e_add_tx_flow_control_drop_filter(pf); + /* Set the max frame size to 0x2600 by default, + * in case other drivers changed the default value. + */ + i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL); + /* initialize RSS rule list */ TAILQ_INIT(&pf->rss_config_list); @@ -2328,7 +2332,6 @@ i40e_dev_start(struct rte_eth_dev *dev) uint32_t intr_vector = 0; struct i40e_vsi *vsi; uint16_t nb_rxq, nb_txq; - uint16_t max_frame_size; hw->adapter_stopped = 0; @@ -2467,9 +2470,6 @@ i40e_dev_start(struct rte_eth_dev *dev) "please call hierarchy_commit() " "before starting the port"); - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; - i40e_set_mac_max_frame(dev, max_frame_size); - return I40E_SUCCESS; tx_err: @@ -2809,9 +2809,6 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev) return i40e_phy_conf_link(hw, abilities, speed, false); } -#define CHECK_INTERVAL 100 /* 100ms */ -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ - static __rte_always_inline void update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@ -2878,6 +2875,8 @@ static __rte_always_inline void update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, bool enable_lse, int wait_to_complete) { +#define CHECK_INTERVAL 100 /* 100ms */ +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ uint32_t rep_cnt = MAX_REPEAT_TIME; struct i40e_link_status link_status; int status; @@ -6738,7 +6737,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev) if (!ret) rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL); - break; default: PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", @@ -12123,40 +12121,6 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf) return ret; } -static void -i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) -{ - struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); - uint32_t rep_cnt = MAX_REPEAT_TIME; - struct rte_eth_link link; - enum i40e_status_code status; - bool can_be_set = true; - - /* - * I40E_MEDIA_TYPE_BASET link up can be ignored - * I40E_MEDIA_TYPE_BASET link down that hw->phy.media_type - * is I40E_MEDIA_TYPE_UNKNOWN - */ - if (hw->phy.media_type != I40E_MEDIA_TYPE_BASET && - hw->phy.media_type != I40E_MEDIA_TYPE_UNKNOWN) { - do { - update_link_reg(hw, &link); - if (link.link_status) - break; - rte_delay_ms(CHECK_INTERVAL); - } while (--rep_cnt); - can_be_set = !!link.link_status; - } - - if (can_be_set) { - status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL); - if (status != I40E_SUCCESS) - PMD_DRV_LOG(ERR, "Failed to set max frame size at port level"); - } else { - PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down"); - } -} - RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE); RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE); #ifdef RTE_ETHDEV_DEBUG_RX -- 2.38.1
Hi i40e maintainers,
On Tue, Dec 13, 2022 at 10:19 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> The mentioned changes broke existing applications when the link status
> of i40e ports is down at the time the port is started.
> Revert those changes, the original issue will need a different fix.
>
> Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level")
> Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
This issue was reported only by our QE, but I suspect that real
deployments will get affected.
Please review.
Thanks.
--
David Marchand
On Tue, Jan 3, 2023 at 3:02 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Hi i40e maintainers,
>
> On Tue, Dec 13, 2022 at 10:19 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > The mentioned changes broke existing applications when the link status
> > of i40e ports is down at the time the port is started.
> > Revert those changes, the original issue will need a different fix.
> >
> > Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> > Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level")
> > Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> This issue was reported only by our QE, but I suspect that real
> deployments will get affected.
> Please review.
Ping.
--
David Marchand
Hello i40e maintainers, John,
On Mon, Jan 9, 2023 at 10:21 AM David Marchand
<david.marchand@redhat.com> wrote:
> On Tue, Jan 3, 2023 at 3:02 PM David Marchand <david.marchand@redhat.com> wrote:
> > Hi i40e maintainers,
> >
> > On Tue, Dec 13, 2022 at 10:19 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > The mentioned changes broke existing applications when the link status
> > > of i40e ports is down at the time the port is started.
> > > Revert those changes, the original issue will need a different fix.
> > >
> > > Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> > > Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level")
> > > Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> >
> > This issue was reported only by our QE, but I suspect that real
> > deployments will get affected.
> > Please review.
>
> Ping.
Ping.
Note: I removed Jie Wang from Cc list since I get bounces.
--
David Marchand
> -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Friday, January 13, 2023 9:33 PM > To: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com> > Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > <wenxuanx.wu@intel.com> > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start > > Hello i40e maintainers, John, > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand > <david.marchand@redhat.com> wrote: > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand > <david.marchand@redhat.com> wrote: > > > Hi i40e maintainers, > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand > > > <david.marchand@redhat.com> wrote: > > > > > > > > The mentioned changes broke existing applications when the link > > > > status of i40e ports is down at the time the port is started. > > > > Revert those changes, the original issue will need a different fix. Hi David Does it break all the application or just a specific application? We may need to understand the issue you met, and try to fix it later. Thank you very much for reporting it out! Regards, Helin > > > > > > > > Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port > > > > level") > > > > Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port > > > > level") > > > > Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > > > > > This issue was reported only by our QE, but I suspect that real > > > deployments will get affected. > > > Please review. > > > > Ping. > > Ping. > > Note: I removed Jie Wang from Cc list since I get bounces. > > -- > David Marchand
On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin <helin.zhang@intel.com> wrote: > > > > > -----Original Message----- > > From: David Marchand <david.marchand@redhat.com> > > Sent: Friday, January 13, 2023 9:33 PM > > To: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com> > > Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; > > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > > <wenxuanx.wu@intel.com> > > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start > > > > Hello i40e maintainers, John, > > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand > > <david.marchand@redhat.com> wrote: > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand > > <david.marchand@redhat.com> wrote: > > > > Hi i40e maintainers, > > > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand > > > > <david.marchand@redhat.com> wrote: > > > > > > > > > > The mentioned changes broke existing applications when the link > > > > > status of i40e ports is down at the time the port is started. > > > > > Revert those changes, the original issue will need a different fix. > Hi David > > Does it break all the application or just a specific application? I don't see how it would not affect all applications seeing how the original patch is dumb. > We may need to understand the issue you met, and try to fix it later. Just unplug the cable or fake a link down on your i40e port, start your application or port, then plug the cable back. The max frame size will never get applied to hw. -- David Marchand
> -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Friday, January 13, 2023 9:47 PM > To: Zhang, Helin <helin.zhang@intel.com> > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; > dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > <wenxuanx.wu@intel.com> > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start > > On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin <helin.zhang@intel.com> wrote: > > > > > > > > > -----Original Message----- > > > From: David Marchand <david.marchand@redhat.com> > > > Sent: Friday, January 13, 2023 9:33 PM > > > To: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > > > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com> > > > Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z > > > <qi.z.zhang@intel.com>; Dapeng Yu <dapengx.yu@intel.com>; Wenxuan > Wu > > > <wenxuanx.wu@intel.com> > > > Subject: Re: [PATCH v2] net/i40e: don't check link status on device > > > start > > > > > > Hello i40e maintainers, John, > > > > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand > > > <david.marchand@redhat.com> wrote: > > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand > > > <david.marchand@redhat.com> wrote: > > > > > Hi i40e maintainers, > > > > > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand > > > > > <david.marchand@redhat.com> wrote: > > > > > > > > > > > > The mentioned changes broke existing applications when the > > > > > > link status of i40e ports is down at the time the port is started. > > > > > > Revert those changes, the original issue will need a different fix. > > Hi David > > > > Does it break all the application or just a specific application? > > I don't see how it would not affect all applications seeing how the original > patch is dumb. > > > We may need to understand the issue you met, and try to fix it later. > > Just unplug the cable or fake a link down on your i40e port, start your > application or port, then plug the cable back. > The max frame size will never get applied to hw. Got it, I will forward to a right expert to check. Thank you very much for reaching out to us! Regards, Helin > > > -- > David Marchand
On Fri, Jan 13, 2023 at 2:51 PM Zhang, Helin <helin.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Friday, January 13, 2023 9:47 PM
> > To: Zhang, Helin <helin.zhang@intel.com>
> > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
> > dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu
> > <wenxuanx.wu@intel.com>
> > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start
> >
> > On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin <helin.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > Sent: Friday, January 13, 2023 9:33 PM
> > > > To: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei
> > > > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com>
> > > > Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Dapeng Yu <dapengx.yu@intel.com>; Wenxuan
> > Wu
> > > > <wenxuanx.wu@intel.com>
> > > > Subject: Re: [PATCH v2] net/i40e: don't check link status on device
> > > > start
> > > >
> > > > Hello i40e maintainers, John,
> > > >
> > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand
> > > > <david.marchand@redhat.com> wrote:
> > > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand
> > > > <david.marchand@redhat.com> wrote:
> > > > > > Hi i40e maintainers,
> > > > > >
> > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand
> > > > > > <david.marchand@redhat.com> wrote:
> > > > > > >
> > > > > > > The mentioned changes broke existing applications when the
> > > > > > > link status of i40e ports is down at the time the port is started.
> > > > > > > Revert those changes, the original issue will need a different fix.
> > > Hi David
> > >
> > > Does it break all the application or just a specific application?
> >
> > I don't see how it would not affect all applications seeing how the original
> > patch is dumb.
> >
> > > We may need to understand the issue you met, and try to fix it later.
> >
> > Just unplug the cable or fake a link down on your i40e port, start your
> > application or port, then plug the cable back.
> > The max frame size will never get applied to hw.
> Got it, I will forward to a right expert to check. Thank you very much for reaching out to us!
I hope I get a reply _soon_.
Or I will just apply those reverts.
Thanks.
--
David Marchand
Hi David, > -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Friday, January 13, 2023 9:53 PM > To: Zhang, Helin <helin.zhang@intel.com> > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; > dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Dapeng > Yu <dapengx.yu@intel.com>; Wenxuan Wu <wenxuanx.wu@intel.com> > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start > > On Fri, Jan 13, 2023 at 2:51 PM Zhang, Helin <helin.zhang@intel.com> wrote: > > > > > > > > > -----Original Message----- > > > From: David Marchand <david.marchand@redhat.com> > > > Sent: Friday, January 13, 2023 9:47 PM > > > To: Zhang, Helin <helin.zhang@intel.com> > > > Cc: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > > > <beilei.xing@intel.com>; Mcnamara, John <john.mcnamara@intel.com>; > > > dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; > > > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > <wenxuanx.wu@intel.com> > > > Subject: Re: [PATCH v2] net/i40e: don't check link status on device > > > start > > > > > > On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin <helin.zhang@intel.com> > wrote: > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: David Marchand <david.marchand@redhat.com> > > > > > Sent: Friday, January 13, 2023 9:33 PM > > > > > To: Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > > > > > <beilei.xing@intel.com>; Mcnamara, John > > > > > <john.mcnamara@intel.com> > > > > > Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z > > > > > <qi.z.zhang@intel.com>; Dapeng Yu <dapengx.yu@intel.com>; > > > > > Wenxuan > > > Wu > > > > > <wenxuanx.wu@intel.com> > > > > > Subject: Re: [PATCH v2] net/i40e: don't check link status on > > > > > device start > > > > > > > > > > Hello i40e maintainers, John, > > > > > > > > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand > > > > > <david.marchand@redhat.com> wrote: > > > > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand > > > > > <david.marchand@redhat.com> wrote: > > > > > > > Hi i40e maintainers, > > > > > > > > > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand > > > > > > > <david.marchand@redhat.com> wrote: > > > > > > > > > > > > > > > > The mentioned changes broke existing applications when the > > > > > > > > link status of i40e ports is down at the time the port is started. > > > > > > > > Revert those changes, the original issue will need a different fix. > > > > Hi David > > > > > > > > Does it break all the application or just a specific application? > > > > > > I don't see how it would not affect all applications seeing how the > > > original patch is dumb. > > > > > > > We may need to understand the issue you met, and try to fix it later. > > > > > > Just unplug the cable or fake a link down on your i40e port, start > > > your application or port, then plug the cable back. > > > The max frame size will never get applied to hw. > > Got it, I will forward to a right expert to check. Thank you very much for > reaching out to us! > > I hope I get a reply _soon_. > Or I will just apply those reverts. If applying those reverts, some issues still exist on our side. I sent one patch to patchwork: https://patchwork.dpdk.org/project/dpdk/patch/20230116105318.19412-1-simei.su@intel.com/. You can try this patch to see whether it can solve the issue on your side. At the same time, on our side, we need to do regression test further to check if this patch will affect other cases, the regression takes some time. Thanks, Simei > > > Thanks. > > -- > David Marchand
16/01/2023 12:02, Su, Simei:
> From: David Marchand <david.marchand@redhat.com>
> > On Fri, Jan 13, 2023 at 2:51 PM Zhang, Helin <helin.zhang@intel.com> wrote:
> > > From: David Marchand <david.marchand@redhat.com>
> > > > On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin <helin.zhang@intel.com>
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > > Hello i40e maintainers, John,
> > > > > >
> > > > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand
> > > > > > <david.marchand@redhat.com> wrote:
> > > > > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand
> > > > > > <david.marchand@redhat.com> wrote:
> > > > > > > > Hi i40e maintainers,
> > > > > > > >
> > > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand
> > > > > > > > <david.marchand@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > The mentioned changes broke existing applications when the
> > > > > > > > > link status of i40e ports is down at the time the port is started.
> > > > > > > > > Revert those changes, the original issue will need a different fix.
> > > > > Hi David
> > > > >
> > > > > Does it break all the application or just a specific application?
> > > >
> > > > I don't see how it would not affect all applications seeing how the
> > > > original patch is dumb.
> > > >
> > > > > We may need to understand the issue you met, and try to fix it later.
> > > >
> > > > Just unplug the cable or fake a link down on your i40e port, start
> > > > your application or port, then plug the cable back.
> > > > The max frame size will never get applied to hw.
> > >
> > > Got it, I will forward to a right expert to check. Thank you very much for
> > reaching out to us!
> >
> > I hope I get a reply _soon_.
> > Or I will just apply those reverts.
>
> If applying those reverts, some issues still exist on our side. I sent one patch to patchwork:
> https://patchwork.dpdk.org/project/dpdk/patch/20230116105318.19412-1-simei.su@intel.com/.
> You can try this patch to see whether it can solve the issue on your side.
> At the same time, on our side, we need to do regression test further to check if this patch will affect other
> cases, the regression takes some time.
>
> Thanks,
> Simei
Simei, do you have any update in this thread?
Last reply was 3 weeks ago. I hope regression test don't take so much time.
Hi Thomas, > -----Original Message----- > From: Thomas Monjalon <thomas@monjalon.net> > Sent: Tuesday, February 7, 2023 7:32 PM > To: Zhang, Helin <helin.zhang@intel.com>; Su, Simei <simei.su@intel.com> > Cc: David Marchand <david.marchand@redhat.com>; dev@dpdk.org; Zhang, > Yuying <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; > Mcnamara, John <john.mcnamara@intel.com>; stable@dpdk.org; Zhang, Qi Z > <qi.z.zhang@intel.com>; Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > <wenxuanx.wu@intel.com> > Subject: Re: [PATCH v2] net/i40e: don't check link status on device start > > 16/01/2023 12:02, Su, Simei: > > From: David Marchand <david.marchand@redhat.com> > > > On Fri, Jan 13, 2023 at 2:51 PM Zhang, Helin <helin.zhang@intel.com> > wrote: > > > > From: David Marchand <david.marchand@redhat.com> > > > > > On Fri, Jan 13, 2023 at 2:39 PM Zhang, Helin > > > > > <helin.zhang@intel.com> > > > > > > From: David Marchand <david.marchand@redhat.com> > > > > > > > Hello i40e maintainers, John, > > > > > > > > > > > > > > On Mon, Jan 9, 2023 at 10:21 AM David Marchand > > > > > > > <david.marchand@redhat.com> wrote: > > > > > > > > On Tue, Jan 3, 2023 at 3:02 PM David Marchand > > > > > > > <david.marchand@redhat.com> wrote: > > > > > > > > > Hi i40e maintainers, > > > > > > > > > > > > > > > > > > On Tue, Dec 13, 2022 at 10:19 AM David Marchand > > > > > > > > > <david.marchand@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > The mentioned changes broke existing applications when > > > > > > > > > > the link status of i40e ports is down at the time the port is > started. > > > > > > > > > > Revert those changes, the original issue will need a different > fix. > > > > > > Hi David > > > > > > > > > > > > Does it break all the application or just a specific application? > > > > > > > > > > I don't see how it would not affect all applications seeing how > > > > > the original patch is dumb. > > > > > > > > > > > We may need to understand the issue you met, and try to fix it later. > > > > > > > > > > Just unplug the cable or fake a link down on your i40e port, > > > > > start your application or port, then plug the cable back. > > > > > The max frame size will never get applied to hw. > > > > > > > > Got it, I will forward to a right expert to check. Thank you very > > > > much for > > > reaching out to us! > > > > > > I hope I get a reply _soon_. > > > Or I will just apply those reverts. > > > > If applying those reverts, some issues still exist on our side. I sent one patch > to patchwork: > > > https://patchwork.dpdk.org/project/dpdk/patch/20230116105318.19412-1-si > mei.su@intel.com/. > > You can try this patch to see whether it can solve the issue on your side. > > At the same time, on our side, we need to do regression test further > > to check if this patch will affect other cases, the regression takes some time. > > > > Thanks, > > Simei > > Simei, do you have any update in this thread? > Last reply was 3 weeks ago. I hope regression test don't take so much time. > > The latest patch for this issue is https://patchwork.dpdk.org/project/dpdk/patch/20230202123632.56730-1-simei.su@intel.com/ which needs to be reworked further. Thanks, Simei
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, December 13, 2022 5:19 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Dapeng Yu
> <dapengx.yu@intel.com>; Wenxuan Wu <wenxuanx.wu@intel.com>; Wang,
> Jie1X <jie1x.wang@intel.com>
> Subject: [PATCH v2] net/i40e: don't check link status on device start
>
> The mentioned changes broke existing applications when the link status of
> i40e ports is down at the time the port is started.
> Revert those changes, the original issue will need a different fix.
>
> Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port level")
> Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port level")
> Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - since the CI reports a failure on v1, simplified the fix by only
> reverting commits,
>
> ---
> drivers/net/i40e/i40e_ethdev.c | 50 +++++-----------------------------
> 1 file changed, 7 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 7726a89d99..a982e42264 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -387,7 +387,6 @@ static int i40e_set_default_mac_addr(struct
> rte_eth_dev *dev,
> struct rte_ether_addr *mac_addr);
>
> static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); -static
> void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size);
>
> static int i40e_ethertype_filter_convert(
> const struct rte_eth_ethertype_filter *input, @@ -1711,6 +1710,11 @@
> eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused)
> */
> i40e_add_tx_flow_control_drop_filter(pf);
>
> + /* Set the max frame size to 0x2600 by default,
> + * in case other drivers changed the default value.
> + */
> + i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0,
> NULL);
> +
> /* initialize RSS rule list */
> TAILQ_INIT(&pf->rss_config_list);
>
> @@ -2328,7 +2332,6 @@ i40e_dev_start(struct rte_eth_dev *dev)
> uint32_t intr_vector = 0;
> struct i40e_vsi *vsi;
> uint16_t nb_rxq, nb_txq;
> - uint16_t max_frame_size;
>
> hw->adapter_stopped = 0;
>
> @@ -2467,9 +2470,6 @@ i40e_dev_start(struct rte_eth_dev *dev)
> "please call hierarchy_commit() "
> "before starting the port");
>
> - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
> - i40e_set_mac_max_frame(dev, max_frame_size);
> -
> return I40E_SUCCESS;
>
> tx_err:
> @@ -2809,9 +2809,6 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
> return i40e_phy_conf_link(hw, abilities, speed, false); }
>
> -#define CHECK_INTERVAL 100 /* 100ms */
> -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> -
> static __rte_always_inline void
> update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@
> -2878,6 +2875,8 @@ static __rte_always_inline void update_link_aq(struct
> i40e_hw *hw, struct rte_eth_link *link,
> bool enable_lse, int wait_to_complete) {
> +#define CHECK_INTERVAL 100 /* 100ms */
> +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> uint32_t rep_cnt = MAX_REPEAT_TIME;
> struct i40e_link_status link_status;
> int status;
> @@ -6738,7 +6737,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
> if (!ret)
> rte_eth_dev_callback_process(dev,
> RTE_ETH_EVENT_INTR_LSC, NULL);
> -
> break;
> default:
> PMD_DRV_LOG(DEBUG, "Request %u is not supported yet", @@
> -12123,40 +12121,6 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
> return ret;
> }
>
> -static void
> -i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) -{
> - struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> - uint32_t rep_cnt = MAX_REPEAT_TIME;
> - struct rte_eth_link link;
> - enum i40e_status_code status;
> - bool can_be_set = true;
> -
> - /*
> - * I40E_MEDIA_TYPE_BASET link up can be ignored
> - * I40E_MEDIA_TYPE_BASET link down that hw->phy.media_type
> - * is I40E_MEDIA_TYPE_UNKNOWN
> - */
> - if (hw->phy.media_type != I40E_MEDIA_TYPE_BASET &&
> - hw->phy.media_type != I40E_MEDIA_TYPE_UNKNOWN) {
> - do {
> - update_link_reg(hw, &link);
> - if (link.link_status)
> - break;
> - rte_delay_ms(CHECK_INTERVAL);
> - } while (--rep_cnt);
> - can_be_set = !!link.link_status;
> - }
> -
> - if (can_be_set) {
> - status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL);
> - if (status != I40E_SUCCESS)
> - PMD_DRV_LOG(ERR, "Failed to set max frame size at port
> level");
> - } else {
> - PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable
> on link down");
> - }
> -}
> -
> RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
> RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE); #ifdef
> RTE_ETHDEV_DEBUG_RX
> --
> 2.38.1
Acked-by: Simei Su <simei.su@intel.com>
Thanks,
Simei
> -----Original Message----- > From: Su, Simei <simei.su@intel.com> > Sent: Monday, March 6, 2023 2:54 PM > To: David Marchand <david.marchand@redhat.com>; dev@dpdk.org > Cc: stable@dpdk.org; Zhang, Yuying <yuying.zhang@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > Subject: RE: [PATCH v2] net/i40e: don't check link status on device start > > > > > -----Original Message----- > > From: David Marchand <david.marchand@redhat.com> > > Sent: Tuesday, December 13, 2022 5:19 PM > > To: dev@dpdk.org > > Cc: stable@dpdk.org; Zhang, Yuying <yuying.zhang@intel.com>; Xing, > > Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; > > Dapeng Yu <dapengx.yu@intel.com>; Wenxuan Wu > <wenxuanx.wu@intel.com>; > > Wang, Jie1X <jie1x.wang@intel.com> > > Subject: [PATCH v2] net/i40e: don't check link status on device start > > > > The mentioned changes broke existing applications when the link status > > of i40e ports is down at the time the port is started. > > Revert those changes, the original issue will need a different fix. > > > > Fixes: a4ba77367923 ("net/i40e: enable maximum frame size at port > > level") > > Fixes: 2184f7cdeeaa ("net/i40e: fix max frame size config at port > > level") > > Fixes: 719469f13b11 ("net/i40e: fix jumbo frame Rx with X722") > > Cc: stable@dpdk.org > > > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > Changes since v1: > > - since the CI reports a failure on v1, simplified the fix by only > > reverting commits, > > > > --- > > drivers/net/i40e/i40e_ethdev.c | 50 > > +++++----------------------------- > > 1 file changed, 7 insertions(+), 43 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index 7726a89d99..a982e42264 > 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -387,7 +387,6 @@ static int i40e_set_default_mac_addr(struct > > rte_eth_dev *dev, > > struct rte_ether_addr *mac_addr); > > > > static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); > > -static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t > > size); > > > > static int i40e_ethertype_filter_convert( > > const struct rte_eth_ethertype_filter *input, @@ -1711,6 +1710,11 > @@ > > eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params > __rte_unused) > > */ > > i40e_add_tx_flow_control_drop_filter(pf); > > > > + /* Set the max frame size to 0x2600 by default, > > + * in case other drivers changed the default value. > > + */ > > + i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, > 0, > > NULL); > > + > > /* initialize RSS rule list */ > > TAILQ_INIT(&pf->rss_config_list); > > > > @@ -2328,7 +2332,6 @@ i40e_dev_start(struct rte_eth_dev *dev) > > uint32_t intr_vector = 0; > > struct i40e_vsi *vsi; > > uint16_t nb_rxq, nb_txq; > > - uint16_t max_frame_size; > > > > hw->adapter_stopped = 0; > > > > @@ -2467,9 +2470,6 @@ i40e_dev_start(struct rte_eth_dev *dev) > > "please call hierarchy_commit() " > > "before starting the port"); > > > > - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD; > > - i40e_set_mac_max_frame(dev, max_frame_size); > > - > > return I40E_SUCCESS; > > > > tx_err: > > @@ -2809,9 +2809,6 @@ i40e_dev_set_link_down(struct rte_eth_dev > *dev) > > return i40e_phy_conf_link(hw, abilities, speed, false); } > > > > -#define CHECK_INTERVAL 100 /* 100ms */ > > -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > - > > static __rte_always_inline void > > update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link) { @@ > > -2878,6 +2875,8 @@ static __rte_always_inline void > > update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link, > > bool enable_lse, int wait_to_complete) { > > +#define CHECK_INTERVAL 100 /* 100ms */ > > +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */ > > uint32_t rep_cnt = MAX_REPEAT_TIME; > > struct i40e_link_status link_status; > > int status; > > @@ -6738,7 +6737,6 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev > *dev) > > if (!ret) > > rte_eth_dev_callback_process(dev, > > RTE_ETH_EVENT_INTR_LSC, NULL); > > - > > break; > > default: > > PMD_DRV_LOG(DEBUG, "Request %u is not > supported yet", @@ > > -12123,40 +12121,6 @@ i40e_cloud_filter_qinq_create(struct i40e_pf > *pf) > > return ret; > > } > > > > -static void > > -i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size) -{ > > - struct i40e_hw *hw = > > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > - uint32_t rep_cnt = MAX_REPEAT_TIME; > > - struct rte_eth_link link; > > - enum i40e_status_code status; > > - bool can_be_set = true; > > - > > - /* > > - * I40E_MEDIA_TYPE_BASET link up can be ignored > > - * I40E_MEDIA_TYPE_BASET link down that hw->phy.media_type > > - * is I40E_MEDIA_TYPE_UNKNOWN > > - */ > > - if (hw->phy.media_type != I40E_MEDIA_TYPE_BASET && > > - hw->phy.media_type != I40E_MEDIA_TYPE_UNKNOWN) { > > - do { > > - update_link_reg(hw, &link); > > - if (link.link_status) > > - break; > > - rte_delay_ms(CHECK_INTERVAL); > > - } while (--rep_cnt); > > - can_be_set = !!link.link_status; > > - } > > - > > - if (can_be_set) { > > - status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, > NULL); > > - if (status != I40E_SUCCESS) > > - PMD_DRV_LOG(ERR, "Failed to set max frame size at > port > > level"); > > - } else { > > - PMD_DRV_LOG(ERR, "Set max frame size at port level not > applicable > > on link down"); > > - } > > -} > > - > > RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE); > > RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE); #ifdef > > RTE_ETHDEV_DEBUG_RX > > -- > > 2.38.1 > > Acked-by: Simei Su <simei.su@intel.com> Applied to dpdk-next-net-intel. Thanks Qi > > Thanks, > Simei