[PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Csókás Bence]

Mutexes cannot be taken in a non-preemptible context,
causing a panic in `fec_ptp_save_state()`. Replacing
`ptp_clk_mutex` by `ptp_clk_lock` fixes this.

Fixes: 91c0d987a9788dcc5fe26baafd73bf9242b68900
Fixes: 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5
Fixes: f79959220fa5fbda939592bf91c7a9ea90419040

Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>

Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec.h      |  2 +-
 drivers/net/ethernet/freescale/fec_main.c | 17 ++++++------
 drivers/net/ethernet/freescale/fec_ptp.c  | 32 +++++++++++------------
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 0cebe4b63adb..9aac74d14f26 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -557,7 +557,7 @@ struct fec_enet_private {
 	struct clk *clk_2x_txclk;
 
 	bool ptp_clk_on;
-	struct mutex ptp_clk_mutex;
+	spinlock_t ptp_clk_lock;
 	unsigned int num_tx_queues;
 	unsigned int num_rx_queues;
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b0d60f898249..98d8f8d6034e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2029,6 +2029,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
+	unsigned long flags;
 
 	if (enable) {
 		ret = clk_prepare_enable(fep->clk_enet_out);
@@ -2036,15 +2037,15 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 			return ret;
 
 		if (fep->clk_ptp) {
-			mutex_lock(&fep->ptp_clk_mutex);
+			spin_lock_irqsave(&fep->ptp_clk_lock, flags);
 			ret = clk_prepare_enable(fep->clk_ptp);
 			if (ret) {
-				mutex_unlock(&fep->ptp_clk_mutex);
+				spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 				goto failed_clk_ptp;
 			} else {
 				fep->ptp_clk_on = true;
 			}
-			mutex_unlock(&fep->ptp_clk_mutex);
+			spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 		}
 
 		ret = clk_prepare_enable(fep->clk_ref);
@@ -2059,10 +2060,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 	} else {
 		clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
-			mutex_lock(&fep->ptp_clk_mutex);
+			spin_lock_irqsave(&fep->ptp_clk_lock, flags);
 			clk_disable_unprepare(fep->clk_ptp);
 			fep->ptp_clk_on = false;
-			mutex_unlock(&fep->ptp_clk_mutex);
+			spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 		}
 		clk_disable_unprepare(fep->clk_ref);
 		clk_disable_unprepare(fep->clk_2x_txclk);
@@ -2075,10 +2076,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 		clk_disable_unprepare(fep->clk_ref);
 failed_clk_ref:
 	if (fep->clk_ptp) {
-		mutex_lock(&fep->ptp_clk_mutex);
+		spin_lock_irqsave(&fep->ptp_clk_lock, flags);
 		clk_disable_unprepare(fep->clk_ptp);
 		fep->ptp_clk_on = false;
-		mutex_unlock(&fep->ptp_clk_mutex);
+		spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 	}
 failed_clk_ptp:
 	clk_disable_unprepare(fep->clk_enet_out);
@@ -3907,7 +3908,7 @@ fec_probe(struct platform_device *pdev)
 	}
 
 	fep->ptp_clk_on = false;
-	mutex_init(&fep->ptp_clk_mutex);
+	spin_lock_init(&fep->ptp_clk_lock);
 
 	/* clk_ref is optional, depends on board */
 	fep->clk_ref = devm_clk_get_optional(&pdev->dev, "enet_clk_ref");
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index c74d04f4b2fd..dc8564a1f2d2 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -365,21 +365,21 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
  */
 static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 {
-	struct fec_enet_private *adapter =
+	struct fec_enet_private *fep =
 	    container_of(ptp, struct fec_enet_private, ptp_caps);
 	u64 ns;
-	unsigned long flags;
+	unsigned long flags, flags2;
 
-	mutex_lock(&adapter->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->ptp_clk_lock, flags);
 	/* Check the ptp clock */
-	if (!adapter->ptp_clk_on) {
-		mutex_unlock(&adapter->ptp_clk_mutex);
+	if (!fep->ptp_clk_on) {
+		spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 		return -EINVAL;
 	}
-	spin_lock_irqsave(&adapter->tmreg_lock, flags);
-	ns = timecounter_read(&adapter->tc);
-	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
-	mutex_unlock(&adapter->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->tmreg_lock, flags2);
+	ns = timecounter_read(&fep->tc);
+	spin_unlock_irqrestore(&fep->tmreg_lock, flags2);
+	spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
 
 	*ts = ns_to_timespec64(ns);
 
@@ -401,13 +401,13 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 	    container_of(ptp, struct fec_enet_private, ptp_caps);
 
 	u64 ns;
-	unsigned long flags;
+	unsigned long flags, flags2;
 	u32 counter;
 
-	mutex_lock(&fep->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->ptp_clk_lock, flags2);
 	/* Check the ptp clock */
 	if (!fep->ptp_clk_on) {
-		mutex_unlock(&fep->ptp_clk_mutex);
+		spin_unlock_irqrestore(&fep->ptp_clk_lock, flags2);
 		return -EINVAL;
 	}
 
@@ -421,7 +421,7 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 	writel(counter, fep->hwp + FEC_ATIME);
 	timecounter_init(&fep->tc, &fep->cc, ns);
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
-	mutex_unlock(&fep->ptp_clk_mutex);
+	spin_unlock_irqrestore(&fep->ptp_clk_lock, flags2);
 	return 0;
 }
 
@@ -516,15 +516,15 @@ static void fec_time_keep(struct work_struct *work)
 {
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct fec_enet_private *fep = container_of(dwork, struct fec_enet_private, time_keep);
-	unsigned long flags;
+	unsigned long flags, flags2;
 
-	mutex_lock(&fep->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->ptp_clk_lock, flags2);
 	if (fep->ptp_clk_on) {
 		spin_lock_irqsave(&fep->tmreg_lock, flags);
 		timecounter_read(&fep->tc);
 		spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 	}
-	mutex_unlock(&fep->ptp_clk_mutex);
+	spin_unlock_irqrestore(&fep->ptp_clk_lock, flags2);
 
 	schedule_delayed_work(&fep->time_keep, HZ);
 }
-- 
2.25.1

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Richard Cochran]

On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote:

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index c74d04f4b2fd..dc8564a1f2d2 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -365,21 +365,21 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>   */
>  static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>  {
> -	struct fec_enet_private *adapter =
> +	struct fec_enet_private *fep =
>  	    container_of(ptp, struct fec_enet_private, ptp_caps);
>  	u64 ns;
> -	unsigned long flags;
> +	unsigned long flags, flags2;
>  
> -	mutex_lock(&adapter->ptp_clk_mutex);
> +	spin_lock_irqsave(&fep->ptp_clk_lock, flags);
>  	/* Check the ptp clock */
> -	if (!adapter->ptp_clk_on) {
> -		mutex_unlock(&adapter->ptp_clk_mutex);
> +	if (!fep->ptp_clk_on) {

BTW This test is silly. If functionality isn't available then the code
should simply not register the clock in the first place.

> +		spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
>  		return -EINVAL;
>  	}
> -	spin_lock_irqsave(&adapter->tmreg_lock, flags);
> -	ns = timecounter_read(&adapter->tc);
> -	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
> -	mutex_unlock(&adapter->ptp_clk_mutex);
> +	spin_lock_irqsave(&fep->tmreg_lock, flags2);
> +	ns = timecounter_read(&fep->tc);
> +	spin_unlock_irqrestore(&fep->tmreg_lock, flags2);
> +	spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);

Two spin locks?  Why not just use one?

Thanks,
Richard

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Csókás Bence]

On 2022. 08. 31. 15:54, Richard Cochran wrote:
> On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote:
> 
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
>> index c74d04f4b2fd..dc8564a1f2d2 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -365,21 +365,21 @@ static int fec_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>>    */
>>   static int fec_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>>   {
>> -	struct fec_enet_private *adapter =
>> +	struct fec_enet_private *fep =
>>   	    container_of(ptp, struct fec_enet_private, ptp_caps);
>>   	u64 ns;
>> -	unsigned long flags;
>> +	unsigned long flags, flags2;
>>   
>> -	mutex_lock(&adapter->ptp_clk_mutex);
>> +	spin_lock_irqsave(&fep->ptp_clk_lock, flags);
>>   	/* Check the ptp clock */
>> -	if (!adapter->ptp_clk_on) {
>> -		mutex_unlock(&adapter->ptp_clk_mutex);
>> +	if (!fep->ptp_clk_on) {
> 
> BTW This test is silly. If functionality isn't available then the code
> should simply not register the clock in the first place.

As I understand, `ptp_clk_on` is a flag indicating whether `clk_ipg` is 
running or not, and not a capability thing. The driver switches it 
run-time in `fec_enet_clk_enable()`.

> 
>> +		spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
>>   		return -EINVAL;
>>   	}
>> -	spin_lock_irqsave(&adapter->tmreg_lock, flags);
>> -	ns = timecounter_read(&adapter->tc);
>> -	spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
>> -	mutex_unlock(&adapter->ptp_clk_mutex);
>> +	spin_lock_irqsave(&fep->tmreg_lock, flags2);
>> +	ns = timecounter_read(&fep->tc);
>> +	spin_unlock_irqrestore(&fep->tmreg_lock, flags2);
>> +	spin_unlock_irqrestore(&fep->ptp_clk_lock, flags);
> 
> Two spin locks?  Why not just use one?

One guards the FEC_ATIME_* registers, the other the `ptp_clk_on` flag. 
For instance, `fec_ptp_adjfreq()` takes `tmreg_lock` but not 
`ptp_clk_lock`, and similarly `fec_enet_clk_enable()` takes 
`ptp_clk_lock` but not `tmreg_lock`.

> 
> Thanks,
> Richard

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Andrew Lunn]

On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote:
> Mutexes cannot be taken in a non-preemptible context,
> causing a panic in `fec_ptp_save_state()`. Replacing
> `ptp_clk_mutex` by `ptp_clk_lock` fixes this.
> 
> Fixes: 91c0d987a9788dcc5fe26baafd73bf9242b68900
> Fixes: 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5
> Fixes: f79959220fa5fbda939592bf91c7a9ea90419040
> 
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  2 +-
>  drivers/net/ethernet/freescale/fec_main.c | 17 ++++++------
>  drivers/net/ethernet/freescale/fec_ptp.c  | 32 +++++++++++------------
>  3 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 0cebe4b63adb..9aac74d14f26 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -557,7 +557,7 @@ struct fec_enet_private {
>  	struct clk *clk_2x_txclk;
>  
>  	bool ptp_clk_on;
> -	struct mutex ptp_clk_mutex;
> +	spinlock_t ptp_clk_lock;
>  	unsigned int num_tx_queues;
>  	unsigned int num_rx_queues;
>  
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index b0d60f898249..98d8f8d6034e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2029,6 +2029,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	int ret;
> +	unsigned long flags;

Please keep to reverse christmas tree
  
>  	if (enable) {
>  		ret = clk_prepare_enable(fep->clk_enet_out);
> @@ -2036,15 +2037,15 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>  			return ret;
>  
>  		if (fep->clk_ptp) {
> -			mutex_lock(&fep->ptp_clk_mutex);
> +			spin_lock_irqsave(&fep->ptp_clk_lock, flags);

Is the ptp hardware accessed in interrupt context? If not, you can use
a plain spinlock, not _irqsave..

Looking at fec_enet_interrupt() and fec_enet_collect_events() i do not
see anything.

    Andrew

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Csókás Bence]

On 2022. 08. 31. 16:03, Andrew Lunn wrote:
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index b0d60f898249..98d8f8d6034e 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2029,6 +2029,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>>   {
>>   	struct fec_enet_private *fep = netdev_priv(ndev);
>>   	int ret;
>> +	unsigned long flags;
> 
> Please keep to reverse christmas tree

checkpatch didn't tell me that was a requirement... Easy to fix though.

>    
>>   	if (enable) {
>>   		ret = clk_prepare_enable(fep->clk_enet_out);
>> @@ -2036,15 +2037,15 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>>   			return ret;
>>   
>>   		if (fep->clk_ptp) {
>> -			mutex_lock(&fep->ptp_clk_mutex);
>> +			spin_lock_irqsave(&fep->ptp_clk_lock, flags);
> 
> Is the ptp hardware accessed in interrupt context? If not, you can use
> a plain spinlock, not _irqsave..

`fec_suspend()` calls `fec_enet_clk_enable()`, which may be a 
non-preemptible context, I'm not sure how the PM subsystem's internals 
work...
Besides, with the way this driver is built, function call dependencies 
all over the place, I think it's better safe than sorry. I don't think 
there is any disadvantage (besides maybe a few lost cycles) of using 
_irqsave in regular process context anyways.

Bence

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Andrew Lunn]

On Wed, Aug 31, 2022 at 04:21:47PM +0200, Csókás Bence wrote:
> 
> On 2022. 08. 31. 16:03, Andrew Lunn wrote:
> > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > > index b0d60f898249..98d8f8d6034e 100644
> > > --- a/drivers/net/ethernet/freescale/fec_main.c
> > > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > > @@ -2029,6 +2029,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
> > >   {
> > >   	struct fec_enet_private *fep = netdev_priv(ndev);
> > >   	int ret;
> > > +	unsigned long flags;
> > 
> > Please keep to reverse christmas tree
> 
> checkpatch didn't tell me that was a requirement... Easy to fix though.

checkpatch does not have the smarts to detect this. And it is a netdev
only thing.

> 
> > >   	if (enable) {
> > >   		ret = clk_prepare_enable(fep->clk_enet_out);
> > > @@ -2036,15 +2037,15 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
> > >   			return ret;
> > >   		if (fep->clk_ptp) {
> > > -			mutex_lock(&fep->ptp_clk_mutex);
> > > +			spin_lock_irqsave(&fep->ptp_clk_lock, flags);
> > 
> > Is the ptp hardware accessed in interrupt context? If not, you can use
> > a plain spinlock, not _irqsave..
> 
> `fec_suspend()` calls `fec_enet_clk_enable()`, which may be a
> non-preemptible context, I'm not sure how the PM subsystem's internals
> work...
> Besides, with the way this driver is built, function call dependencies all
> over the place, I think it's better safe than sorry. I don't think there is
> any disadvantage (besides maybe a few lost cycles) of using _irqsave in
> regular process context anyways.

Those using real time will probably disagree.

There is also a different between not being able to sleep, and not
being able to process an interrupt for some other hardware. You got a
report about taking a mutex in atomic context. That just means you
cannot sleep, probably because a spinlock is held. That is very
different to not being able to handle interrupts. You only need
spin_lock_irqsave() if the interrupt handler also needs the same spin
lock to protect it from a thread using the spin lock.

     Andrew

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Francesco Dolcini]

On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote:
> Mutexes cannot be taken in a non-preemptible context,
> causing a panic in `fec_ptp_save_state()`. Replacing
> `ptp_clk_mutex` by `ptp_clk_lock` fixes this.

I would probably add the kernel BUG trace to the commit message.

> Fixes: 91c0d987a9788dcc5fe26baafd73bf9242b68900
> Fixes: 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5
> Fixes: f79959220fa5fbda939592bf91c7a9ea90419040

Just

Fixes: f79959220fa5 ("fec: Restart PPS after link state change")

> 
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>

Is this https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/ the report?

I just stumbled on the same issue on latest torvalds 6.0-rc3.

[   22.718465] =============================
[   22.725431] [ BUG: Invalid wait context ]
[   22.732439] 6.0.0-rc3-00007-gdcf8e5633e2e #1 Tainted: G        W
[   22.742278] -----------------------------
[   22.749217] kworker/3:1/45 is trying to lock:
[   22.757157] c211b71c (&fep->ptp_clk_mutex){+.+.}-{3:3}, at: fec_ptp_gettime+0x30/0xcc
[   22.770814] other info that might help us debug this:
[   22.778718] context-{4:4}
[   22.784065] 4 locks held by kworker/3:1/45:
[   22.790949]  #0: c20072a8 ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x1e4/0x730
[   22.806494]  #1: e6a15f18 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: process_one_work+0x1e4/0x730
[   22.822744]  #2: c287a478 (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x34/0x228
[   22.835884]  #3: c211b2a4 (&dev->tx_global_lock){+...}-{2:2}, at: netif_tx_lock+0x10/0x1c
[   22.849669] stack backtrace:
[   22.855340] CPU: 3 PID: 45 Comm: kworker/3:1 Tainted: G        W          6.0.0-rc3-00007-gdcf8e5633e2e #1
[   22.870713] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[   22.880149] Workqueue: events_power_efficient phy_state_machine
[   22.888999]  unwind_backtrace from show_stack+0x10/0x14
[   22.897107]  show_stack from dump_stack_lvl+0x58/0x70
[   22.905067]  dump_stack_lvl from __lock_acquire+0x2844/0x29d4
[   22.913755]  __lock_acquire from lock_acquire+0x108/0x37c
[   22.922065]  lock_acquire from __mutex_lock+0x88/0x105c
[   22.930203]  __mutex_lock from mutex_lock_nested+0x1c/0x24
[   22.938611]  mutex_lock_nested from fec_ptp_gettime+0x30/0xcc
[   22.947278]  fec_ptp_gettime from fec_ptp_save_state+0x14/0x50
[   22.955991]  fec_ptp_save_state from fec_restart+0x48/0x8d4
[   22.964410]  fec_restart from fec_enet_adjust_link+0xa8/0x184
[   22.973004]  fec_enet_adjust_link from phy_link_change+0x28/0x54
[   22.981898]  phy_link_change from phy_check_link_status+0x94/0x108
[   22.990954]  phy_check_link_status from phy_state_machine+0x68/0x228
[   23.000153]  phy_state_machine from process_one_work+0x288/0x730
[   23.008968]  process_one_work from worker_thread+0x38/0x4d0
[   23.017289]  worker_thread from kthread+0xe4/0x104
[   23.024758]  kthread from ret_from_fork+0x14/0x28
[   23.032065] Exception stack(0xe6a15fb0 to 0xe6a15ff8)
[   23.039664] 5fa0:                                     00000000 00000000 00000000 00000000
[   23.052816] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   23.066101] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000


Francesco

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Jakub Kicinski]

On Wed, 31 Aug 2022 19:12:59 +0200 Francesco Dolcini wrote:
> > Fixes: 91c0d987a9788dcc5fe26baafd73bf9242b68900
> > Fixes: 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5
> > Fixes: f79959220fa5fbda939592bf91c7a9ea90419040  
> 
> Just
> 
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")

Yup, and please remove any empty lines between tags of all types.

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Csókás Bence]

On 2022. 08. 31. 18:24, Andrew Lunn wrote:
 >>> Please keep to reverse christmas tree
 >>
 >> checkpatch didn't tell me that was a requirement... Easy to fix though.
 >
 > checkpatch does not have the smarts to detect this. And it is a netdev
 > only thing.

I thought checkpatch also has the per-subsystem rules, too.

 > There is also a different between not being able to sleep, and not
 > being able to process an interrupt for some other hardware. You got a
 > report about taking a mutex in atomic context. That just means you
 > cannot sleep, probably because a spinlock is held. That is very
 > different to not being able to handle interrupts. You only need
 > spin_lock_irqsave() if the interrupt handler also needs the same spin
 > lock to protect it from a thread using the spin lock.

Alright, I will switch to plain `spin_lock()` then.

On 2022. 08. 31. 19:12, Francesco Dolcini wrote:
> On Wed, Aug 31, 2022 at 02:56:31PM +0200, Csókás Bence wrote:
>> Mutexes cannot be taken in a non-preemptible context,
>> causing a panic in `fec_ptp_save_state()`. Replacing
>> `ptp_clk_mutex` by `ptp_clk_lock` fixes this.
> 
> I would probably add the kernel BUG trace to the commit message.
> 
>> Fixes: 91c0d987a9788dcc5fe26baafd73bf9242b68900
>> Fixes: 6a4d7234ae9a3bb31181f348ade9bbdb55aeb5c5
>> Fixes: f79959220fa5fbda939592bf91c7a9ea90419040
> 
> Just
> 
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change >
>>
>> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Is this https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/ the report?

Yes, precisely.

> 
> I just stumbled on the same issue on latest torvalds 6.0-rc3.
> 
> [   22.718465] =============================
> [   22.725431] [ BUG: Invalid wait context ]
> [   22.732439] 6.0.0-rc3-00007-gdcf8e5633e2e #1 Tainted: G        W
> [   22.742278] -----------------------------
> [   22.749217] kworker/3:1/45 is trying to lock:
> [   22.757157] c211b71c (&fep->ptp_clk_mutex){+.+.}-{3:3}, at: fec_ptp_gettime+0x30/0xcc
> [   22.770814] other info that might help us debug this:
> [   22.778718] context-{4:4}
> [   22.784065] 4 locks held by kworker/3:1/45:
> [   22.790949]  #0: c20072a8 ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x1e4/0x730
> [   22.806494]  #1: e6a15f18 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: process_one_work+0x1e4/0x730
> [   22.822744]  #2: c287a478 (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x34/0x228
> [   22.835884]  #3: c211b2a4 (&dev->tx_global_lock){+...}-{2:2}, at: netif_tx_lock+0x10/0x1c

Thank you, this lock was the source of the problem!

> 
> Francesco
> 

Bence

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Csókás Bence]

On 2022. 09. 01. 9:51, Csókás Bence wrote:
> 
> On 2022. 08. 31. 18:24, Andrew Lunn wrote:
>  >>> Please keep to reverse christmas tree
>  >>
>  >> checkpatch didn't tell me that was a requirement... Easy to fix though.
>  >
>  > checkpatch does not have the smarts to detect this. And it is a netdev
>  > only thing.
> 
> I thought checkpatch also has the per-subsystem rules, too.
> 
>  > There is also a different between not being able to sleep, and not
>  > being able to process an interrupt for some other hardware. You got a
>  > report about taking a mutex in atomic context. That just means you
>  > cannot sleep, probably because a spinlock is held. That is very
>  > different to not being able to handle interrupts. You only need
>  > spin_lock_irqsave() if the interrupt handler also needs the same spin
>  > lock to protect it from a thread using the spin lock.
> 
> Alright, I will switch to plain `spin_lock()` then.

By the way, what about `&fep->tmreg_lock`? Should that also be switched 
to `spin_lock()`? If not, how should I handle the nested locking? 
Calling `spin_lock_irqsave(&fep->tmreg_lock)` after 
`spin_lock(&&fep->ptp_clk_lock)` seems pointless. Should I lock with 
`spin_lock_irqsave(&fep->ptp_clk_lock)` there?

Re: [PATCH] Use a spinlock to guard `fep->ptp_clk_on`

[Andrew Lunn]

On Thu, Sep 01, 2022 at 10:06:03AM +0200, Csókás Bence wrote:
> 
> On 2022. 09. 01. 9:51, Csókás Bence wrote:
> > 
> > On 2022. 08. 31. 18:24, Andrew Lunn wrote:
> >  >>> Please keep to reverse christmas tree
> >  >>
> >  >> checkpatch didn't tell me that was a requirement... Easy to fix though.
> >  >
> >  > checkpatch does not have the smarts to detect this. And it is a netdev
> >  > only thing.
> > 
> > I thought checkpatch also has the per-subsystem rules, too.
> > 
> >  > There is also a different between not being able to sleep, and not
> >  > being able to process an interrupt for some other hardware. You got a
> >  > report about taking a mutex in atomic context. That just means you
> >  > cannot sleep, probably because a spinlock is held. That is very
> >  > different to not being able to handle interrupts. You only need
> >  > spin_lock_irqsave() if the interrupt handler also needs the same spin
> >  > lock to protect it from a thread using the spin lock.
> > 
> > Alright, I will switch to plain `spin_lock()` then.
> 
> By the way, what about `&fep->tmreg_lock`? Should that also be switched to
> `spin_lock()`? If not, how should I handle the nested locking? Calling
> `spin_lock_irqsave(&fep->tmreg_lock)` after `spin_lock(&&fep->ptp_clk_lock)`
> seems pointless. Should I lock with `spin_lock_irqsave(&fep->ptp_clk_lock)`
> there?

Richard was making the point, do you need two locks?

What are the locks protecting? Could you use one lock for both use
cases? Should the other lock also not be an _irqsave()?

       Andrew