> -----Original Message-----
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Monday, April 2, 2018 10:34 PM
> To: Chas Williams <3ch...@gmail.com>
> Cc: Zhang, Helin <helin.zh...@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo...@intel.com>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; Charles (Chas) Williams <ch...@att.com>
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: update data->eth_link status on
> start
>
>
>
> > -----Original Message-----
> > From: Chas Williams [mailto:3ch...@gmail.com]
> > Sent: Monday, April 2, 2018 9:57 PM
> > To: Zhang, Qi Z <qi.z.zh...@intel.com>
> > Cc: Zhang, Helin <helin.zh...@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > <wenzhuo...@intel.com>; Ananyev, Konstantin
> > <konstantin.anan...@intel.com>; Charles (Chas) Williams
> > <ch...@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.zh...@intel.com>
> wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Chas Williams [mailto:3ch...@gmail.com]
> > >> Sent: Monday, April 2, 2018 9:38 PM
> > >> To: Zhang, Qi Z <qi.z.zh...@intel.com>
> > >> Cc: Zhang, Helin <helin.zh...@intel.com>; dev@dpdk.org; Lu,
> Wenzhuo
> > >> <wenzhuo...@intel.com>; Ananyev, Konstantin
> > >> <konstantin.anan...@intel.com>; Charles (Chas) Williams
> > >> <ch...@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.zh...@intel.com>
> > wrote:
> > >> > Hi Williams:
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Chas
> > >> >> Williams
> > >> >> Sent: Saturday, March 31, 2018 1:22 AM
> > >> >> To: Zhang, Helin <helin.zh...@intel.com>
> > >> >> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo...@intel.com>;
> Ananyev,
> > >> >> Konstantin <konstantin.anan...@intel.com>; Charles (Chas)
> > >> >> Williams <ch...@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.zh...@intel.com>
> > >> >> wrote:
> > >> >> >
> > >> >> >
> > >> >> >> -----Original Message-----
> > >> >> >> From: dev [mailto:dev-boun...@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" <ch...@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 <ch...@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
> > >> >> >
