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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- 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 --]