From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5FCC84380F; Thu, 4 Jan 2024 02:17:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2175E402BF; Thu, 4 Jan 2024 02:17:18 +0100 (CET) Received: from mail-oo1-f41.google.com (mail-oo1-f41.google.com [209.85.161.41]) by mails.dpdk.org (Postfix) with ESMTP id 664FF40266; Thu, 4 Jan 2024 02:17:16 +0100 (CET) Received: by mail-oo1-f41.google.com with SMTP id 006d021491bc7-5961a2726aaso18765eaf.0; Wed, 03 Jan 2024 17:17:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1704331035; x=1704935835; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NN/Tx3LjCDyrAEAN3Xh08isLSXBgGjcpkZj/fG0Ap+I=; b=aT/8ib2J78CRTi8jBnWVttQaq8FfwcF/mIovJHDMtNk3GLxiHgC9GeAxtAWmmfkPwG CCKZ9QhDav1SKPRISIi+LycGavzV3g/HjYm1rVJ1pKTlnw9ILSCrfEffezimpobKvbYB XRhh+iTyTqMDuOrxL1DsV7Oyh9ISqztmGYHTXsXVOkpQJomPmykTIBfYEJ674o0EoqkZ XSOg3f0pElcPySocWci/YdoU57Y7TxnuTqX2InTmqgW4ZdVVlZ0KpcCNf4LDwT1l8FR+ N/xJGQvEgdaAvRlwOsiCu1GNGcZAEC5R9VKied9tsvNGzlu2RjkeYM3MVGxjRWWxhAQJ ciWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704331035; x=1704935835; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=NN/Tx3LjCDyrAEAN3Xh08isLSXBgGjcpkZj/fG0Ap+I=; b=QRiT/vzCi4OU1gOt3ouE243jKvBU9w+lb2Jz040r/zAu5U6/b9Otonc80oai12MU9S 268qSZnu1ifz3oMkEPgATUrTnVIUw7AaiCjtYnjgLCoiPNeVBiwt2iy8xlrvoYkCK4s+ yhLPR+SQksGHmNCYVsibnIOmV5svH3t99Byz/9Hdf1E9GI4PfUva9rFljYDgU3mUdMfH jqsHB+1A55nIcgOGfXyEoCn1fZoiqI1CHoQzPVTB1UmGCk4d3uyXay7GZwkt8s2w4CRk MLBOYwV3cBNkMjqoLGfpIZ5oPOewSZPaqAB2q4IsnAGvUGkjIXNgktk0nj+5SUeRtXwy wFJg== X-Gm-Message-State: AOJu0Yz1YGSDV4WYnJKT/Wf1n5UbPrbBwGiK5M4lZ/9JF0iEyu5t42EC d16mu7/2hydA2Y+25Htv22ybcw/WWpLkBN44xBI= X-Google-Smtp-Source: AGHT+IEVdCRVRLMH8Iag3qlhJDxvwlPIRb5k4bZ2H17CpNfb8EOF2BP+hx3UZA+z1P2MLbQjAUZh+fe3Xb91M5+WC7E= X-Received: by 2002:a05:6820:1ca0:b0:591:2de4:1fb3 with SMTP id ct32-20020a0568201ca000b005912de41fb3mr16714719oob.4.1704331035592; Wed, 03 Jan 2024 17:17:15 -0800 (PST) MIME-Version: 1.0 References: <20231214084054.2593194-1-qi.z.zhang@intel.com> In-Reply-To: From: Shuang Han Date: Thu, 4 Jan 2024 09:17:04 +0800 Message-ID: Subject: Re: [PATCH v2] net/ice: fix link update To: "Zhang, Qi Z" Cc: "Yang, Qiming" , "dev@dpdk.org" , "stable@dpdk.org" Content-Type: multipart/alternative; boundary="000000000000f4cdf4060e147dbf" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000f4cdf4060e147dbf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable ice_atomic_write_link_status in ice_link_update function is not protected by the spinlock, there maybe a situation=EF=BC=9A 1.dev_start call ice_link_update with wait_to_complete =3D 1=EF=BC=8Cand get link_status =3D 0 2.LSC interrupt handler call ice_link_update and get link_status =3D 1 3.LSC interrupt handler call ice_atomic_write_link_status update link status to 1 4.dev_start call ice_atomic_write_link_status update link status to 0 So I think ice_atomic_write_link_status must be protected by spinlock Zhang, Qi Z =E4=BA=8E2023=E5=B9=B412=E6=9C=8821=E6= =97=A5=E5=91=A8=E5=9B=9B 10:44=E5=86=99=E9=81=93=EF=BC=9A > > > > -----Original Message----- > > From: Yang, Qiming > > Sent: Thursday, December 21, 2023 9:44 AM > > To: Zhang, Qi Z > > Cc: dev@dpdk.org; stable@dpdk.org > > Subject: RE: [PATCH v2] net/ice: fix link update > > > > hi > > > > > -----Original Message----- > > > From: Zhang, Qi Z > > > Sent: Thursday, December 14, 2023 4:41 PM > > > To: Yang, Qiming > > > Cc: dev@dpdk.org; Zhang, Qi Z ; stable@dpdk.org > > > Subject: [PATCH v2] net/ice: fix link update > > > > > > The ice_aq_get_link_info function is not thread-safe. However, it is > > > possible to simultaneous invocations during both the dev_start and th= e > > > LSC interrupt handler, potentially leading to unexpected adminq > > > errors. This patch addresses the issue by introducing a thread-safe > > > wrapper that utilizes a spinlock. > > > > > > Fixes: cf911d90e366 ("net/ice: support link update") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Qi Zhang > > > --- > > > v2: > > > - fix coding style warning. > > > > > > drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++------ > > > drivers/net/ice/ice_ethdev.h | 4 ++++ > > > 2 files changed, 24 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/ice/ice_ethdev.c > > > b/drivers/net/ice/ice_ethdev.c index 3ccba4db80..1f8ab5158a 100644 > > > --- a/drivers/net/ice/ice_ethdev.c > > > +++ b/drivers/net/ice/ice_ethdev.c > > > @@ -1804,6 +1804,7 @@ ice_pf_setup(struct ice_pf *pf) > > > } > > > > > > pf->main_vsi =3D vsi; > > > + rte_spinlock_init(&pf->link_lock); > > > > > > return 0; > > > } > > > @@ -3621,17 +3622,31 @@ ice_rxq_intr_setup(struct rte_eth_dev *dev) > > > return 0; > > > } > > > > > > +static enum ice_status > > > +ice_get_link_info_safe(struct ice_pf *pf, bool ena_lse, > > > + struct ice_link_status *link) { > > > + struct ice_hw *hw =3D ICE_PF_TO_HW(pf); > > > + int ret; > > > + > > > + rte_spinlock_lock(&pf->link_lock); > > > + > > > + ret =3D ice_aq_get_link_info(hw->port_info, ena_lse, link, NULL); > > > + > > > + rte_spinlock_unlock(&pf->link_lock); > > > + > > > + return ret; > > > +} > > > + > > > static void > > > ice_get_init_link_status(struct rte_eth_dev *dev) { > > > - struct ice_hw *hw =3D ICE_DEV_PRIVATE_TO_HW(dev->data- > > > >dev_private); > > > struct ice_pf *pf =3D ICE_DEV_PRIVATE_TO_PF(dev->data- > > > >dev_private); > > > bool enable_lse =3D dev->data->dev_conf.intr_conf.lsc ? true : fa= lse; > > > struct ice_link_status link_status; > > > int ret; > > > > > > - ret =3D ice_aq_get_link_info(hw->port_info, enable_lse, > > > - &link_status, NULL); > > > + ret =3D ice_get_link_info_safe(pf, enable_lse, &link_status); > > > if (ret !=3D ICE_SUCCESS) { > > > PMD_DRV_LOG(ERR, "Failed to get link info"); > > > pf->init_link_up =3D false; > > > @@ -3996,7 +4011,7 @@ ice_link_update(struct rte_eth_dev *dev, int > > > wait_to_complete) { #define CHECK_INTERVAL 50 /* 50ms */ #define > > > MAX_REPEAT_TIME 40 /* 2s (40 * 50ms) in total */ > > > - struct ice_hw *hw =3D ICE_DEV_PRIVATE_TO_HW(dev->data- > > > >dev_private); > > > + struct ice_pf *pf =3D ICE_DEV_PRIVATE_TO_PF(dev->data- > > > >dev_private); > > > struct ice_link_status link_status; > > > struct rte_eth_link link, old; > > > int status; > > > @@ -4010,8 +4025,7 @@ ice_link_update(struct rte_eth_dev *dev, int > > > wait_to_complete) > > > > > > do { > > > /* Get link status information from hardware */ > > > - status =3D ice_aq_get_link_info(hw->port_info, enable_lse= , > > > - &link_status, NULL); > > > + status =3D ice_get_link_info_safe(pf, enable_lse, > &link_status); > > > if (status !=3D ICE_SUCCESS) { > > > link.link_speed =3D RTE_ETH_SPEED_NUM_100M; > > > link.link_duplex =3D RTE_ETH_LINK_FULL_DUPLEX; di= ff - > > -git > > > a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h index > > > abe6dcdc23..d607f028e0 100644 > > > --- a/drivers/net/ice/ice_ethdev.h > > > +++ b/drivers/net/ice/ice_ethdev.h > > > @@ -548,6 +548,10 @@ struct ice_pf { > > > uint64_t rss_hf; > > > struct ice_tm_conf tm_conf; > > > uint16_t outer_ethertype; > > > + /* lock prevent race condition between lsc interrupt handler > > > + * and link status update during dev_start. > > > + */ > > > + rte_spinlock_t link_lock; > > > }; > > > > > > #define ICE_MAX_QUEUE_NUM 2048 > > > -- > > > 2.31.1 > > > > > > Acked-by: Qiming Yang > > Applied to dpdk-next-net-intel. > > Thanks > Qi > --000000000000f4cdf4060e147dbf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
ice_atomic_write_link_status in=C2=A0ice_link_update funct= ion is not protected by the spinlock, there maybe a situation=EF=BC=9A
= 1.dev_start call ice_link_update with=C2=A0wait_to_complete =3D 1=EF=BC=8Ca= nd get=C2=A0link_status =3D 0
2.LSC interrupt handler call ic= e_link_update and get link_status =3D 1
3.LSC interrupt handler c= all ice_atomic_write_link_status update link status to 1
4.dev_st= art call ice_atomic_write_link_status update link status to 0
So = I think ice_atomic_write_link_status must be protected by spinlock

Zha= ng, Qi Z <qi.z.zhang@intel.com> =E4=BA=8E2023=E5=B9=B412=E6=9C=8821=E6=97=A5=E5=91=A8=E5=9B=9B 10:44= =E5=86=99=E9=81=93=EF=BC=9A


> -----Original Message-----
> From: Yang, Qiming <qiming.yang@intel.com>
> Sent: Thursday, December 21, 2023 9:44 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org= ; stable@dpdk.org<= br> > Subject: RE: [PATCH v2] net/ice: fix link update
>
> hi
>
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: Thursday, December 14, 2023 4:41 PM
> > To: Yang, Qiming <qiming.yang@intel.com>
> > Cc: dev@dpdk.or= g; Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> > Subject: [PATCH v2] net/ice: fix link update
> >
> > The ice_aq_get_link_info function is not thread-safe. However, it= is
> > possible to simultaneous invocations during both the dev_start an= d the
> > LSC interrupt handler, potentially leading to unexpected adminq > > errors. This patch addresses the issue by introducing a thread-sa= fe
> > wrapper that utilizes a spinlock.
> >
> > Fixes: cf911d90e366 ("net/ice: support link update") > > Cc: stable@d= pdk.org
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > v2:
> > - fix coding style warning.
> >
> >=C2=A0 drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++-----= -
> > drivers/net/ice/ice_ethdev.h |=C2=A0 4 ++++
> >=C2=A0 2 files changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_ethdev.c
> > b/drivers/net/ice/ice_ethdev.c index 3ccba4db80..1f8ab5158a 10064= 4
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -1804,6 +1804,7 @@ ice_pf_setup(struct ice_pf *pf)
> >=C2=A0 =C2=A0 =C2=A0}
> >
> >=C2=A0 =C2=A0 =C2=A0pf->main_vsi =3D vsi;
> > +=C2=A0 =C2=A0rte_spinlock_init(&pf->link_lock);
> >
> >=C2=A0 =C2=A0 =C2=A0return 0;
> >=C2=A0 }
> > @@ -3621,17 +3622,31 @@ ice_rxq_intr_setup(struct rte_eth_dev *de= v)
> >=C2=A0 =C2=A0 =C2=A0return 0;
> >=C2=A0 }
> >
> > +static enum ice_status
> > +ice_get_link_info_safe(struct ice_pf *pf, bool ena_lse,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s= truct ice_link_status *link) {
> > +=C2=A0 =C2=A0struct ice_hw *hw =3D ICE_PF_TO_HW(pf);
> > +=C2=A0 =C2=A0int ret;
> > +
> > +=C2=A0 =C2=A0rte_spinlock_lock(&pf->link_lock);
> > +
> > +=C2=A0 =C2=A0ret =3D ice_aq_get_link_info(hw->port_info, ena_= lse, link, NULL);
> > +
> > +=C2=A0 =C2=A0rte_spinlock_unlock(&pf->link_lock);
> > +
> > +=C2=A0 =C2=A0return ret;
> > +}
> > +
> >=C2=A0 static void
> >=C2=A0 ice_get_init_link_status(struct rte_eth_dev *dev)=C2=A0 { > > -=C2=A0 =C2=A0struct ice_hw *hw =3D ICE_DEV_PRIVATE_TO_HW(dev->= ;data-
> > >dev_private);
> >=C2=A0 =C2=A0 =C2=A0struct ice_pf *pf =3D ICE_DEV_PRIVATE_TO_PF(de= v->data-
> > >dev_private);
> >=C2=A0 =C2=A0 =C2=A0bool enable_lse =3D dev->data->dev_conf.= intr_conf.lsc ? true : false;
> >=C2=A0 =C2=A0 =C2=A0struct ice_link_status link_status;
> >=C2=A0 =C2=A0 =C2=A0int ret;
> >
> > -=C2=A0 =C2=A0ret =3D ice_aq_get_link_info(hw->port_info, enab= le_lse,
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &link_status, NULL);
> > +=C2=A0 =C2=A0ret =3D ice_get_link_info_safe(pf, enable_lse, &= ;link_status);
> >=C2=A0 =C2=A0 =C2=A0if (ret !=3D ICE_SUCCESS) {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PMD_DRV_LOG(ERR, &= quot;Failed to get link info");
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pf->init_link_u= p =3D false;
> > @@ -3996,7 +4011,7 @@ ice_link_update(struct rte_eth_dev *dev, in= t
> > wait_to_complete)=C2=A0 {=C2=A0 #define CHECK_INTERVAL 50=C2=A0 /= * 50ms */=C2=A0 #define
> > MAX_REPEAT_TIME 40=C2=A0 /* 2s (40 * 50ms) in total */
> > -=C2=A0 =C2=A0struct ice_hw *hw =3D ICE_DEV_PRIVATE_TO_HW(dev->= ;data-
> > >dev_private);
> > +=C2=A0 =C2=A0struct ice_pf *pf =3D ICE_DEV_PRIVATE_TO_PF(dev->= ;data-
> > >dev_private);
> >=C2=A0 =C2=A0 =C2=A0struct ice_link_status link_status;
> >=C2=A0 =C2=A0 =C2=A0struct rte_eth_link link, old;
> >=C2=A0 =C2=A0 =C2=A0int status;
> > @@ -4010,8 +4025,7 @@ ice_link_update(struct rte_eth_dev *dev, in= t
> > wait_to_complete)
> >
> >=C2=A0 =C2=A0 =C2=A0do {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Get link status= information from hardware */
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D ice_aq_get_l= ink_info(hw->port_info, enable_lse,
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0&link_status, NULL);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D ice_get_link= _info_safe(pf, enable_lse, &link_status);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (status !=3D IC= E_SUCCESS) {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0link.link_speed =3D RTE_ETH_SPEED_NUM_100M;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0link.link_duplex =3D RTE_ETH_LINK_FULL_DUPLEX; diff -
> -git
> > a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h ind= ex
> > abe6dcdc23..d607f028e0 100644
> > --- a/drivers/net/ice/ice_ethdev.h
> > +++ b/drivers/net/ice/ice_ethdev.h
> > @@ -548,6 +548,10 @@ struct ice_pf {
> >=C2=A0 =C2=A0 =C2=A0uint64_t rss_hf;
> >=C2=A0 =C2=A0 =C2=A0struct ice_tm_conf tm_conf;
> >=C2=A0 =C2=A0 =C2=A0uint16_t outer_ethertype;
> > +=C2=A0 =C2=A0/* lock prevent race condition between lsc interrup= t handler
> > +=C2=A0 =C2=A0 * and link status update during dev_start.
> > +=C2=A0 =C2=A0 */
> > +=C2=A0 =C2=A0rte_spinlock_t link_lock;
> >=C2=A0 };
> >
> >=C2=A0 #define ICE_MAX_QUEUE_NUM=C2=A0 2048
> > --
> > 2.31.1
>
>
> Acked-by: Qiming Yang <qiming.yang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
--000000000000f4cdf4060e147dbf--