| Message ID | 20220830111516.82875-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: fec: Use unlocked timecounter reads for saving state | expand |
> - fec_ptp_gettime(&fep->ptp_caps, &fep->ptp_saved_state.ts_phc); > + if (preempt_count_equals(0)) { ~/linux/drivers$ grep -r preempt_count_equals * ~/linux/drivers$ No other driver plays games like this. Why not unconditionally take the lock? Andrew
On 2022. 08. 30. 14:29, Andrew Lunn wrote: >> - fec_ptp_gettime(&fep->ptp_caps, &fep->ptp_saved_state.ts_phc); >> + if (preempt_count_equals(0)) { > > ~/linux/drivers$ grep -r preempt_count_equals * > ~/linux/drivers$ > > No other driver plays games like this. > > Why not unconditionally take the lock? Because then we would be back at the original problem (see Marc's message): | [ 14.001542] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:283 | [ 14.010604] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 13, name: kworker/0:1 | [ 14.018737] preempt_count: 201, expected: 0 We cannot take a mutex in atomic context. However, we also don't *need to* take a mutex in atomic context. > > Andrew If someone has a better solution, I'm open to suggestions. But to me, it seems that there are only 3 options: 1. Unconditionally taking the mutex was what I originally did, but that caused issues in Marc's setup. 2. Not taking the mutex at all is what I proposed in v1 of this patch. But as Richard pointed out, `timecounter_read()` actually does a Read-Modify-Write on the `FEC_ATIME_CTRL` register, that *could* get interrupted if not guarded by the mutex (or atomic context). 3. The final option, check if we are in an atomic or otherwise non-interruptible context, and if not, take a mutex. Otherwise, proceed normally. Which is this version of the patch. Bence
On Tue, Aug 30, 2022 at 05:05:24PM +0200, Csókás Bence wrote: > > > On 2022. 08. 30. 14:29, Andrew Lunn wrote: > > > - fec_ptp_gettime(&fep->ptp_caps, &fep->ptp_saved_state.ts_phc); > > > + if (preempt_count_equals(0)) { > > > > ~/linux/drivers$ grep -r preempt_count_equals * > > ~/linux/drivers$ > > > > No other driver plays games like this. > > > > Why not unconditionally take the lock? > > Because then we would be back at the original problem (see Marc's message): > > | [ 14.001542] BUG: sleeping function called from invalid context at > kernel/locking/mutex.c:283 > > | [ 14.010604] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 13, > name: kworker/0:1 > > | [ 14.018737] preempt_count: 201, expected: 0 > > We cannot take a mutex in atomic context. However, we also don't *need to* > take a mutex in atomic context. You are not taking a mutex, you are taking a spinlock. You can do that in atomic context. Can you protect everything which needs protecting with a spinlock? And avoid sleeping... Andrew
On Tue, Aug 30, 2022 at 05:05:24PM +0200, Csókás Bence wrote: > 3. The final option, check if we are in an atomic or otherwise > non-interruptible context, and if not, take a mutex. Otherwise, proceed > normally. Which is this version of the patch. Just replace the mutex with a spinlock. Thanks, Richard
Hi "Csókás,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on net-next/master linus/master v6.0-rc3 next-20220831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/net-fec-Use-unlocked-timecounter-reads-for-saving-state/20220830-191644
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b05972f01e7d30419987a1f221b5593668fd6448
config: arm-mxs_defconfig
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 1c66bacd6cde1f37d6ac96c45b389666a1334ec0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/1b907e75a5f827529bfe24d68038c69fac840901
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Cs-k-s-Bence/net-fec-Use-unlocked-timecounter-reads-for-saving-state/20220830-191644
git checkout 1b907e75a5f827529bfe24d68038c69fac840901
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/net/ethernet/freescale/fec_ptp.c:648:6: error: call to undeclared function 'preempt_count_equals'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
if (preempt_count_equals(0)) {
^
>> drivers/net/ethernet/freescale/fec_ptp.c:649:39: error: use of undeclared identifier 'flags'
spin_lock_irqsave(&fep->tmreg_lock, flags);
^
>> drivers/net/ethernet/freescale/fec_ptp.c:649:39: error: use of undeclared identifier 'flags'
drivers/net/ethernet/freescale/fec_ptp.c:651:44: error: use of undeclared identifier 'flags'
spin_unlock_irqrestore(&fep->tmreg_lock, flags);
^
4 errors generated.
vim +/preempt_count_equals +648 drivers/net/ethernet/freescale/fec_ptp.c
643
644 void fec_ptp_save_state(struct fec_enet_private *fep)
645 {
646 u32 atime_inc_corr;
647
> 648 if (preempt_count_equals(0)) {
> 649 spin_lock_irqsave(&fep->tmreg_lock, flags);
650 fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
651 spin_unlock_irqrestore(&fep->tmreg_lock, flags);
652 } else {
653 fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
654 }
655 fep->ptp_saved_state.ns_sys = ktime_get_ns();
656
657 fep->ptp_saved_state.at_corr = readl(fep->hwp + FEC_ATIME_CORR);
658 atime_inc_corr = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_CORR_MASK;
659 fep->ptp_saved_state.at_inc_corr = (u8)(atime_inc_corr >> FEC_T_INC_CORR_OFFSET);
660 }
661
On 2022. 08. 30. 17:21, Andrew Lunn wrote: > You are not taking a mutex, you are taking a spinlock. You can do that > in atomic context. Can you protect everything which needs protecting > with a spinlock? And avoid sleeping... You are correct, the issue Marc experienced turns out to be caused by a `mutex_lock()` introduced in 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5... On 2022. 08. 31. 5:41, Richard Cochran wrote: > Just replace the mutex with a spinlock. Will do. Disregard this patch and see "[PATCH] Use a spinlock to guard `fep->ptp_clk_on`" instead Bence
Hi "Csókás,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on net-next/master linus/master v6.0-rc3 next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/net-fec-Use-unlocked-timecounter-reads-for-saving-state/20220830-191644
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b05972f01e7d30419987a1f221b5593668fd6448
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20220902/202209020550.JEmUsnnl-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1b907e75a5f827529bfe24d68038c69fac840901
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Cs-k-s-Bence/net-fec-Use-unlocked-timecounter-reads-for-saving-state/20220830-191644
git checkout 1b907e75a5f827529bfe24d68038c69fac840901
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/ethernet/freescale/fec_ptp.c: In function 'fec_ptp_save_state':
>> drivers/net/ethernet/freescale/fec_ptp.c:648:13: error: implicit declaration of function 'preempt_count_equals'; did you mean 'preempt_count_sub'? [-Werror=implicit-function-declaration]
648 | if (preempt_count_equals(0)) {
| ^~~~~~~~~~~~~~~~~~~~
| preempt_count_sub
In file included from include/linux/bitops.h:7,
from include/linux/thread_info.h:27,
from arch/s390/include/asm/preempt.h:6,
from include/linux/preempt.h:78,
from arch/s390/include/asm/timex.h:13,
from include/linux/timex.h:67,
from include/linux/time32.h:13,
from include/linux/time.h:60,
from include/linux/stat.h:19,
from include/linux/module.h:13,
from drivers/net/ethernet/freescale/fec_ptp.c:10:
>> drivers/net/ethernet/freescale/fec_ptp.c:649:53: error: 'flags' undeclared (first use in this function)
649 | spin_lock_irqsave(&fep->tmreg_lock, flags);
| ^~~~~
include/linux/typecheck.h:11:16: note: in definition of macro 'typecheck'
11 | typeof(x) __dummy2; \
| ^
include/linux/spinlock.h:379:9: note: in expansion of macro 'raw_spin_lock_irqsave'
379 | raw_spin_lock_irqsave(spinlock_check(lock), flags); \
| ^~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/freescale/fec_ptp.c:649:17: note: in expansion of macro 'spin_lock_irqsave'
649 | spin_lock_irqsave(&fep->tmreg_lock, flags);
| ^~~~~~~~~~~~~~~~~
drivers/net/ethernet/freescale/fec_ptp.c:649:53: note: each undeclared identifier is reported only once for each function it appears in
649 | spin_lock_irqsave(&fep->tmreg_lock, flags);
| ^~~~~
include/linux/typecheck.h:11:16: note: in definition of macro 'typecheck'
11 | typeof(x) __dummy2; \
| ^
include/linux/spinlock.h:379:9: note: in expansion of macro 'raw_spin_lock_irqsave'
379 | raw_spin_lock_irqsave(spinlock_check(lock), flags); \
| ^~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/freescale/fec_ptp.c:649:17: note: in expansion of macro 'spin_lock_irqsave'
649 | spin_lock_irqsave(&fep->tmreg_lock, flags);
| ^~~~~~~~~~~~~~~~~
include/linux/typecheck.h:12:25: warning: comparison of distinct pointer types lacks a cast
12 | (void)(&__dummy == &__dummy2); \
| ^~
include/linux/spinlock.h:241:17: note: in expansion of macro 'typecheck'
241 | typecheck(unsigned long, flags); \
| ^~~~~~~~~
include/linux/spinlock.h:379:9: note: in expansion of macro 'raw_spin_lock_irqsave'
379 | raw_spin_lock_irqsave(spinlock_check(lock), flags); \
| ^~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/freescale/fec_ptp.c:649:17: note: in expansion of macro 'spin_lock_irqsave'
649 | spin_lock_irqsave(&fep->tmreg_lock, flags);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +648 drivers/net/ethernet/freescale/fec_ptp.c
643
644 void fec_ptp_save_state(struct fec_enet_private *fep)
645 {
646 u32 atime_inc_corr;
647
> 648 if (preempt_count_equals(0)) {
> 649 spin_lock_irqsave(&fep->tmreg_lock, flags);
650 fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
651 spin_unlock_irqrestore(&fep->tmreg_lock, flags);
652 } else {
653 fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc);
654 }
655 fep->ptp_saved_state.ns_sys = ktime_get_ns();
656
657 fep->ptp_saved_state.at_corr = readl(fep->hwp + FEC_ATIME_CORR);
658 atime_inc_corr = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_CORR_MASK;
659 fep->ptp_saved_state.at_inc_corr = (u8)(atime_inc_corr >> FEC_T_INC_CORR_OFFSET);
660 }
661
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index b656cda75c92..7bc7ab4b5d3a 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -597,7 +597,7 @@ struct fec_enet_private { unsigned int next_counter; struct { - struct timespec64 ts_phc; + u64 ns_phc; u64 ns_sys; u32 at_corr; u8 at_inc_corr; @@ -613,7 +613,7 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr); int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); void fec_ptp_save_state(struct fec_enet_private *fep); -int fec_ptp_restore_state(struct fec_enet_private *fep); +void fec_ptp_restore_state(struct fec_enet_private *fep); /****************************************************************************/ #endif /* FEC_H */ diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c index 78fb8818d168..2ad93668844a 100644 --- a/drivers/net/ethernet/freescale/fec_ptp.c +++ b/drivers/net/ethernet/freescale/fec_ptp.c @@ -636,7 +636,13 @@ void fec_ptp_save_state(struct fec_enet_private *fep) { u32 atime_inc_corr; - fec_ptp_gettime(&fep->ptp_caps, &fep->ptp_saved_state.ts_phc); + if (preempt_count_equals(0)) { + spin_lock_irqsave(&fep->tmreg_lock, flags); + fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc); + spin_unlock_irqrestore(&fep->tmreg_lock, flags); + } else { + fep->ptp_saved_state.ns_phc = timecounter_read(&fep->tc); + } fep->ptp_saved_state.ns_sys = ktime_get_ns(); fep->ptp_saved_state.at_corr = readl(fep->hwp + FEC_ATIME_CORR); @@ -644,16 +650,17 @@ void fec_ptp_save_state(struct fec_enet_private *fep) fep->ptp_saved_state.at_inc_corr = (u8)(atime_inc_corr >> FEC_T_INC_CORR_OFFSET); } -int fec_ptp_restore_state(struct fec_enet_private *fep) +void fec_ptp_restore_state(struct fec_enet_private *fep) { u32 atime_inc = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK; - u64 ns_sys; + u64 ns; writel(fep->ptp_saved_state.at_corr, fep->hwp + FEC_ATIME_CORR); atime_inc |= ((u32)fep->ptp_saved_state.at_inc_corr) << FEC_T_INC_CORR_OFFSET; writel(atime_inc, fep->hwp + FEC_ATIME_INC); - ns_sys = ktime_get_ns() - fep->ptp_saved_state.ns_sys; - timespec64_add_ns(&fep->ptp_saved_state.ts_phc, ns_sys); - return fec_ptp_settime(&fep->ptp_caps, &fep->ptp_saved_state.ts_phc); + ns = ktime_get_ns() - fep->ptp_saved_state.ns_sys + fep->ptp_saved_state.ns_phc; + + writel(ns & fep->cc.mask, fep->hwp + FEC_ATIME); + timecounter_init(&fep->tc, &fep->cc, ns); }
`fec_ptp_save_state()` may be called from an atomic context, which makes `fec_ptp_gettime()` unable to acquire a mutex. Using the lower-level timecounter ops remedies the problem. Reported-by: Marc Kleine-Budde <mkl@pengutronix.de> Fixes: f79959220fa5 Signed-off-by: Csókás Bence <csokas.bence@prolan.hu> --- drivers/net/ethernet/freescale/fec.h | 4 ++-- drivers/net/ethernet/freescale/fec_ptp.c | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 8 deletions(-)