[v5] net/i40e: rework maximum frame size configuration
Checks
Commit Message
This patch reverts mentioned changes below to remove unnecessary link
status check and only moves max frame size configuration to dev_start.
Also, it sets the parameter "wait to complete" true to wait for complete
right after setting link up.
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: Simei Su <simei.su@intel.com>
---
v5:
* Fix misspelling in commit log.
v4:
* Refine commit log.
* Avoid duplicate call to set parameter "wait to complete" true.
v3:
* Put link update before interrupt enable.
v2:
* Refine commit log.
* Add link update.
drivers/net/i40e/i40e_ethdev.c | 54 ++++++++++--------------------------------
1 file changed, 13 insertions(+), 41 deletions(-)
Comments
On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
>
> This patch reverts mentioned changes below to remove unnecessary link
> status check and only moves max frame size configuration to dev_start.
> Also, it sets the parameter "wait to complete" true to wait for complete
> right after setting link up.
Why is the change on link status needed?
Is it necessary?
>
> 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: Simei Su <simei.su@intel.com>
I would have preferred you reply to my original report.
At least, I'd like you add some credit with my name in the commitlog.
For the record, the differences with my v1 are:
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 5635dd03cf..5d57bb9a0e 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2327,6 +2327,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;
@@ -2447,7 +2448,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
PMD_DRV_LOG(WARNING, "Fail to set phy mask");
/* Call get_link_info aq command to enable/disable LSE */
- i40e_dev_link_update(dev, 0);
+ i40e_dev_link_update(dev, 1);
}
if (dev->data->dev_conf.intr_conf.rxq == 0) {
@@ -2465,8 +2466,16 @@ i40e_dev_start(struct rte_eth_dev *dev)
"please call hierarchy_commit() "
"before starting the port");
- i40e_aq_set_mac_config(hw, dev->data->mtu + I40E_ETH_OVERHEAD, TRUE,
- false, 0, NULL);
+ max_frame_size = dev->data->mtu ?
+ dev->data->mtu + I40E_ETH_OVERHEAD :
+ I40E_FRAME_SIZE_MAX;
+
+ /* Set the max frame size to HW*/
+ ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0, NULL);
+ if (ret) {
+ PMD_DRV_LOG(ERR, "Fail to set mac config");
+ return ret;
+ }
return I40E_SUCCESS;
Qi, don't apply this fix yet.
I'll generate some binaries internally to have Red Hat QE run their tests.
Thanks.
On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> @@ -2467,8 +2466,16 @@ 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);
> + max_frame_size = dev->data->mtu ?
> + dev->data->mtu + I40E_ETH_OVERHEAD :
> + I40E_FRAME_SIZE_MAX;
> +
> + /* Set the max frame size to HW*/
> + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0, NULL);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to set mac config");
> + return ret;
> + }
Reading this patch again.
Returning here seems incorrect as we leave rx/tx queue in started state.
Don't we need to jump to tx_err label on error?
>
> return I40E_SUCCESS;
>
On Thu, Feb 2, 2023 at 2:24 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> > @@ -2467,8 +2466,16 @@ 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);
> > + max_frame_size = dev->data->mtu ?
> > + dev->data->mtu + I40E_ETH_OVERHEAD :
> > + I40E_FRAME_SIZE_MAX;
> > +
> > + /* Set the max frame size to HW*/
> > + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0, NULL);
> > + if (ret) {
> > + PMD_DRV_LOG(ERR, "Fail to set mac config");
> > + return ret;
> > + }
>
> Reading this patch again.
>
> Returning here seems incorrect as we leave rx/tx queue in started state.
> Don't we need to jump to tx_err label on error?
There is probably an issue with interrupt handler still being registered too.
Qi, i40e maintainers, please review this patch carefully, and ping me
when it is ready so that we can test it at RH.
Thanks.
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, February 2, 2023 8:56 PM
> To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; dev@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v5] net/i40e: rework maximum frame size configuration
>
> On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> >
> > This patch reverts mentioned changes below to remove unnecessary link
> > status check and only moves max frame size configuration to dev_start.
> > Also, it sets the parameter "wait to complete" true to wait for
> > complete right after setting link up.
>
> Why is the change on link status needed?
> Is it necessary?
Indeed, it doesn't change the link status, it only involves update, waiting for it to complete.
Sorry not to describe correctly about setting the " wait to complete" true.
>
> >
> > 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: Simei Su <simei.su@intel.com>
>
> I would have preferred you reply to my original report.
> At least, I'd like you add some credit with my name in the commitlog.
Sorry, I will notice it in next version.
Thanks,
Simei
>
>
> For the record, the differences with my v1 are:
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 5635dd03cf..5d57bb9a0e 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2327,6 +2327,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;
>
> @@ -2447,7 +2448,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
> PMD_DRV_LOG(WARNING, "Fail to set phy
> mask");
>
> /* Call get_link_info aq command to enable/disable LSE
> */
> - i40e_dev_link_update(dev, 0);
> + i40e_dev_link_update(dev, 1);
> }
>
> if (dev->data->dev_conf.intr_conf.rxq == 0) { @@ -2465,8 +2466,16
> @@ i40e_dev_start(struct rte_eth_dev *dev)
> "please call hierarchy_commit() "
> "before starting the port");
>
> - i40e_aq_set_mac_config(hw, dev->data->mtu +
> I40E_ETH_OVERHEAD, TRUE,
> - false, 0, NULL);
> + max_frame_size = dev->data->mtu ?
> + dev->data->mtu + I40E_ETH_OVERHEAD :
> + I40E_FRAME_SIZE_MAX;
> +
> + /* Set the max frame size to HW*/
> + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0,
> NULL);
> + if (ret) {
> + PMD_DRV_LOG(ERR, "Fail to set mac config");
> + return ret;
> + }
>
> return I40E_SUCCESS;
>
>
> Qi, don't apply this fix yet.
> I'll generate some binaries internally to have Red Hat QE run their tests.
>
>
> Thanks.
>
> --
> David Marchand
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, February 2, 2023 9:24 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; dev@dpdk.org; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v5] net/i40e: rework maximum frame size configuration
>
> On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> > @@ -2467,8 +2466,16 @@ 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);
> > + max_frame_size = dev->data->mtu ?
> > + dev->data->mtu + I40E_ETH_OVERHEAD :
> > + I40E_FRAME_SIZE_MAX;
> > +
> > + /* Set the max frame size to HW*/
> > + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0,
> NULL);
> > + if (ret) {
> > + PMD_DRV_LOG(ERR, "Fail to set mac config");
> > + return ret;
> > + }
>
> Reading this patch again.
>
> Returning here seems incorrect as we leave rx/tx queue in started state.
> Don't we need to jump to tx_err label on error?
Yes, it's my fault to return here incorrectly. I will modify it in next version.
Thanks,
Simei
>
> >
> > return I40E_SUCCESS;
> >
>
> --
> David Marchand
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, February 2, 2023 9:49 PM
> To: Su, Simei <simei.su@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v5] net/i40e: rework maximum frame size configuration
>
> On Thu, Feb 2, 2023 at 2:24 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> > > @@ -2467,8 +2466,16 @@ 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);
> > > + max_frame_size = dev->data->mtu ?
> > > + dev->data->mtu + I40E_ETH_OVERHEAD :
> > > + I40E_FRAME_SIZE_MAX;
> > > +
> > > + /* Set the max frame size to HW*/
> > > + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0,
> NULL);
> > > + if (ret) {
> > > + PMD_DRV_LOG(ERR, "Fail to set mac config");
> > > + return ret;
> > > + }
> >
> > Reading this patch again.
> >
> > Returning here seems incorrect as we leave rx/tx queue in started state.
> > Don't we need to jump to tx_err label on error?
>
> There is probably an issue with interrupt handler still being registered too.
> Qi, i40e maintainers, please review this patch carefully, and ping me when it
> is ready so that we can test it at RH.
>
This change will not break interrupt handler, the second parameter is waiting to complete. Just waiting more time to make sure adminq command execute completed. So that subsequent commands(MTU set) can be executed.
And if you have other issues report at RH system, please report it.
>
> Thanks.
> --
> David Marchand
On Fri, Feb 3, 2023 at 9:39 AM Yang, Qiming <qiming.yang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, February 2, 2023 9:49 PM
> > To: Su, Simei <simei.su@intel.com>; Zhang, Yuying
> > <yuying.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> > stable@dpdk.org
> > Subject: Re: [PATCH v5] net/i40e: rework maximum frame size configuration
> >
> > On Thu, Feb 2, 2023 at 2:24 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei.su@intel.com> wrote:
> > > > @@ -2467,8 +2466,16 @@ 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);
> > > > + max_frame_size = dev->data->mtu ?
> > > > + dev->data->mtu + I40E_ETH_OVERHEAD :
> > > > + I40E_FRAME_SIZE_MAX;
> > > > +
> > > > + /* Set the max frame size to HW*/
> > > > + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0,
> > NULL);
> > > > + if (ret) {
> > > > + PMD_DRV_LOG(ERR, "Fail to set mac config");
> > > > + return ret;
> > > > + }
> > >
> > > Reading this patch again.
> > >
> > > Returning here seems incorrect as we leave rx/tx queue in started state.
> > > Don't we need to jump to tx_err label on error?
> >
> > There is probably an issue with interrupt handler still being registered too.
> > Qi, i40e maintainers, please review this patch carefully, and ping me when it
> > is ready so that we can test it at RH.
> >
>
> This change will not break interrupt handler, the second parameter is waiting to complete. Just waiting more time to make sure adminq command execute completed. So that subsequent commands(MTU set) can be executed.
> And if you have other issues report at RH system, please report it.
If i40e_aq_set_mac_config() fails and we return directly, an interrupt
handler is left registered, don't you think it is an issue?
In any case, I will wait for the next revision before asking for tests
on my side.
Thanks.
@@ -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,
@@ -2449,7 +2448,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
PMD_DRV_LOG(WARNING, "Fail to set phy mask");
/* Call get_link_info aq command to enable/disable LSE */
- i40e_dev_link_update(dev, 0);
+ i40e_dev_link_update(dev, 1);
}
if (dev->data->dev_conf.intr_conf.rxq == 0) {
@@ -2467,8 +2466,16 @@ 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);
+ max_frame_size = dev->data->mtu ?
+ dev->data->mtu + I40E_ETH_OVERHEAD :
+ I40E_FRAME_SIZE_MAX;
+
+ /* Set the max frame size to HW*/
+ ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0, NULL);
+ if (ret) {
+ PMD_DRV_LOG(ERR, "Fail to set mac config");
+ return ret;
+ }
return I40E_SUCCESS;
@@ -2809,9 +2816,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 +2882,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;
@@ -12123,40 +12129,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