[dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Chas Williams]

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);
+
 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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Helin]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Chas Williams]

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
>

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Chas Williams]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Chas Williams]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Dai, Wei]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Helin]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Chas Williams]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Qi Z]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Zhang, Helin]

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

Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on start

[Riyadi Selamat]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: Type: text/html, Size: 23 bytes --]