<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=gb2312">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
Hi Bruce,</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
Thank you for your comments and you're right.</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);" class="elementToProof">
My apologies—since this patch was submitted some time ago, I lost track of the context when sending v2 and made an incorrect modification. I'll remove the erroneous code.</div>
<div id="appendonsend"></div>
<div><br>
</div>
<p style="text-align: left; text-indent: 0px; background-color: rgb(255, 255, 255); margin: 0in;" class="elementToProof">
<span style="font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(36, 36, 36);">Regards</span></p>
<p style="text-align: left; text-indent: 0px; background-color: rgb(255, 255, 255); margin: 0in;" class="elementToProof">
<span style="font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(36, 36, 36);">Zhichao</span></p>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<br>
</div>
<hr style="display: inline-block; width: 98%;">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<b>发件人:</b> Richardson, Bruce <bruce.richardson@intel.com><br>
<b>已发送:</b> 2025 年 11 月 12 日 星期三 20:04<br>
<b>收件人:</b> Zeng, ZhichaoX <zhichaox.zeng@intel.com><br>
<b>抄送:</b> dev@dpdk.org <dev@dpdk.org>; stable@dpdk.org <stable@dpdk.org>; Burakov, Anatoly <anatoly.burakov@intel.com>; Jeff Guo <jia.guo@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>; Wenzhuo Lu <wenzhuo.lu@intel.com><br>
<b>主题:</b> Re: [PATCH v2] net/ice: fix statistics read error </div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-size: 11pt;">On Mon, Nov 10, 2025 at 04:05:14PM +0800, Zhichao Zeng wrote:<br>
> The statistics contain 40 bits. The lower 32 bits are read first, followed<br>
> by the upper 8 bits.<br>
><br>
> In some cases, after reading the lower 32 bits, a carry occurs from<br>
> the lower bits, which causes the final statistics to be incorrect.<br>
><br>
> This commit fixes this issue.<br>
><br>
> Fixes: a37bde56314d ("net/ice: support statistics")<br>
> Cc: stable@dpdk.org<br>
><br>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com><br>
><br>
> ---<br>
> v2: replace single retries with loops<br>
> ---<br>
> drivers/net/intel/ice/ice_ethdev.c | 18 +++++++++++-------<br>
> 1 file changed, 11 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c<br>
> index 4669eba7c7..016b25c63a 100644<br>
> --- a/drivers/net/intel/ice/ice_ethdev.c<br>
> +++ b/drivers/net/intel/ice/ice_ethdev.c<br>
> @@ -6351,10 +6351,16 @@ ice_stat_update_40(struct ice_hw *hw,<br>
> uint64_t *stat)<br>
> {<br>
> uint64_t new_data;<br>
> + uint32_t lo_old, hi, lo;<br>
> <br>
> - new_data = (uint64_t)ICE_READ_REG(hw, loreg);<br>
> - new_data |= (uint64_t)(ICE_READ_REG(hw, hireg) & ICE_8_BIT_MASK) <<<br>
> - ICE_32_BIT_WIDTH;<br>
> + do {<br>
> + lo_old = ICE_READ_REG(hw, loreg);<br>
> + hi = ICE_READ_REG(hw, hireg);<br>
> + lo = ICE_READ_REG(hw, loreg);<br>
> + } while (lo_old > lo);<br>
> +<br>
> + new_data = (uint64_t)lo;<br>
> + new_data |= (uint64_t)(hi & ICE_8_BIT_MASK) << ICE_32_BIT_WIDTH;<br>
> <br>
> if (!offset_loaded)<br>
> *offset = new_data;<br>
> @@ -6363,10 +6369,8 @@ ice_stat_update_40(struct ice_hw *hw,<br>
> *stat = new_data - *offset;<br>
> else<br>
> *stat = (uint64_t)((new_data +<br>
> - ((uint64_t)1 << ICE_40_BIT_WIDTH)) -<br>
> - *offset);<br>
> -<br>
> - *stat &= ICE_40_BIT_MASK;<br>
> + ((uint64_t)1 << ICE_32_BIT_WIDTH))<br>
> + - *offset);<br>
<br>
This part wasn't in v1, was it? It looks wrong to me, and the original code<br>
looks correct. Given that offset and new_data should both be 40-bit<br>
quantities, any wraparound would be at 40-bits rather than 32-bits no? Can<br>
you explain why we would use a 32-bit shift here?<br>
<br>
> }<br>
> <br>
> /**<br>
> --<br>
> 2.34.1<br>
><br>
</div>
</body>
</html>