[dpdk-dev] net/ixgbe: update data->eth_link status on start
Checks
Commit Message
From: "Charles (Chas) Williams" <chas3@att.com>
dev->data->eth_link isn't updated until the first interrupt. If a
link is carrier down, then this interrupt may never happen. Before we
finished starting the PMD, call ixgbe_dev_link_update() to ensure we
have a valid status.
Signed-off-by: Chas Williams <chas3@att.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
1 file changed, 4 insertions(+)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> Sent: Wednesday, February 14, 2018 6:56 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start
>
> From: "Charles (Chas) Williams" <chas3@att.com>
>
> dev->data->eth_link isn't updated until the first interrupt. If a
> link is carrier down, then this interrupt may never happen. Before we finished
> starting the PMD, call ixgbe_dev_link_update() to ensure we have a valid status.
>
> Signed-off-by: Chas Williams <chas3@att.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 37eb668..27d29dc 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> if (err)
> goto error;
>
> + ixgbe_dev_link_update(dev, 0);
It is called in rte_eth_dev_start() if lsc is not enabled,
and it is not needed here to avoid any duplication.
BTW, did you see any issue or just check the code? Thanks!
/Helin
> +
> skip_link_setup:
>
> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8 @@
> ixgbevf_dev_start(struct rte_eth_dev *dev)
>
> ixgbevf_dev_rxtx_start(dev);
>
> + ixgbevf_dev_link_update(dev, 0);
> +
> /* check and configure queue intr-vector mapping */
> if (rte_intr_cap_multiple(intr_handle) &&
> dev->data->dev_conf.intr_conf.rxq) {
> --
> 2.9.5
On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin <helin.zhang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> Sent: Wednesday, February 14, 2018 6:56 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start
>>
>> From: "Charles (Chas) Williams" <chas3@att.com>
>>
>> dev->data->eth_link isn't updated until the first interrupt. If a
>> link is carrier down, then this interrupt may never happen. Before we finished
>> starting the PMD, call ixgbe_dev_link_update() to ensure we have a valid status.
>>
>> Signed-off-by: Chas Williams <chas3@att.com>
>> ---
>> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 37eb668..27d29dc 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>> if (err)
>> goto error;
>>
>> + ixgbe_dev_link_update(dev, 0);
> It is called in rte_eth_dev_start() if lsc is not enabled,
> and it is not needed here to avoid any duplication.
> BTW, did you see any issue or just check the code? Thanks!
Yes, I see an issue with bonding. If LSC is enabled, then link_status
isn't set until the first interrupt to update the link status. If a
link is down, this interrupt may never happen result in link_status
being somewhat undefined.
>
> /Helin
>
>> +
>> skip_link_setup:
>>
>> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8 @@
>> ixgbevf_dev_start(struct rte_eth_dev *dev)
>>
>> ixgbevf_dev_rxtx_start(dev);
>>
>> + ixgbevf_dev_link_update(dev, 0);
>> +
>> /* check and configure queue intr-vector mapping */
>> if (rte_intr_cap_multiple(intr_handle) &&
>> dev->data->dev_conf.intr_conf.rxq) {
>> --
>> 2.9.5
>
Hi Williams:
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> Sent: Saturday, March 31, 2018 1:22 AM
> To: Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> <chas3@att.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin <helin.zhang@intel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> >> Sent: Wednesday, February 14, 2018 6:56 AM
> >> To: dev@dpdk.org
> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status
> >> on start
> >>
> >> From: "Charles (Chas) Williams" <chas3@att.com>
> >>
> >> dev->data->eth_link isn't updated until the first interrupt. If a
> >> link is carrier down, then this interrupt may never happen. Before
> >> we finished starting the PMD, call ixgbe_dev_link_update() to ensure we
> have a valid status.
> >>
> >> Signed-off-by: Chas Williams <chas3@att.com>
> >> ---
> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 37eb668..27d29dc 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >> if (err)
> >> goto error;
> >>
> >> + ixgbe_dev_link_update(dev, 0);
> > It is called in rte_eth_dev_start() if lsc is not enabled, and it is
> > not needed here to avoid any duplication.
> > BTW, did you see any issue or just check the code? Thanks!
>
> Yes, I see an issue with bonding. If LSC is enabled, then link_status isn't set
> until the first interrupt to update the link status. If a link is down, this
> interrupt may never happen result in link_status being somewhat undefined.
Is your issue only happened on VF?
For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so link status is assume be updated.
If you think it is just missed in VF, can you implemented this in the similar way as PF?
Regards
Qi
>
> >
> > /Helin
> >
> >> +
> >> skip_link_setup:
> >>
> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8 @@
> >> ixgbevf_dev_start(struct rte_eth_dev *dev)
> >>
> >> ixgbevf_dev_rxtx_start(dev);
> >>
> >> + ixgbevf_dev_link_update(dev, 0);
> >> +
> >> /* check and configure queue intr-vector mapping */
> >> if (rte_intr_cap_multiple(intr_handle) &&
> >> dev->data->dev_conf.intr_conf.rxq) {
> >> --
> >> 2.9.5
> >
On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> Hi Williams:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> Sent: Saturday, March 31, 2018 1:22 AM
>> To: Zhang, Helin <helin.zhang@intel.com>
>> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
>> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
>> <chas3@att.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
>> start
>>
>> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin <helin.zhang@intel.com>
>> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> >> Sent: Wednesday, February 14, 2018 6:56 AM
>> >> To: dev@dpdk.org
>> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
>> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status
>> >> on start
>> >>
>> >> From: "Charles (Chas) Williams" <chas3@att.com>
>> >>
>> >> dev->data->eth_link isn't updated until the first interrupt. If a
>> >> link is carrier down, then this interrupt may never happen. Before
>> >> we finished starting the PMD, call ixgbe_dev_link_update() to ensure we
>> have a valid status.
>> >>
>> >> Signed-off-by: Chas Williams <chas3@att.com>
>> >> ---
>> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
>> >> 1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> index 37eb668..27d29dc 100644
>> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>> >> if (err)
>> >> goto error;
>> >>
>> >> + ixgbe_dev_link_update(dev, 0);
>> > It is called in rte_eth_dev_start() if lsc is not enabled, and it is
>> > not needed here to avoid any duplication.
>> > BTW, did you see any issue or just check the code? Thanks!
>>
>> Yes, I see an issue with bonding. If LSC is enabled, then link_status isn't set
>> until the first interrupt to update the link status. If a link is down, this
>> interrupt may never happen result in link_status being somewhat undefined.
>
> Is your issue only happened on VF?
> For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so link status is assume be updated.
> If you think it is just missed in VF, can you implemented this in the similar way as PF?
No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't
update (atomically write)
the dev->data->dev_link. After .dev_start() finishes, I need the
link_status of the adapter to
be set. I can't wait until I hope the first interrupt has happened
that would update
dev->data->dev_link. How long would I wait? I don't really care if
the link is down,
or up, or whatever, but it can't be partially filled in. Bonding (in
802.3ad) wants the
links to be similarly configured.
>
> Regards
> Qi
>
>>
>> >
>> > /Helin
>> >
>> >> +
>> >> skip_link_setup:
>> >>
>> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8 @@
>> >> ixgbevf_dev_start(struct rte_eth_dev *dev)
>> >>
>> >> ixgbevf_dev_rxtx_start(dev);
>> >>
>> >> + ixgbevf_dev_link_update(dev, 0);
>> >> +
>> >> /* check and configure queue intr-vector mapping */
>> >> if (rte_intr_cap_multiple(intr_handle) &&
>> >> dev->data->dev_conf.intr_conf.rxq) {
>> >> --
>> >> 2.9.5
>> >
> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Monday, April 2, 2018 9:38 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Charles (Chas) Williams <chas3@att.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > Hi Williams:
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> >> Sent: Saturday, March 31, 2018 1:22 AM
> >> To: Zhang, Helin <helin.zhang@intel.com>
> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> >> <chas3@att.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> status on start
> >>
> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
> >> <helin.zhang@intel.com>
> >> wrote:
> >> >
> >> >
> >> >> -----Original Message-----
> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
> >> >> To: dev@dpdk.org
> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> >> status on start
> >> >>
> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
> >> >>
> >> >> dev->data->eth_link isn't updated until the first interrupt. If a
> >> >> link is carrier down, then this interrupt may never happen.
> >> >> Before we finished starting the PMD, call ixgbe_dev_link_update()
> >> >> to ensure we
> >> have a valid status.
> >> >>
> >> >> Signed-off-by: Chas Williams <chas3@att.com>
> >> >> ---
> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> >> >> 1 file changed, 4 insertions(+)
> >> >>
> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> index 37eb668..27d29dc 100644
> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >> >> if (err)
> >> >> goto error;
> >> >>
> >> >> + ixgbe_dev_link_update(dev, 0);
> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and it
> >> > is not needed here to avoid any duplication.
> >> > BTW, did you see any issue or just check the code? Thanks!
> >>
> >> Yes, I see an issue with bonding. If LSC is enabled, then
> >> link_status isn't set until the first interrupt to update the link
> >> status. If a link is down, this interrupt may never happen result in
> link_status being somewhat undefined.
> >
> > Is your issue only happened on VF?
> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so link status is
> assume be updated.
> > If you think it is just missed in VF, can you implemented this in the similar way
> as PF?
>
> No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't update
> (atomically write) the dev->data->dev_link. After .dev_start() finishes, I need
> the link_status of the adapter to be set.
I saw dev_link.link_status does be updated.
err = ixgbe_check_link(hw, &speed, &link_up, 0);
if (err)
goto error;
dev->data->dev_link.link_status = link_up;
> I can't wait until I hope the first
> interrupt has happened that would update
> dev->data->dev_link. How long would I wait? I don't really care if
> the link is down,
> or up, or whatever, but it can't be partially filled in. Bonding (in
> 802.3ad) wants the
> links to be similarly configured.
>
> >
> > Regards
> > Qi
> >
> >>
> >> >
> >> > /Helin
> >> >
> >> >> +
> >> >> skip_link_setup:
> >> >>
> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8
> >> >> @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> >> >>
> >> >> ixgbevf_dev_rxtx_start(dev);
> >> >>
> >> >> + ixgbevf_dev_link_update(dev, 0);
> >> >> +
> >> >> /* check and configure queue intr-vector mapping */
> >> >> if (rte_intr_cap_multiple(intr_handle) &&
> >> >> dev->data->dev_conf.intr_conf.rxq) {
> >> >> --
> >> >> 2.9.5
> >> >
On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Chas Williams [mailto:3chas3@gmail.com]
>> Sent: Monday, April 2, 2018 9:38 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; Charles (Chas) Williams <chas3@att.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
>> start
>>
>> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>> > Hi Williams:
>> >
>> >> -----Original Message-----
>> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> >> Sent: Saturday, March 31, 2018 1:22 AM
>> >> To: Zhang, Helin <helin.zhang@intel.com>
>> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
>> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
>> >> <chas3@att.com>
>> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
>> >> status on start
>> >>
>> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
>> >> <helin.zhang@intel.com>
>> >> wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
>> >> >> To: dev@dpdk.org
>> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
>> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
>> >> >> status on start
>> >> >>
>> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
>> >> >>
>> >> >> dev->data->eth_link isn't updated until the first interrupt. If a
>> >> >> link is carrier down, then this interrupt may never happen.
>> >> >> Before we finished starting the PMD, call ixgbe_dev_link_update()
>> >> >> to ensure we
>> >> have a valid status.
>> >> >>
>> >> >> Signed-off-by: Chas Williams <chas3@att.com>
>> >> >> ---
>> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
>> >> >> 1 file changed, 4 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> index 37eb668..27d29dc 100644
>> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>> >> >> if (err)
>> >> >> goto error;
>> >> >>
>> >> >> + ixgbe_dev_link_update(dev, 0);
>> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and it
>> >> > is not needed here to avoid any duplication.
>> >> > BTW, did you see any issue or just check the code? Thanks!
>> >>
>> >> Yes, I see an issue with bonding. If LSC is enabled, then
>> >> link_status isn't set until the first interrupt to update the link
>> >> status. If a link is down, this interrupt may never happen result in
>> link_status being somewhat undefined.
>> >
>> > Is your issue only happened on VF?
>> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so link status is
>> assume be updated.
>> > If you think it is just missed in VF, can you implemented this in the similar way
>> as PF?
>>
>> No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't update
>> (atomically write) the dev->data->dev_link. After .dev_start() finishes, I need
>> the link_status of the adapter to be set.
>
> I saw dev_link.link_status does be updated.
> err = ixgbe_check_link(hw, &speed, &link_up, 0);
> if (err)
> goto error;
> dev->data->dev_link.link_status = link_up;
That doesn't fill in link_duplex, link_speed, or link_autoneg.
802.3ad requires that all the ports of
the same bonding group have the same speed and duplex. I need to be
able to check the speed
and the duplex at least (and autoneg as well).
>
>> I can't wait until I hope the first
>> interrupt has happened that would update
>> dev->data->dev_link. How long would I wait? I don't really care if
>> the link is down,
>> or up, or whatever, but it can't be partially filled in. Bonding (in
>> 802.3ad) wants the
>> links to be similarly configured.
>>
>> >
>> > Regards
>> > Qi
>> >
>> >>
>> >> >
>> >> > /Helin
>> >> >
>> >> >> +
>> >> >> skip_link_setup:
>> >> >>
>> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8
>> >> >> @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
>> >> >>
>> >> >> ixgbevf_dev_rxtx_start(dev);
>> >> >>
>> >> >> + ixgbevf_dev_link_update(dev, 0);
>> >> >> +
>> >> >> /* check and configure queue intr-vector mapping */
>> >> >> if (rte_intr_cap_multiple(intr_handle) &&
>> >> >> dev->data->dev_conf.intr_conf.rxq) {
>> >> >> --
>> >> >> 2.9.5
>> >> >
> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Monday, April 2, 2018 9:57 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Charles (Chas) Williams <chas3@att.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
> On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Chas Williams [mailto:3chas3@gmail.com]
> >> Sent: Monday, April 2, 2018 9:38 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> >> <chas3@att.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> status on start
> >>
> >> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> >> > Hi Williams:
> >> >
> >> >> -----Original Message-----
> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> >> >> Sent: Saturday, March 31, 2018 1:22 AM
> >> >> To: Zhang, Helin <helin.zhang@intel.com>
> >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> >> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> >> >> <chas3@att.com>
> >> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> >> status on start
> >> >>
> >> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
> >> >> <helin.zhang@intel.com>
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> >> >> >> Williams
> >> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
> >> >> >> To: dev@dpdk.org
> >> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> >> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> >> >> status on start
> >> >> >>
> >> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
> >> >> >>
> >> >> >> dev->data->eth_link isn't updated until the first interrupt.
> >> >> >> dev->data->If a
> >> >> >> link is carrier down, then this interrupt may never happen.
> >> >> >> Before we finished starting the PMD, call
> >> >> >> ixgbe_dev_link_update() to ensure we
> >> >> have a valid status.
> >> >> >>
> >> >> >> Signed-off-by: Chas Williams <chas3@att.com>
> >> >> >> ---
> >> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> >> >> >> 1 file changed, 4 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> index 37eb668..27d29dc 100644
> >> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >> >> >> if (err)
> >> >> >> goto error;
> >> >> >>
> >> >> >> + ixgbe_dev_link_update(dev, 0);
> >> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and
> >> >> > it is not needed here to avoid any duplication.
> >> >> > BTW, did you see any issue or just check the code? Thanks!
> >> >>
> >> >> Yes, I see an issue with bonding. If LSC is enabled, then
> >> >> link_status isn't set until the first interrupt to update the link
> >> >> status. If a link is down, this interrupt may never happen result
> >> >> in
> >> link_status being somewhat undefined.
> >> >
> >> > Is your issue only happened on VF?
> >> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so
> >> > link status is
> >> assume be updated.
> >> > If you think it is just missed in VF, can you implemented this in
> >> > the similar way
> >> as PF?
> >>
> >> No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't
> >> update (atomically write) the dev->data->dev_link. After
> >> .dev_start() finishes, I need the link_status of the adapter to be set.
> >
> > I saw dev_link.link_status does be updated.
> > err = ixgbe_check_link(hw, &speed, &link_up, 0);
> > if (err)
> > goto error;
> > dev->data->dev_link.link_status = link_up;
>
> That doesn't fill in link_duplex, link_speed, or link_autoneg.
> 802.3ad requires that all the ports of
> the same bonding group have the same speed and duplex. I need to be able
> to check the speed and the duplex at least (and autoneg as well).
OK, I'm not sure if it is better to call this at rte_eth_dev_start
So far we have below code.
if (dev->data->dev_conf.intr_conf.lsc == 0) {
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
(*dev->dev_ops->link_update)(dev, 0);
}
Can we just remove the lsc check.
I'd like to see other people's comments.
>
> >
> >> I can't wait until I hope the first
> >> interrupt has happened that would update
> >> dev->data->dev_link. How long would I wait? I don't really care if
> >> the link is down,
> >> or up, or whatever, but it can't be partially filled in. Bonding (in
> >> 802.3ad) wants the
> >> links to be similarly configured.
> >>
> >> >
> >> > Regards
> >> > Qi
> >> >
> >> >>
> >> >> >
> >> >> > /Helin
> >> >> >
> >> >> >> +
> >> >> >> skip_link_setup:
> >> >> >>
> >> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6
> >> >> >> +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> >> >> >>
> >> >> >> ixgbevf_dev_rxtx_start(dev);
> >> >> >>
> >> >> >> + ixgbevf_dev_link_update(dev, 0);
> >> >> >> +
> >> >> >> /* check and configure queue intr-vector mapping */
> >> >> >> if (rte_intr_cap_multiple(intr_handle) &&
> >> >> >> dev->data->dev_conf.intr_conf.rxq) {
> >> >> >> --
> >> >> >> 2.9.5
> >> >> >
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Monday, April 2, 2018 10:34 PM
> To: Chas Williams <3chas3@gmail.com>
> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Charles (Chas) Williams <chas3@att.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
>
>
> > -----Original Message-----
> > From: Chas Williams [mailto:3chas3@gmail.com]
> > Sent: Monday, April 2, 2018 9:57 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> > <chas3@att.com>
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > status on start
> >
> > On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Chas Williams [mailto:3chas3@gmail.com]
> > >> Sent: Monday, April 2, 2018 9:38 PM
> > >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > >> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu,
> Wenzhuo
> > >> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > >> <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> > >> <chas3@att.com>
> > >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> status on start
> > >>
> > >> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com>
> > wrote:
> > >> > Hi Williams:
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> > >> >> Williams
> > >> >> Sent: Saturday, March 31, 2018 1:22 AM
> > >> >> To: Zhang, Helin <helin.zhang@intel.com>
> > >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> Ananyev,
> > >> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas)
> > >> >> Williams <chas3@att.com>
> > >> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> >> status on start
> > >> >>
> > >> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
> > >> >> <helin.zhang@intel.com>
> > >> >> wrote:
> > >> >> >
> > >> >> >
> > >> >> >> -----Original Message-----
> > >> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> > >> >> >> Williams
> > >> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
> > >> >> >> To: dev@dpdk.org
> > >> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> > >> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> >> >> status on start
> > >> >> >>
> > >> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
> > >> >> >>
> > >> >> >> dev->data->eth_link isn't updated until the first interrupt.
> > >> >> >> dev->data->If a
> > >> >> >> link is carrier down, then this interrupt may never happen.
> > >> >> >> Before we finished starting the PMD, call
> > >> >> >> ixgbe_dev_link_update() to ensure we
> > >> >> have a valid status.
> > >> >> >>
> > >> >> >> Signed-off-by: Chas Williams <chas3@att.com>
> > >> >> >> ---
> > >> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> > >> >> >> 1 file changed, 4 insertions(+)
> > >> >> >>
> > >> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> index 37eb668..27d29dc 100644
> > >> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev
> *dev)
> > >> >> >> if (err)
> > >> >> >> goto error;
> > >> >> >>
> > >> >> >> + ixgbe_dev_link_update(dev, 0);
> > >> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and
> > >> >> > it is not needed here to avoid any duplication.
> > >> >> > BTW, did you see any issue or just check the code? Thanks!
> > >> >>
> > >> >> Yes, I see an issue with bonding. If LSC is enabled, then
> > >> >> link_status isn't set until the first interrupt to update the
> > >> >> link status. If a link is down, this interrupt may never happen
> > >> >> result in
> > >> link_status being somewhat undefined.
> > >> >
> > >> > Is your issue only happened on VF?
> > >> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so
> > >> > link status is
> > >> assume be updated.
> > >> > If you think it is just missed in VF, can you implemented this in
> > >> > the similar way
> > >> as PF?
> > >>
> > >> No, I don't believe it's isolated to VF. ixgbe_check_link()
> > >> doesn't update (atomically write) the dev->data->dev_link. After
> > >> .dev_start() finishes, I need the link_status of the adapter to be set.
> > >
> > > I saw dev_link.link_status does be updated.
> > > err = ixgbe_check_link(hw, &speed, &link_up, 0);
> > > if (err)
> > > goto error;
> > > dev->data->dev_link.link_status = link_up;
> >
> > That doesn't fill in link_duplex, link_speed, or link_autoneg.
> > 802.3ad requires that all the ports of the same bonding group have the
> > same speed and duplex. I need to be able to check the speed and the
> > duplex at least (and autoneg as well).
>
> OK, I'm not sure if it is better to call this at rte_eth_dev_start So far we have
> below code.
> if (dev->data->dev_conf.intr_conf.lsc == 0) {
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update,
> -ENOTSUP);
> (*dev->dev_ops->link_update)(dev, 0);
> }
> Can we just remove the lsc check.
> I'd like to see other people's comments.
>
I have tested both 82599 PF and VF.
I add a breakpoint at the code line added by this patch.
Before the added line, all members of the dev->data->dev_link are zeros in both PF and VF.
After the added line, the dev_link is updated.
Indeed, I agree Zhang Qi's suggestion.
If the lsc check in rte_eth_dev_start is removed, it can benefit all PMDs.
Of course, Qi's suggestion also address your issue.
> >
> > >
> > >> I can't wait until I hope the first interrupt has happened that
> > >> would update
> > >> dev->data->dev_link. How long would I wait? I don't really care
> > >> dev->data->if
> > >> the link is down,
> > >> or up, or whatever, but it can't be partially filled in. Bonding
> > >> (in
> > >> 802.3ad) wants the
> > >> links to be similarly configured.
> > >>
> > >> >
> > >> > Regards
> > >> > Qi
> > >> >
> > >> >>
> > >> >> >
> > >> >> > /Helin
> > >> >> >
> > >> >> >> +
> > >> >> >> skip_link_setup:
> > >> >> >>
> > >> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6
> > >> >> >> +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> > >> >> >>
> > >> >> >> ixgbevf_dev_rxtx_start(dev);
> > >> >> >>
> > >> >> >> + ixgbevf_dev_link_update(dev, 0);
> > >> >> >> +
> > >> >> >> /* check and configure queue intr-vector mapping */
> > >> >> >> if (rte_intr_cap_multiple(intr_handle) &&
> > >> >> >> dev->data->dev_conf.intr_conf.rxq) {
> > >> >> >> --
> > >> >> >> 2.9.5
> > >> >> >
> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Monday, April 2, 2018 9:57 PM
> To: Zhang, Qi Z
> Cc: Zhang, Helin; dev@dpdk.org; Lu, Wenzhuo; Ananyev, Konstantin; Charles
> (Chas) Williams
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
> On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Chas Williams [mailto:3chas3@gmail.com]
> >> Sent: Monday, April 2, 2018 9:38 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> >> <chas3@att.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> status on start
> >>
> >> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >> > Hi Williams:
> >> >
> >> >> -----Original Message-----
> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> >> >> Sent: Saturday, March 31, 2018 1:22 AM
> >> >> To: Zhang, Helin <helin.zhang@intel.com>
> >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> >> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> >> >> <chas3@att.com>
> >> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> >> status on start
> >> >>
> >> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
> >> >> <helin.zhang@intel.com>
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> >> >> >> Williams
> >> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
> >> >> >> To: dev@dpdk.org
> >> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> >> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> >> >> >> status on start
> >> >> >>
> >> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
> >> >> >>
> >> >> >> dev->data->eth_link isn't updated until the first interrupt.
> >> >> >> dev->data->If a
> >> >> >> link is carrier down, then this interrupt may never happen.
> >> >> >> Before we finished starting the PMD, call
> >> >> >> ixgbe_dev_link_update() to ensure we
> >> >> have a valid status.
> >> >> >>
> >> >> >> Signed-off-by: Chas Williams <chas3@att.com>
> >> >> >> ---
> >> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> >> >> >> 1 file changed, 4 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> index 37eb668..27d29dc 100644
> >> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >> >> >> if (err)
> >> >> >> goto error;
> >> >> >>
> >> >> >> + ixgbe_dev_link_update(dev, 0);
> >> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and
> >> >> > it is not needed here to avoid any duplication.
> >> >> > BTW, did you see any issue or just check the code? Thanks!
> >> >>
> >> >> Yes, I see an issue with bonding. If LSC is enabled, then
> >> >> link_status isn't set until the first interrupt to update the link
> >> >> status. If a link is down, this interrupt may never happen result
> >> >> in
> >> link_status being somewhat undefined.
> >> >
> >> > Is your issue only happened on VF?
> >> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so
> >> > link status is
> >> assume be updated.
> >> > If you think it is just missed in VF, can you implemented this in
> >> > the similar way
> >> as PF?
> >>
> >> No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't
> >> update (atomically write) the dev->data->dev_link. After
> >> .dev_start() finishes, I need the link_status of the adapter to be set.
> >
> > I saw dev_link.link_status does be updated.
> > err = ixgbe_check_link(hw, &speed, &link_up, 0);
> > if (err)
> > goto error;
> > dev->data->dev_link.link_status = link_up;
>
> That doesn't fill in link_duplex, link_speed, or link_autoneg.
> 802.3ad requires that all the ports of
> the same bonding group have the same speed and duplex. I need to be able to
> check the speed and the duplex at least (and autoneg as well).
I am thinking it might be a bit late to update the link status in dev_start(), and we
need to check if the full link update should be done in eth_ixgbe_dev_init() and
eth_ixgbevf_dev_init().
@Qi, Chas, could you help to check if my idea is correct?
Also I think this applies to other PMDs, e.g. igb, i40e. At least we need to double check.
Regards,
Helin
>
> >
> >> I can't wait until I hope the first
> >> interrupt has happened that would update
> >> dev->data->dev_link. How long would I wait? I don't really care if
> >> the link is down,
> >> or up, or whatever, but it can't be partially filled in. Bonding (in
> >> 802.3ad) wants the
> >> links to be similarly configured.
> >>
> >> >
> >> > Regards
> >> > Qi
> >> >
> >> >>
> >> >> >
> >> >> > /Helin
> >> >> >
> >> >> >> +
> >> >> >> skip_link_setup:
> >> >> >>
> >> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6
> >> >> >> +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> >> >> >>
> >> >> >> ixgbevf_dev_rxtx_start(dev);
> >> >> >>
> >> >> >> + ixgbevf_dev_link_update(dev, 0);
> >> >> >> +
> >> >> >> /* check and configure queue intr-vector mapping */
> >> >> >> if (rte_intr_cap_multiple(intr_handle) &&
> >> >> >> dev->data->dev_conf.intr_conf.rxq) {
> >> >> >> --
> >> >> >> 2.9.5
> >> >> >
On Fri, Apr 6, 2018 at 10:52 AM, Zhang, Helin <helin.zhang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Chas Williams [mailto:3chas3@gmail.com]
>> Sent: Monday, April 2, 2018 9:57 PM
>> To: Zhang, Qi Z
>> Cc: Zhang, Helin; dev@dpdk.org; Lu, Wenzhuo; Ananyev, Konstantin; Charles
>> (Chas) Williams
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
>> start
>>
>> On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Chas Williams [mailto:3chas3@gmail.com]
>> >> Sent: Monday, April 2, 2018 9:38 PM
>> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> >> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
>> >> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
>> >> <konstantin.ananyev@intel.com>; Charles (Chas) Williams
>> >> <chas3@att.com>
>> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
>> >> status on start
>> >>
>> >> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>> >> > Hi Williams:
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> >> >> Sent: Saturday, March 31, 2018 1:22 AM
>> >> >> To: Zhang, Helin <helin.zhang@intel.com>
>> >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
>> >> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
>> >> >> <chas3@att.com>
>> >> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
>> >> >> status on start
>> >> >>
>> >> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
>> >> >> <helin.zhang@intel.com>
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
>> >> >> >> Williams
>> >> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
>> >> >> >> To: dev@dpdk.org
>> >> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
>> >> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
>> >> >> >> status on start
>> >> >> >>
>> >> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
>> >> >> >>
>> >> >> >> dev->data->eth_link isn't updated until the first interrupt.
>> >> >> >> dev->data->If a
>> >> >> >> link is carrier down, then this interrupt may never happen.
>> >> >> >> Before we finished starting the PMD, call
>> >> >> >> ixgbe_dev_link_update() to ensure we
>> >> >> have a valid status.
>> >> >> >>
>> >> >> >> Signed-off-by: Chas Williams <chas3@att.com>
>> >> >> >> ---
>> >> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
>> >> >> >> 1 file changed, 4 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> >> index 37eb668..27d29dc 100644
>> >> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> >> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>> >> >> >> if (err)
>> >> >> >> goto error;
>> >> >> >>
>> >> >> >> + ixgbe_dev_link_update(dev, 0);
>> >> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and
>> >> >> > it is not needed here to avoid any duplication.
>> >> >> > BTW, did you see any issue or just check the code? Thanks!
>> >> >>
>> >> >> Yes, I see an issue with bonding. If LSC is enabled, then
>> >> >> link_status isn't set until the first interrupt to update the link
>> >> >> status. If a link is down, this interrupt may never happen result
>> >> >> in
>> >> link_status being somewhat undefined.
>> >> >
>> >> > Is your issue only happened on VF?
>> >> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so
>> >> > link status is
>> >> assume be updated.
>> >> > If you think it is just missed in VF, can you implemented this in
>> >> > the similar way
>> >> as PF?
>> >>
>> >> No, I don't believe it's isolated to VF. ixgbe_check_link() doesn't
>> >> update (atomically write) the dev->data->dev_link. After
>> >> .dev_start() finishes, I need the link_status of the adapter to be set.
>> >
>> > I saw dev_link.link_status does be updated.
>> > err = ixgbe_check_link(hw, &speed, &link_up, 0);
>> > if (err)
>> > goto error;
>> > dev->data->dev_link.link_status = link_up;
>>
>> That doesn't fill in link_duplex, link_speed, or link_autoneg.
>> 802.3ad requires that all the ports of
>> the same bonding group have the same speed and duplex. I need to be able to
>> check the speed and the duplex at least (and autoneg as well).
> I am thinking it might be a bit late to update the link status in dev_start(), and we
> need to check if the full link update should be done in eth_ixgbe_dev_init() and
> eth_ixgbevf_dev_init().
> @Qi, Chas, could you help to check if my idea is correct?
> Also I think this applies to other PMDs, e.g. igb, i40e. At least we need to double check.
I don't think link_status needs updated sooner unless you think there
is some path
in the PMDs that need to check the link status. I am not aware of anything like
that in the Intel PMDs (but certainly not an expert in their
behavior). Since the DPDK
API is inherently single-threaded there's no external reason to have
the link status
updated until just before .dev_start() finishes.
>
> Regards,
> Helin
>
>>
>> >
>> >> I can't wait until I hope the first
>> >> interrupt has happened that would update
>> >> dev->data->dev_link. How long would I wait? I don't really care if
>> >> the link is down,
>> >> or up, or whatever, but it can't be partially filled in. Bonding (in
>> >> 802.3ad) wants the
>> >> links to be similarly configured.
>> >>
>> >> >
>> >> > Regards
>> >> > Qi
>> >> >
>> >> >>
>> >> >> >
>> >> >> > /Helin
>> >> >> >
>> >> >> >> +
>> >> >> >> skip_link_setup:
>> >> >> >>
>> >> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6
>> >> >> >> +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
>> >> >> >>
>> >> >> >> ixgbevf_dev_rxtx_start(dev);
>> >> >> >>
>> >> >> >> + ixgbevf_dev_link_update(dev, 0);
>> >> >> >> +
>> >> >> >> /* check and configure queue intr-vector mapping */
>> >> >> >> if (rte_intr_cap_multiple(intr_handle) &&
>> >> >> >> dev->data->dev_conf.intr_conf.rxq) {
>> >> >> >> --
>> >> >> >> 2.9.5
>> >> >> >
Hi Helin:
> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, April 6, 2018 10:52 PM
> To: Chas Williams <3chas3@gmail.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> <chas3@att.com>
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
>
>
> > -----Original Message-----
> > From: Chas Williams [mailto:3chas3@gmail.com]
> > Sent: Monday, April 2, 2018 9:57 PM
> > To: Zhang, Qi Z
> > Cc: Zhang, Helin; dev@dpdk.org; Lu, Wenzhuo; Ananyev, Konstantin;
> > Charles
> > (Chas) Williams
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > status on start
> >
> > On Mon, Apr 2, 2018 at 9:45 AM, Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Chas Williams [mailto:3chas3@gmail.com]
> > >> Sent: Monday, April 2, 2018 9:38 PM
> > >> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > >> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > >> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > >> <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> > >> <chas3@att.com>
> > >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> status on start
> > >>
> > >> On Mon, Apr 2, 2018 at 8:40 AM, Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> > >> > Hi Williams:
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> > >> >> Williams
> > >> >> Sent: Saturday, March 31, 2018 1:22 AM
> > >> >> To: Zhang, Helin <helin.zhang@intel.com>
> > >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev,
> > >> >> Konstantin <konstantin.ananyev@intel.com>; Charles (Chas)
> > >> >> Williams <chas3@att.com>
> > >> >> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> >> status on start
> > >> >>
> > >> >> On Fri, Mar 30, 2018 at 12:30 PM, Zhang, Helin
> > >> >> <helin.zhang@intel.com>
> > >> >> wrote:
> > >> >> >
> > >> >> >
> > >> >> >> -----Original Message-----
> > >> >> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas
> > >> >> >> Williams
> > >> >> >> Sent: Wednesday, February 14, 2018 6:56 AM
> > >> >> >> To: dev@dpdk.org
> > >> >> >> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> > >> >> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link
> > >> >> >> status on start
> > >> >> >>
> > >> >> >> From: "Charles (Chas) Williams" <chas3@att.com>
> > >> >> >>
> > >> >> >> dev->data->eth_link isn't updated until the first interrupt.
> > >> >> >> dev->data->If a
> > >> >> >> link is carrier down, then this interrupt may never happen.
> > >> >> >> Before we finished starting the PMD, call
> > >> >> >> ixgbe_dev_link_update() to ensure we
> > >> >> have a valid status.
> > >> >> >>
> > >> >> >> Signed-off-by: Chas Williams <chas3@att.com>
> > >> >> >> ---
> > >> >> >> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> > >> >> >> 1 file changed, 4 insertions(+)
> > >> >> >>
> > >> >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> index 37eb668..27d29dc 100644
> > >> >> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> >> >> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > >> >> >> if (err)
> > >> >> >> goto error;
> > >> >> >>
> > >> >> >> + ixgbe_dev_link_update(dev, 0);
> > >> >> > It is called in rte_eth_dev_start() if lsc is not enabled, and
> > >> >> > it is not needed here to avoid any duplication.
> > >> >> > BTW, did you see any issue or just check the code? Thanks!
> > >> >>
> > >> >> Yes, I see an issue with bonding. If LSC is enabled, then
> > >> >> link_status isn't set until the first interrupt to update the
> > >> >> link status. If a link is down, this interrupt may never happen
> > >> >> result in
> > >> link_status being somewhat undefined.
> > >> >
> > >> > Is your issue only happened on VF?
> > >> > For PF, I saw ixgbe_check_link is called at ixgbe_dev_start, so
> > >> > link status is
> > >> assume be updated.
> > >> > If you think it is just missed in VF, can you implemented this in
> > >> > the similar way
> > >> as PF?
> > >>
> > >> No, I don't believe it's isolated to VF. ixgbe_check_link()
> > >> doesn't update (atomically write) the dev->data->dev_link. After
> > >> .dev_start() finishes, I need the link_status of the adapter to be set.
> > >
> > > I saw dev_link.link_status does be updated.
> > > err = ixgbe_check_link(hw, &speed, &link_up, 0);
> > > if (err)
> > > goto error;
> > > dev->data->dev_link.link_status = link_up;
> >
> > That doesn't fill in link_duplex, link_speed, or link_autoneg.
> > 802.3ad requires that all the ports of the same bonding group have the
> > same speed and duplex. I need to be able to check the speed and the
> > duplex at least (and autoneg as well).
> I am thinking it might be a bit late to update the link status in dev_start(), and
> we need to check if the full link update should be done in eth_ixgbe_dev_init()
> and eth_ixgbevf_dev_init().
> @Qi, Chas, could you help to check if my idea is correct?
> Also I think this applies to other PMDs, e.g. igb, i40e. At least we need to
> double check.
link_update should not be called at init, because link speed is not configured yet,
it is configured at rte_eth_dev_configure, and usually it is applied at dev_start.
Basically I agree with the idea of the patch, link status should be valid when device
is started even lsc =1, but not sure if it is a common issue or we can move to ether layer,
anyway this could be on a separate topic
so how about just ack this and make ixgbe work first, what do you think?
Regards
Qi
>
> Regards,
> Helin
>
> >
> > >
> > >> I can't wait until I hope the first interrupt has happened that
> > >> would update
> > >> dev->data->dev_link. How long would I wait? I don't really care
> > >> dev->data->if
> > >> the link is down,
> > >> or up, or whatever, but it can't be partially filled in. Bonding
> > >> (in
> > >> 802.3ad) wants the
> > >> links to be similarly configured.
> > >>
> > >> >
> > >> > Regards
> > >> > Qi
> > >> >
> > >> >>
> > >> >> >
> > >> >> > /Helin
> > >> >> >
> > >> >> >> +
> > >> >> >> skip_link_setup:
> > >> >> >>
> > >> >> >> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6
> > >> >> >> +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
> > >> >> >>
> > >> >> >> ixgbevf_dev_rxtx_start(dev);
> > >> >> >>
> > >> >> >> + ixgbevf_dev_link_update(dev, 0);
> > >> >> >> +
> > >> >> >> /* check and configure queue intr-vector mapping */
> > >> >> >> if (rte_intr_cap_multiple(intr_handle) &&
> > >> >> >> dev->data->dev_conf.intr_conf.rxq) {
> > >> >> >> --
> > >> >> >> 2.9.5
> > >> >> >
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> Sent: Wednesday, February 14, 2018 6:56 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Charles (Chas) Williams <chas3@att.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start
>
> From: "Charles (Chas) Williams" <chas3@att.com>
>
> dev->data->eth_link isn't updated until the first interrupt. If a
> link is carrier down, then this interrupt may never happen. Before we
> finished starting the PMD, call ixgbe_dev_link_update() to ensure we have a
> valid status.
>
> Signed-off-by: Chas Williams <chas3@att.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 37eb668..27d29dc 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> if (err)
> goto error;
>
> + ixgbe_dev_link_update(dev, 0);
> +
> skip_link_setup:
>
> if (rte_intr_allow_others(intr_handle)) { @@ -5033,6 +5035,8 @@
> ixgbevf_dev_start(struct rte_eth_dev *dev)
>
> ixgbevf_dev_rxtx_start(dev);
>
> + ixgbevf_dev_link_update(dev, 0);
> +
> /* check and configure queue intr-vector mapping */
> if (rte_intr_cap_multiple(intr_handle) &&
> dev->data->dev_conf.intr_conf.rxq) {
> --
> 2.9.5
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Sunday, April 8, 2018 10:05 AM
> To: Chas Williams; dev@dpdk.org
> Cc: Lu, Wenzhuo; Ananyev, Konstantin; Charles (Chas) Williams
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> > Sent: Wednesday, February 14, 2018 6:56 AM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Charles (Chas) Williams
> > <chas3@att.com>
> > Subject: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> > start
> >
> > From: "Charles (Chas) Williams" <chas3@att.com>
> >
> > dev->data->eth_link isn't updated until the first interrupt. If a
> > link is carrier down, then this interrupt may never happen. Before we
> > finished starting the PMD, call ixgbe_dev_link_update() to ensure we
> > have a valid status.
> >
> > Signed-off-by: Chas Williams <chas3@att.com>
>
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel, thanks!
/Helin
@@ -2666,6 +2666,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
if (err)
goto error;
+ ixgbe_dev_link_update(dev, 0);
+
skip_link_setup:
if (rte_intr_allow_others(intr_handle)) {
@@ -5033,6 +5035,8 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
ixgbevf_dev_rxtx_start(dev);
+ ixgbevf_dev_link_update(dev, 0);
+
/* check and configure queue intr-vector mapping */
if (rte_intr_cap_multiple(intr_handle) &&
dev->data->dev_conf.intr_conf.rxq) {