[PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on`

[PATCH v2] net: fec: 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 `tmreg_lock` fixes this.

Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
---
 drivers/net/ethernet/freescale/fec.h      |  1 -
 drivers/net/ethernet/freescale/fec_main.c | 17 +++++++-------
 drivers/net/ethernet/freescale/fec_ptp.c  | 28 ++++++++---------------
 3 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 0cebe4b63adb..38f095260e1f 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -557,7 +557,6 @@ struct fec_enet_private {
 	struct clk *clk_2x_txclk;
 
 	bool ptp_clk_on;
-	struct mutex ptp_clk_mutex;
 	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..ab1ee9508f76 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2028,6 +2028,7 @@ static void fec_enet_phy_reset_after_clk_enable(struct net_device *ndev)
 static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	unsigned long flags;
 	int ret;
 
 	if (enable) {
@@ -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->tmreg_lock, flags);
 			ret = clk_prepare_enable(fep->clk_ptp);
 			if (ret) {
-				mutex_unlock(&fep->ptp_clk_mutex);
+				spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 				goto failed_clk_ptp;
 			} else {
 				fep->ptp_clk_on = true;
 			}
-			mutex_unlock(&fep->ptp_clk_mutex);
+			spin_unlock_irqrestore(&fep->tmreg_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->tmreg_lock, flags);
 			clk_disable_unprepare(fep->clk_ptp);
 			fep->ptp_clk_on = false;
-			mutex_unlock(&fep->ptp_clk_mutex);
+			spin_unlock_irqrestore(&fep->tmreg_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->tmreg_lock, flags);
 		clk_disable_unprepare(fep->clk_ptp);
 		fep->ptp_clk_on = false;
-		mutex_unlock(&fep->ptp_clk_mutex);
+		spin_unlock_irqrestore(&fep->tmreg_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->tmreg_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..8dd5a2615a89 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -365,21 +365,19 @@ 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;
 
-	mutex_lock(&adapter->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->tmreg_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->tmreg_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);
+	ns = timecounter_read(&fep->tc);
+	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 
 	*ts = ns_to_timespec64(ns);
 
@@ -404,10 +402,10 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 	unsigned long flags;
 	u32 counter;
 
-	mutex_lock(&fep->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->tmreg_lock, flags);
 	/* Check the ptp clock */
 	if (!fep->ptp_clk_on) {
-		mutex_unlock(&fep->ptp_clk_mutex);
+		spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 		return -EINVAL;
 	}
 
@@ -417,11 +415,9 @@ static int fec_ptp_settime(struct ptp_clock_info *ptp,
 	 */
 	counter = ns & fep->cc.mask;
 
-	spin_lock_irqsave(&fep->tmreg_lock, flags);
 	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);
 	return 0;
 }
 
@@ -518,13 +514,11 @@ static void fec_time_keep(struct work_struct *work)
 	struct fec_enet_private *fep = container_of(dwork, struct fec_enet_private, time_keep);
 	unsigned long flags;
 
-	mutex_lock(&fep->ptp_clk_mutex);
+	spin_lock_irqsave(&fep->tmreg_lock, flags);
 	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->tmreg_lock, flags);
 
 	schedule_delayed_work(&fep->time_keep, HZ);
 }
@@ -599,8 +593,6 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	}
 	fep->ptp_inc = NSEC_PER_SEC / fep->cycle_speed;
 
-	spin_lock_init(&fep->tmreg_lock);
-
 	fec_ptp_start_cyclecounter(ndev);
 
 	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
-- 
2.25.1

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

[Francesco Dolcini]

On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
> 
> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
This should be removed, there was no issue with just this commit. Am I
wrong?

One of the reasons of the Fixes tag is to backport to stable releases,
and you do not want this commit in 5.15.x.

Francesco

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

[Csókás Bence]

On 2022. 09. 01. 17:04, Francesco Dolcini wrote:
> On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
>>
>> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> This should be removed, there was no issue with just this commit. Am I
> wrong?
> 
> One of the reasons of the Fixes tag is to backport to stable releases,
> and you do not want this commit in 5.15.x.

I do want it, actually, because I got a PATCH AUTOSEL already for 
f79959220fa5fbda939592bf91c7a9ea90419040 for 5.15 & 5.19, but I told 
them not to backport until this fix is merged too.

Bence

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

[Florian Fainelli]

On 9/1/2022 7:04 AM, 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 `tmreg_lock` fixes this.
> 
> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> ---

[snip]

>   	schedule_delayed_work(&fep->time_keep, HZ);
>   }
> @@ -599,8 +593,6 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>   	}
>   	fep->ptp_inc = NSEC_PER_SEC / fep->cycle_speed;
>   
> -	spin_lock_init(&fep->tmreg_lock);

This change needs to be kept as there is no other code in the driver 
that would initialize the tmreg_lock otherwise. Try building a kernel 
with spinlock debugging enabled and you should see it barf with an 
incorrect spinlock bad magic.
-- 
Florian

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

[Csókás Bence]

On 2022. 09. 01. 18:26, Florian Fainelli wrote:
> 
>>       schedule_delayed_work(&fep->time_keep, HZ);
>>   }
>> @@ -599,8 +593,6 @@ void fec_ptp_init(struct platform_device *pdev, 
>> int irq_idx)
>>       }
>>       fep->ptp_inc = NSEC_PER_SEC / fep->cycle_speed;
>> -    spin_lock_init(&fep->tmreg_lock);
> 
> This change needs to be kept as there is no other code in the driver 
> that would initialize the tmreg_lock otherwise. Try building a kernel 
> with spinlock debugging enabled and you should see it barf with an 
> incorrect spinlock bad magic.

`fec_ptp_init()` is called from `fec_probe()`, which init's the spinlock:

 > @@ -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->tmreg_lock);

Bence

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

[Florian Fainelli]

On 9/2/2022 12:35 AM, Csókás Bence wrote:
> 
> On 2022. 09. 01. 18:26, Florian Fainelli wrote:
>>
>>>       schedule_delayed_work(&fep->time_keep, HZ);
>>>   }
>>> @@ -599,8 +593,6 @@ void fec_ptp_init(struct platform_device *pdev, 
>>> int irq_idx)
>>>       }
>>>       fep->ptp_inc = NSEC_PER_SEC / fep->cycle_speed;
>>> -    spin_lock_init(&fep->tmreg_lock);
>>
>> This change needs to be kept as there is no other code in the driver 
>> that would initialize the tmreg_lock otherwise. Try building a kernel 
>> with spinlock debugging enabled and you should see it barf with an 
>> incorrect spinlock bad magic.
> 
> `fec_ptp_init()` is called from `fec_probe()`, which init's the spinlock:
> 
>  > @@ -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->tmreg_lock);

Ah indeed, I missed that, thanks!
-- 
Florian

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

[Francesco Dolcini]

On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
> 
> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>

Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com> # Toradex Apalis iMX6

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 1 Sep 2022 16:04:03 +0200 you wrote:
> Mutexes cannot be taken in a non-preemptible context,
> causing a panic in `fec_ptp_save_state()`. Replacing
> `ptp_clk_mutex` by `tmreg_lock` fixes this.
> 
> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> 
> [...]

Here is the summary with links:
  - [v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on`
    https://git.kernel.org/netdev/net/c/b353b241f1eb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2509 bytes --]

On 9/1/22 4:04 PM, 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 `tmreg_lock` fixes this.

I was on holidays, but this doesn't look good.

> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  1 -
>  drivers/net/ethernet/freescale/fec_main.c | 17 +++++++-------
>  drivers/net/ethernet/freescale/fec_ptp.c  | 28 ++++++++---------------
>  3 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 0cebe4b63adb..38f095260e1f 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -557,7 +557,6 @@ struct fec_enet_private {
>  	struct clk *clk_2x_txclk;
>  
>  	bool ptp_clk_on;
> -	struct mutex ptp_clk_mutex;
>  	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..ab1ee9508f76 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2028,6 +2028,7 @@ static void fec_enet_phy_reset_after_clk_enable(struct net_device *ndev)
>  static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> +	unsigned long flags;
>  	int ret;
>  
>  	if (enable) {
> @@ -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->tmreg_lock, flags);
>  			ret = clk_prepare_enable(fep->clk_ptp);

clock_prepare() (and thus clk_prepare_enable()) must not be called from atomic
context.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2976 bytes --]

On 05.09.2022 09:38:04, Marc Kleine-Budde wrote:
> On 9/1/22 4:04 PM, 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 `tmreg_lock` fixes this.
> 
> I was on holidays, but this doesn't look good.

Does anyone care to fix this? Csókás?

> > @@ -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->tmreg_lock, flags);
> >  			ret = clk_prepare_enable(fep->clk_ptp);
> 
> clock_prepare() (and thus clk_prepare_enable()) must not be called from atomic
> context.

You cannot call clk_prepare_enable() from atomic context. If you compile
your kernel with lockdep, you'll get this splat:

| [    5.907789] BUG: sleeping function called from invalid context at drivers/net/ethernet/freescale/fec_main.c:2071
| [    5.918140] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: swapper/0
| [    5.926181] preempt_count: 1, expected: 0                                                    
| [    5.930223] 2 locks held by swapper/0/1:                                                     
| [    5.934180]  #0: c41a4094 (&dev->mutex){....}-{4:4}, at: __driver_attach+0x8c/0x150
| [    5.941968]  #1: c42439b0 (&fep->tmreg_lock){....}-{3:3}, at: fec_enet_clk_enable+0x5c/0x25c
| [    5.950533] irq event stamp: 124698                                                          
| [    5.954052] hardirqs last  enabled at (124697): [<c0f6c6dc>] _raw_spin_unlock_irqrestore+0x58/0x6c
| [    5.963058] hardirqs last disabled at (124698): [<c0f6c490>] _raw_spin_lock_irqsave+0x88/0xa4
| [    5.971622] softirqs last  enabled at (122454): [<c02b47bc>] bdi_register_va+0x1ac/0x1d8                                                                                                     
| [    5.979753] softirqs last disabled at (122452): [<c02b4738>] bdi_register_va+0x128/0x1d8

| $ head -2071 drivers/net/ethernet/freescale/fec_main.c | tail -15
| 
| static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
| {
|         struct fec_enet_private *fep = netdev_priv(ndev);
|         unsigned long flags;
|         int ret;
| 
|         if (enable) {
|                 ret = clk_prepare_enable(fep->clk_enet_out);
|                 if (ret)
|                         return ret;
| 
|                 if (fep->clk_ptp) {
|                         spin_lock_irqsave(&fep->tmreg_lock, flags);
|                         ret = clk_prepare_enable(fep->clk_ptp);

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Francesco Dolcini]

On Wed, Sep 07, 2022 at 04:39:15PM +0200, Marc Kleine-Budde wrote:
> On 05.09.2022 09:38:04, Marc Kleine-Budde wrote:
> > On 9/1/22 4:04 PM, 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 `tmreg_lock` fixes this.
> > 
> > I was on holidays, but this doesn't look good.
> 
> Does anyone care to fix this? Csókás?
I do care, however I do not really have resources for anything better than
reverting both of the problematic commits.

If we do not have a fix, I guess this is the only way. Csókás?

For some reason when I tested on imx6 :-( I had no warning, now the
change was merged and while doing a quick test on a imx7
I got new issues.

[   37.061582] ========================================================
[   37.070203] WARNING: possible irq lock inversion dependency detected
[   37.078847] 6.0.0-rc4-00137-g506357871c18 #3 Tainted: G        W
[   37.087944] --------------------------------------------------------
[   37.096615] systemd/580 just changed the state of lock:
[   37.104160] c218f2a4 (&dev->tx_global_lock){+.-.}-{2:2}, at: dev_watchdog+0x18/0x2c4
[   37.116691] but this lock took another, SOFTIRQ-unsafe lock in the past:
[   37.125896]  (&fep->tmreg_lock){+.+.}-{2:2}
[   37.125921]
[   37.125921]
[   37.125921] and interrupts could create inverse lock ordering between them.
[   37.125921]
[   37.153018]
[   37.153018] other info that might help us debug this:
[   37.163824]  Possible interrupt unsafe locking scenario:
[   37.163824]
[   37.174714]        CPU0                    CPU1
[   37.181215]        ----                    ----
[   37.187631]   lock(&fep->tmreg_lock);
[   37.193110]                                local_irq_disable();
[   37.200841]                                lock(&dev->tx_global_lock);
[   37.209153]                                lock(&fep->tmreg_lock);
[   37.217061]   <Interrupt>
[   37.221328]     lock(&dev->tx_global_lock);
[   37.227167]
[   37.227167]  *** DEADLOCK ***

that just goes away reverting this patch.

Francesco

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

[Csókás Bence]

> On 05.09.2022 09:38:04, Marc Kleine-Budde wrote:
> > On 9/1/22 4:04 PM, 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 `tmreg_lock` fixes this.
> >
> >I was on holidays, but this doesn't look good.
> 
> Does anyone care to fix this? Csókás?

Yes, I will attempt to fix it. But, like you the week before, I am also out of office right now, so please be patient.

Bence

[REGRESSION] Re: [PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on`

[Guenter Roeck]

On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
> 
> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>

For regzbot:

This patch results in the following backtrace.

[   18.401688] BUG: sleeping function called from invalid context at drivers/clk/imx/clk-pllv3.c:68
[   18.402277] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: swapper/0
[   18.402531] preempt_count: 1, expected: 0
[   18.402781] 3 locks held by swapper/0/1:
[   18.402967]  #0: c423ac8c (&dev->mutex){....}-{3:3}, at: __driver_attach+0x80/0x158
[   18.404364]  #1: c40dc8e8 (&fep->tmreg_lock){....}-{2:2}, at: fec_enet_clk_enable+0x58/0x250
[   18.404752]  #2: c1a71af8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0xc/0xd4
[   18.405246] irq event stamp: 129384
[   18.405403] hardirqs last  enabled at (129383): [<c10850b0>] _raw_spin_unlock_irqrestore+0x50/0x64
[   18.405667] hardirqs last disabled at (129384): [<c1084e70>] _raw_spin_lock_irqsave+0x64/0x68
[   18.405915] softirqs last  enabled at (129218): [<c01017bc>] __do_softirq+0x2ac/0x604
[   18.406255] softirqs last disabled at (129209): [<c012eee4>] __irq_exit_rcu+0x138/0x17c
[   18.406792] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G                 N 6.0.0-rc5 #1
[   18.407131] Hardware name: Freescale i.MX7 Dual (Device Tree)
[   18.407590]  unwind_backtrace from show_stack+0x10/0x14
[   18.407890]  show_stack from dump_stack_lvl+0x68/0x90
[   18.408097]  dump_stack_lvl from __might_resched+0x17c/0x284
[   18.408328]  __might_resched from clk_pllv3_wait_lock+0x4c/0xcc
[   18.408557]  clk_pllv3_wait_lock from clk_core_prepare+0xc4/0x328
[   18.408783]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.408986]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.409205]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.409416]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.409631]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.409847]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.410065]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.410284]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.410513]  clk_core_prepare from clk_core_prepare+0x50/0x328
[   18.410729]  clk_core_prepare from clk_prepare+0x20/0x30
[   18.410936]  clk_prepare from fec_enet_clk_enable+0x68/0x250
[   18.411143]  fec_enet_clk_enable from fec_probe+0x32c/0x1430
[   18.411352]  fec_probe from platform_probe+0x58/0xbc
[   18.411558]  platform_probe from really_probe+0xc4/0x2f4
[   18.411772]  really_probe from __driver_probe_device+0x80/0xe4
[   18.411983]  __driver_probe_device from driver_probe_device+0x2c/0xc4
[   18.412203]  driver_probe_device from __driver_attach+0x8c/0x158
[   18.412418]  __driver_attach from bus_for_each_dev+0x74/0xc0
[   18.412631]  bus_for_each_dev from bus_add_driver+0x154/0x1e8
[   18.412844]  bus_add_driver from driver_register+0x88/0x11c
[   18.413055]  driver_register from do_one_initcall+0x68/0x428
[   18.413271]  do_one_initcall from kernel_init_freeable+0x18c/0x240
[   18.413502]  kernel_init_freeable from kernel_init+0x14/0x144
[   18.413721]  kernel_init from ret_from_fork+0x14/0x28
[   18.414036] Exception stack(0xd0825fb0 to 0xd0825ff8)
[   18.414523] 5fa0:                                     00000000 00000000 00000000 00000000
[   18.414792] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   18.415032] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000

Re: [REGRESSION] Re: [PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on` #forregzbot

[Thorsten Leemhuis]

TWIMC: this mail is primarily send for documentation purposes and for
regzbot, my Linux kernel regression tracking bot. These mails usually
contain '#forregzbot' in the subject, to make them easy to spot and filter.

On 14.09.22 15:53, Guenter Roeck wrote:
> On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
>>
>> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
>> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
>> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
>> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
> 
> For regzbot:

Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:

#regzbot ^introduced b353b241f1eb9b626
#regzbot title net: fec: backtrace: BUG: sleeping function called from
invalid context at drivers/clk/imx/clk-pllv3.c
#regzbot ignore-activity
#regzbot monitor:
https://lore.kernel.org/all/20220912073106.2544207-1-bence98@sch.bme.hu/
#regzbot monitor:
https://lore.kernel.org/all/20220912070143.98153-1-francesco.dolcini@toradex.com/

> This patch results in the following backtrace.
> 
> [   18.401688] BUG: sleeping function called from invalid context at drivers/clk/imx/clk-pllv3.c:68
> [   18.402277] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: swapper/0
> [   18.402531] preempt_count: 1, expected: 0
> [   18.402781] 3 locks held by swapper/0/1:
> [   18.402967]  #0: c423ac8c (&dev->mutex){....}-{3:3}, at: __driver_attach+0x80/0x158
> [   18.404364]  #1: c40dc8e8 (&fep->tmreg_lock){....}-{2:2}, at: fec_enet_clk_enable+0x58/0x250
> [   18.404752]  #2: c1a71af8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0xc/0xd4
> [   18.405246] irq event stamp: 129384
> [   18.405403] hardirqs last  enabled at (129383): [<c10850b0>] _raw_spin_unlock_irqrestore+0x50/0x64
> [   18.405667] hardirqs last disabled at (129384): [<c1084e70>] _raw_spin_lock_irqsave+0x64/0x68
> [   18.405915] softirqs last  enabled at (129218): [<c01017bc>] __do_softirq+0x2ac/0x604
> [   18.406255] softirqs last disabled at (129209): [<c012eee4>] __irq_exit_rcu+0x138/0x17c
> [   18.406792] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G                 N 6.0.0-rc5 #1
> [   18.407131] Hardware name: Freescale i.MX7 Dual (Device Tree)
> [   18.407590]  unwind_backtrace from show_stack+0x10/0x14
> [   18.407890]  show_stack from dump_stack_lvl+0x68/0x90
> [   18.408097]  dump_stack_lvl from __might_resched+0x17c/0x284
> [   18.408328]  __might_resched from clk_pllv3_wait_lock+0x4c/0xcc
> [   18.408557]  clk_pllv3_wait_lock from clk_core_prepare+0xc4/0x328
> [   18.408783]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.408986]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.409205]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.409416]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.409631]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.409847]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.410065]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.410284]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.410513]  clk_core_prepare from clk_core_prepare+0x50/0x328
> [   18.410729]  clk_core_prepare from clk_prepare+0x20/0x30
> [   18.410936]  clk_prepare from fec_enet_clk_enable+0x68/0x250
> [   18.411143]  fec_enet_clk_enable from fec_probe+0x32c/0x1430
> [   18.411352]  fec_probe from platform_probe+0x58/0xbc
> [   18.411558]  platform_probe from really_probe+0xc4/0x2f4
> [   18.411772]  really_probe from __driver_probe_device+0x80/0xe4
> [   18.411983]  __driver_probe_device from driver_probe_device+0x2c/0xc4
> [   18.412203]  driver_probe_device from __driver_attach+0x8c/0x158
> [   18.412418]  __driver_attach from bus_for_each_dev+0x74/0xc0
> [   18.412631]  bus_for_each_dev from bus_add_driver+0x154/0x1e8
> [   18.412844]  bus_add_driver from driver_register+0x88/0x11c
> [   18.413055]  driver_register from do_one_initcall+0x68/0x428
> [   18.413271]  do_one_initcall from kernel_init_freeable+0x18c/0x240
> [   18.413502]  kernel_init_freeable from kernel_init+0x14/0x144
> [   18.413721]  kernel_init from ret_from_fork+0x14/0x28
> [   18.414036] Exception stack(0xd0825fb0 to 0xd0825ff8)
> [   18.414523] 5fa0:                                     00000000 00000000 00000000 00000000
> [   18.414792] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   18.415032] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 
> 
> 
> 

Re: [REGRESSION] Re: [PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on` #forregzbot

[Thorsten Leemhuis]

On 18.09.22 20:59, Thorsten Leemhuis wrote:
> TWIMC: this mail is primarily send for documentation purposes and for
> regzbot, my Linux kernel regression tracking bot. These mails usually
> contain '#forregzbot' in the subject, to make them easy to spot and filter.
> 
> On 14.09.22 15:53, Guenter Roeck wrote:
>> On Thu, Sep 01, 2022 at 04:04:03PM +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 `tmreg_lock` fixes this.
>>>
>>> Fixes: 6a4d7234ae9a ("net: fec: ptp: avoid register access when ipg clock is disabled")
>>> Fixes: f79959220fa5 ("fec: Restart PPS after link state change")
>>> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Link: https://lore.kernel.org/all/20220827160922.642zlcd5foopozru@pengutronix.de/
>>> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
>>
>> For regzbot:
> 
> Thanks for the report. To be sure below issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced b353b241f1eb9b626
> #regzbot title net: fec: backtrace: BUG: sleeping function called from
> invalid context at drivers/clk/imx/clk-pllv3.c
> #regzbot ignore-activity
> #regzbot monitor:
> https://lore.kernel.org/all/20220912073106.2544207-1-bence98@sch.bme.hu/
> #regzbot monitor:
> https://lore.kernel.org/all/20220912070143.98153-1-francesco.dolcini@toradex.com/

#regzbot fixed-by: 01b825f997ac28f