<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:DengXian;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:"\@DengXian";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal">Thanks for raising this.<o:p></o:p></p>
<p class="MsoNormal">Agree with your finding, the current implementation ensures data integrity but not guarantee 100% correctness, as updates to link status by interrupts could be missed in certain cases though less happen.<o:p></o:p></p>
<p class="MsoNormal">Please raise a Bugzilla if you think its urgent for your usage and welcome to submit a patch directly.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Regards<o:p></o:p></p>
<p class="MsoNormal">Qi<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Shuang Han <hanshuang87@gmail.com> <br>
<b>Sent:</b> Thursday, January 4, 2024 9:17 AM<br>
<b>To:</b> Zhang, Qi Z <qi.z.zhang@intel.com><br>
<b>Cc:</b> Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org; stable@dpdk.org<br>
<b>Subject:</b> Re: [PATCH v2] net/ice: fix link update<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">ice_atomic_write_link_status in ice_link_update function is not protected by the spinlock, there maybe a situation<span lang="ZH-CN" style="font-family:DengXian">:</span><o:p></o:p></p>
<div>
<p class="MsoNormal">1.dev_start call ice_link_update with wait_to_complete = 1<span lang="ZH-CN" style="font-family:DengXian">,</span>and get link_status = 0<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">2.LSC interrupt handler call ice_link_update and get link_status = 1<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">3.LSC interrupt handler call ice_atomic_write_link_status update link status to 1<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">4.dev_start call ice_atomic_write_link_status update link status to 0<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">So I think ice_atomic_write_link_status must be protected by spinlock<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">Zhang, Qi Z <<a href="mailto:qi.z.zhang@intel.com">qi.z.zhang@intel.com</a>>
<span lang="ZH-CN" style="font-family:DengXian">于</span>2023<span lang="ZH-CN" style="font-family:DengXian">年</span>12<span lang="ZH-CN" style="font-family:DengXian">月</span>21<span lang="ZH-CN" style="font-family:DengXian">日周四</span> 10:44<span lang="ZH-CN" style="font-family:DengXian">写道:</span><o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"><br>
<br>
> -----Original Message-----<br>
> From: Yang, Qiming <<a href="mailto:qiming.yang@intel.com" target="_blank">qiming.yang@intel.com</a>><br>
> Sent: Thursday, December 21, 2023 9:44 AM<br>
> To: Zhang, Qi Z <<a href="mailto:qi.z.zhang@intel.com" target="_blank">qi.z.zhang@intel.com</a>><br>
> Cc: <a href="mailto:dev@dpdk.org" target="_blank">dev@dpdk.org</a>; <a href="mailto:stable@dpdk.org" target="_blank">
stable@dpdk.org</a><br>
> Subject: RE: [PATCH v2] net/ice: fix link update<br>
> <br>
> hi<br>
> <br>
> > -----Original Message-----<br>
> > From: Zhang, Qi Z <<a href="mailto:qi.z.zhang@intel.com" target="_blank">qi.z.zhang@intel.com</a>><br>
> > Sent: Thursday, December 14, 2023 4:41 PM<br>
> > To: Yang, Qiming <<a href="mailto:qiming.yang@intel.com" target="_blank">qiming.yang@intel.com</a>><br>
> > Cc: <a href="mailto:dev@dpdk.org" target="_blank">dev@dpdk.org</a>; Zhang, Qi Z <<a href="mailto:qi.z.zhang@intel.com" target="_blank">qi.z.zhang@intel.com</a>>;
<a href="mailto:stable@dpdk.org" target="_blank">stable@dpdk.org</a><br>
> > Subject: [PATCH v2] net/ice: fix link update<br>
> ><br>
> > The ice_aq_get_link_info function is not thread-safe. However, it is<br>
> > possible to simultaneous invocations during both the dev_start and the<br>
> > LSC interrupt handler, potentially leading to unexpected adminq<br>
> > errors. This patch addresses the issue by introducing a thread-safe<br>
> > wrapper that utilizes a spinlock.<br>
> ><br>
> > Fixes: cf911d90e366 ("net/ice: support link update")<br>
> > Cc: <a href="mailto:stable@dpdk.org" target="_blank">stable@dpdk.org</a><br>
> ><br>
> > Signed-off-by: Qi Zhang <<a href="mailto:qi.z.zhang@intel.com" target="_blank">qi.z.zhang@intel.com</a>><br>
> > ---<br>
> > v2:<br>
> > - fix coding style warning.<br>
> ><br>
> > drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++------<br>
> > drivers/net/ice/ice_ethdev.h | 4 ++++<br>
> > 2 files changed, 24 insertions(+), 6 deletions(-)<br>
> ><br>
> > diff --git a/drivers/net/ice/ice_ethdev.c<br>
> > b/drivers/net/ice/ice_ethdev.c index 3ccba4db80..1f8ab5158a 100644<br>
> > --- a/drivers/net/ice/ice_ethdev.c<br>
> > +++ b/drivers/net/ice/ice_ethdev.c<br>
> > @@ -1804,6 +1804,7 @@ ice_pf_setup(struct ice_pf *pf)<br>
> > }<br>
> ><br>
> > pf->main_vsi = vsi;<br>
> > + rte_spinlock_init(&pf->link_lock);<br>
> ><br>
> > return 0;<br>
> > }<br>
> > @@ -3621,17 +3622,31 @@ ice_rxq_intr_setup(struct rte_eth_dev *dev)<br>
> > return 0;<br>
> > }<br>
> ><br>
> > +static enum ice_status<br>
> > +ice_get_link_info_safe(struct ice_pf *pf, bool ena_lse,<br>
> > + struct ice_link_status *link) {<br>
> > + struct ice_hw *hw = ICE_PF_TO_HW(pf);<br>
> > + int ret;<br>
> > +<br>
> > + rte_spinlock_lock(&pf->link_lock);<br>
> > +<br>
> > + ret = ice_aq_get_link_info(hw->port_info, ena_lse, link, NULL);<br>
> > +<br>
> > + rte_spinlock_unlock(&pf->link_lock);<br>
> > +<br>
> > + return ret;<br>
> > +}<br>
> > +<br>
> > static void<br>
> > ice_get_init_link_status(struct rte_eth_dev *dev) {<br>
> > - struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-<br>
> > >dev_private);<br>
> > struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-<br>
> > >dev_private);<br>
> > bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;<br>
> > struct ice_link_status link_status;<br>
> > int ret;<br>
> ><br>
> > - ret = ice_aq_get_link_info(hw->port_info, enable_lse,<br>
> > - &link_status, NULL);<br>
> > + ret = ice_get_link_info_safe(pf, enable_lse, &link_status);<br>
> > if (ret != ICE_SUCCESS) {<br>
> > PMD_DRV_LOG(ERR, "Failed to get link info");<br>
> > pf->init_link_up = false;<br>
> > @@ -3996,7 +4011,7 @@ ice_link_update(struct rte_eth_dev *dev, int<br>
> > wait_to_complete) { #define CHECK_INTERVAL 50 /* 50ms */ #define<br>
> > MAX_REPEAT_TIME 40 /* 2s (40 * 50ms) in total */<br>
> > - struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-<br>
> > >dev_private);<br>
> > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-<br>
> > >dev_private);<br>
> > struct ice_link_status link_status;<br>
> > struct rte_eth_link link, old;<br>
> > int status;<br>
> > @@ -4010,8 +4025,7 @@ ice_link_update(struct rte_eth_dev *dev, int<br>
> > wait_to_complete)<br>
> ><br>
> > do {<br>
> > /* Get link status information from hardware */<br>
> > - status = ice_aq_get_link_info(hw->port_info, enable_lse,<br>
> > - &link_status, NULL);<br>
> > + status = ice_get_link_info_safe(pf, enable_lse, &link_status);<br>
> > if (status != ICE_SUCCESS) {<br>
> > link.link_speed = RTE_ETH_SPEED_NUM_100M;<br>
> > link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX; diff -<br>
> -git<br>
> > a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h index<br>
> > abe6dcdc23..d607f028e0 100644<br>
> > --- a/drivers/net/ice/ice_ethdev.h<br>
> > +++ b/drivers/net/ice/ice_ethdev.h<br>
> > @@ -548,6 +548,10 @@ struct ice_pf {<br>
> > uint64_t rss_hf;<br>
> > struct ice_tm_conf tm_conf;<br>
> > uint16_t outer_ethertype;<br>
> > + /* lock prevent race condition between lsc interrupt handler<br>
> > + * and link status update during dev_start.<br>
> > + */<br>
> > + rte_spinlock_t link_lock;<br>
> > };<br>
> ><br>
> > #define ICE_MAX_QUEUE_NUM 2048<br>
> > --<br>
> > 2.31.1<br>
> <br>
> <br>
> Acked-by: Qiming Yang <<a href="mailto:qiming.yang@intel.com" target="_blank">qiming.yang@intel.com</a>><br>
<br>
Applied to dpdk-next-net-intel.<br>
<br>
Thanks<br>
Qi<o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>