The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
Fixes: 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation") Cc: stable@vger.kernel.org # 6.1 Signed-off-by: Yajun Deng yajun.deng@linux.dev --- drivers/net/ethernet/intel/i40e/i40e_ptp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c index ffea0c9c82f1..97a9efe7b713 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c @@ -361,9 +361,9 @@ static int i40e_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) 1000000ULL << 16);
if (neg_adj) - adj = I40E_PTP_40GB_INCVAL - diff; + adj = freq - diff; else - adj = I40E_PTP_40GB_INCVAL + diff; + adj = freq + diff;
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32);
On 6/26/2023 7:26 PM, Yajun Deng wrote:
The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
Fixes: 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation") Cc: stable@vger.kernel.org # 6.1 Signed-off-by: Yajun Deng yajun.deng@linux.dev
drivers/net/ethernet/intel/i40e/i40e_ptp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c index ffea0c9c82f1..97a9efe7b713 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c @@ -361,9 +361,9 @@ static int i40e_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) 1000000ULL << 16); if (neg_adj)
adj = I40E_PTP_40GB_INCVAL - diff;
adj = freq - diff;
adj = I40E_PTP_40GB_INCVAL + diff;
adj = freq + diff;wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32);
This straight forward fix makes sense. However, it wasn't obvious to me without context why the 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation") was where the fault got introduced. Thus, here is that context for anyone else who failed to spot it just looking at shrunk patch diffs.
--> code before that commit <---
/**
- i40e_ptp_adjfreq - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @ppb: Parts per billion adjustment from the base
- Adjust the frequency of the PHC by the indicated parts per billion from the
- base frequency.
**/ static int i40e_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, freq, diff; int neg_adj = 0;
if (ppb < 0) { neg_adj = 1; ppb = -ppb; } freq = I40E_PTP_40GB_INCVAL;
frequency is left just as base I40E_PTP_40GB_INCVAL.
freq *= ppb; diff = div_u64(freq, 1000000000ULL); if (neg_adj) adj = I40E_PTP_40GB_INCVAL - diff; else adj = I40E_PTP_40GB_INCVAL + diff;
So the base here can't be freq since we modify freq above using *=, but using I40E_PTP_40GB_INCVAL is consistent.
/* At some link speeds, the base incval is so large that directly * multiplying by ppb would result in arithmetic overflow even when * using a u64. Avoid this by instead calculating the new incval * always in terms of the 40GbE clock rate and then multiplying by the * link speed factor afterwards. This does result in slightly lower * precision at lower link speeds, but it is fairly minor. */ smp_mb(); /* Force any pending update before accessing. */ adj *= READ_ONCE(pf->ptp_adj_mult);
Finally, the multiply is applied last. This affects the combined base + difference, and is done in order to avoid overflowing the *= used in the original implementation.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
---> code after that commit <---
/**
- i40e_ptp_adjfreq - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @ppb: Parts per billion adjustment from the base
- Adjust the frequency of the PHC by the indicated parts per billion from the
- base frequency.
**/ static int i40e_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, freq, diff; int neg_adj = 0;
if (ppb < 0) { neg_adj = 1; ppb = -ppb; } smp_mb(); /* Force any pending update before accessing. */ freq = I40E_PTP_40GB_INCVAL * READ_ONCE(pf->ptp_adj_mult); diff = mul_u64_u64_div_u64(freq, (u64)ppb, 1000000000ULL);
Here, we assign freq to be the I40E_PTP_40GB_INCVAL times the ptp_adj_mult value, and then we don't modify it, instead using mul_u64_u64_div_u64.
if (neg_adj) adj = I40E_PTP_40GB_INCVAL - diff; else adj = I40E_PTP_40GB_INCVAL + diff;
But then the diff is applied on the wrong value, and no multiplication is done afterwards.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
---> current version with adjust_by_scaled_ppm <---
/**
- i40e_ptp_adjfine - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @scaled_ppm: Scaled parts per million adjustment from base
- Adjust the frequency of the PHC by the indicated delta from the base
- frequency.
- Scaled parts per million is ppm with a 16 bit binary fractional field.
**/ static int i40e_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, base_adj;
smp_mb(); /* Force any pending update before accessing. */ base_adj = I40E_PTP_40GB_INCVAL * READ_ONCE(pf->ptp_adj_mult); adj = adjust_by_scaled_ppm(base_adj, scaled_ppm);
Using adjust_by_scaled_ppm correctly performs the calculation and uses the base adjustment, so there's no error here.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
Thanks for finding and fixing this mistake. I think its the simplest fix to get into the stable kernel that are broken, since taking the adjust_by_scaled_ppm version would require additional patches.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
On 2023/6/28 04:20, Jacob Keller wrote:
On 6/26/2023 7:26 PM, Yajun Deng wrote:
The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
Fixes: 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation") Cc: stable@vger.kernel.org # 6.1 Signed-off-by: Yajun Deng yajun.deng@linux.dev
drivers/net/ethernet/intel/i40e/i40e_ptp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c index ffea0c9c82f1..97a9efe7b713 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c @@ -361,9 +361,9 @@ static int i40e_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) 1000000ULL << 16); if (neg_adj)
adj = I40E_PTP_40GB_INCVAL - diff;
adj = freq - diff;
adj = I40E_PTP_40GB_INCVAL + diff;
adj = freq + diff;wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32);
This straight forward fix makes sense. However, it wasn't obvious to me without context why the 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation") was where the fault got introduced. Thus, here is that context for anyone else who failed to spot it just looking at shrunk patch diffs.
--> code before that commit <---
/**
- i40e_ptp_adjfreq - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @ppb: Parts per billion adjustment from the base
- Adjust the frequency of the PHC by the indicated parts per billion from the
- base frequency.
**/ static int i40e_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, freq, diff; int neg_adj = 0;
if (ppb < 0) { neg_adj = 1; ppb = -ppb; } freq = I40E_PTP_40GB_INCVAL;frequency is left just as base I40E_PTP_40GB_INCVAL.
freq *= ppb; diff = div_u64(freq, 1000000000ULL); if (neg_adj) adj = I40E_PTP_40GB_INCVAL - diff; else adj = I40E_PTP_40GB_INCVAL + diff;So the base here can't be freq since we modify freq above using *=, but using I40E_PTP_40GB_INCVAL is consistent.
/* At some link speeds, the base incval is so large that directly * multiplying by ppb would result in arithmetic overflow even when * using a u64. Avoid this by instead calculating the new incval * always in terms of the 40GbE clock rate and then multiplying by the * link speed factor afterwards. This does result in slightly lower * precision at lower link speeds, but it is fairly minor. */ smp_mb(); /* Force any pending update before accessing. */ adj *= READ_ONCE(pf->ptp_adj_mult);Finally, the multiply is applied last. This affects the combined base + difference, and is done in order to avoid overflowing the *= used in the original implementation.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
---> code after that commit <---
/**
- i40e_ptp_adjfreq - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @ppb: Parts per billion adjustment from the base
- Adjust the frequency of the PHC by the indicated parts per billion from the
- base frequency.
**/ static int i40e_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, freq, diff; int neg_adj = 0;
if (ppb < 0) { neg_adj = 1; ppb = -ppb; } smp_mb(); /* Force any pending update before accessing. */ freq = I40E_PTP_40GB_INCVAL * READ_ONCE(pf->ptp_adj_mult); diff = mul_u64_u64_div_u64(freq, (u64)ppb, 1000000000ULL);Here, we assign freq to be the I40E_PTP_40GB_INCVAL times the ptp_adj_mult value, and then we don't modify it, instead using mul_u64_u64_div_u64.
if (neg_adj) adj = I40E_PTP_40GB_INCVAL - diff; else adj = I40E_PTP_40GB_INCVAL + diff;But then the diff is applied on the wrong value, and no multiplication is done afterwards.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
---> current version with adjust_by_scaled_ppm <---
/**
- i40e_ptp_adjfine - Adjust the PHC frequency
- @ptp: The PTP clock structure
- @scaled_ppm: Scaled parts per million adjustment from base
- Adjust the frequency of the PHC by the indicated delta from the base
- frequency.
- Scaled parts per million is ppm with a 16 bit binary fractional field.
**/ static int i40e_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) { struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps); struct i40e_hw *hw = &pf->hw; u64 adj, base_adj;
smp_mb(); /* Force any pending update before accessing. */ base_adj = I40E_PTP_40GB_INCVAL * READ_ONCE(pf->ptp_adj_mult); adj = adjust_by_scaled_ppm(base_adj, scaled_ppm);Using adjust_by_scaled_ppm correctly performs the calculation and uses the base adjustment, so there's no error here.
wr32(hw, I40E_PRTTSYN_INC_L, adj & 0xFFFFFFFF); wr32(hw, I40E_PRTTSYN_INC_H, adj >> 32); return 0;}
Thanks for finding and fixing this mistake. I think its the simplest fix to get into the stable kernel that are broken, since taking the adjust_by_scaled_ppm version would require additional patches.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Kindly ping...
On 9/25/2023 12:55 AM, Yajun Deng wrote:
On 2023/6/28 04:20, Jacob Keller wrote:
On 6/26/2023 7:26 PM, Yajun Deng wrote:
The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
...
Thanks for finding and fixing this mistake. I think its the simplest fix to get into the stable kernel that are broken, since taking the adjust_by_scaled_ppm version would require additional patches.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Kindly ping...
As this patch looks to be for stable, you need to follow the process for that. I believe your situation would fall into option 3: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Thanks, Tony
On 2023/9/26 07:59, Tony Nguyen wrote:
On 9/25/2023 12:55 AM, Yajun Deng wrote:
On 2023/6/28 04:20, Jacob Keller wrote:
On 6/26/2023 7:26 PM, Yajun Deng wrote:
The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
...
Thanks for finding and fixing this mistake. I think its the simplest fix to get into the stable kernel that are broken, since taking the adjust_by_scaled_ppm version would require additional patches.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Kindly ping...
As this patch looks to be for stable, you need to follow the process for that. I believe your situation would fall into option 3: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Yes, it needs an upstream commit ID. But this patch didn't need to apply to the upstream.
As the commit of the patch, the issue was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the commit is a new feature and hasn't been backported to the stable releases.
Therefore, the patch does not have an upstream commit ID, and only needs to be applied to stable.
Thanks, Tony
On Tue, Sep 26, 2023 at 09:54:29AM +0800, Yajun Deng wrote:
On 2023/9/26 07:59, Tony Nguyen wrote:
On 9/25/2023 12:55 AM, Yajun Deng wrote:
On 2023/6/28 04:20, Jacob Keller wrote:
On 6/26/2023 7:26 PM, Yajun Deng wrote:
The new adjustment should be based on the base frequency, not the I40E_PTP_40GB_INCVAL in i40e_ptp_adjfine().
This issue was introduced in commit 3626a690b717 ("i40e: use mul_u64_u64_div_u64 for PTP frequency calculation"), and was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the latter is a new feature and hasn't been backported to the stable releases.
This issue affects both v6.0 and v6.1 versions, and the v6.1 version is an LTS version.
...
Thanks for finding and fixing this mistake. I think its the simplest fix to get into the stable kernel that are broken, since taking the adjust_by_scaled_ppm version would require additional patches.
Reviewed-by: Jacob Keller jacob.e.keller@intel.com
Kindly ping...
As this patch looks to be for stable, you need to follow the process for that. I believe your situation would fall into option 3: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Yes, it needs an upstream commit ID. But this patch didn't need to apply to the upstream.
As the commit of the patch, the issue was fixed in commit 1060707e3809 ("ptp: introduce helpers to adjust by scaled parts per million"). However the commit is a new feature and hasn't been backported to the stable releases.
Therefore, the patch does not have an upstream commit ID, and only needs to be applied to stable.
That wasn't very obvious to most of us, perhaps resend it and explicitly ask for acks/reviews so it can be only applied to the 6.1.y tree?
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jacob Keller -
Tony Nguyen -
Yajun Deng