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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

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