Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Heiner,

Am Mittwoch, 1. März 2023, 07:29:04 CET schrieb Heiner Kallweit:
> On 28.02.2023 14:34, Alexander Stein wrote:
> > Before putting the PHY into IEEE power down mode, disable IRQs to
> > prevent accessing the PHY once MDIO has already been shutdown.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > I get this backtrace when trying to put the system into 'mem' powersaving
> > state.
> 
> I would have expected the following commit to prevent this scenario:
> 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend")
> 
> Can you check whether phydev->irq_suspended gets set in
> mdio_bus_phy_suspend() in your case?

No, this is not getting set, because mac_managed_pm is true.

> Which MAC driver do you use with this PHY?

This platform uses drivers/net/ethernet/freescale/fec_main.c

Best regards,
Alexander

> > [   31.355468] ------------[ cut here ]------------
> > [   31.360089] WARNING: CPU: 1 PID: 77 at drivers/net/phy/phy.c:1183
> > phy_error+0x10/0x54 [   31.367932] Modules linked in: bluetooth 8021q
> > garp stp mrp llc snd_soc_tlv320aic32x4_spi hantro_vpu snd_soc_fsl_
> > asoc_card snd_soc_fsl_sai snd_soc_imx_audmux snd_soc_fsl_utils
> > snd_soc_tlv320aic32x4_i2c snd_soc_simple_card_utils i mx_pcm_dma
> > snd_soc_tlv320aic32x4 snd_soc_core v4l2_vp9 snd_pcm_dmaengine v4l2_h264
> > videobuf2_dma_contig v4l2_mem2mem> 
> >  videobuf2_memops videobuf2_v4l2 videobuf2_common snd_pcm crct10dif_ce
> >  governor_userspace snd_timer imx_bus snd cfg8> 
> > 0211 soundcore pwm_imx27 imx_sdma virt_dma qoriq_thermal pwm_beeper fuse
> > ipv6 [   31.372014] PM: suspend devices took 0.184 seconds
> > [   31.415246] CPU: 1 PID: 77 Comm: irq/39-0-0025 Not tainted
> > 6.2.0-next-20230228+ #1425 2e0329a68388c493d090f81d406 77fb8aeac52cf
> > [   31.415257] Hardware name: TQ-Systems GmbH i.MX8MQ TQMa8MQ on MBa8Mx
> > (DT) [   31.415261] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> > BTYPE=--) [   31.445168] pc : phy_error+0x10/0x54
> > [   31.448749] lr : dp83867_handle_interrupt+0x78/0x88
> > [   31.453633] sp : ffff80000a353cb0
> > [   31.456947] x29: ffff80000a353cb0 x28: 0000000000000000 x27:
> > 0000000000000000 [   31.464091] x26: 0000000000000000 x25:
> > ffff800008dbb408 x24: ffff800009885568 [   31.471235] x23:
> > ffff0000c0e4b860 x22: ffff0000c0e4b8dc x21: ffff0000c0a46d18 [  
> > 31.478380] x20: ffff8000098d18a8 x19: ffff0000c0a46800 x18:
> > 0000000000000007 [   31.485525] x17: 6f63657320313030 x16:
> > 2e30206465737061 x15: 6c65282064657465 [   31.492669] x14:
> > 6c706d6f6320736b x13: 2973646e6f636573 x12: 0000000000000000 [  
> > 31.499815] x11: ffff800009362180 x10: 0000000000000a80 x9 :
> > ffff80000a3537a0 [   31.506959] x8 : 0000000000000000 x7 :
> > 0000000000000930 x6 : ffff0000c1494700 [   31.514104] x5 :
> > 0000000000000000 x4 : 0000000000000000 x3 : ffff0000c0a3d480 [  
> > 31.521248] x2 : 0000000000000000 x1 : ffff0000c0f3d700 x0 :
> > ffff0000c0a46800 [   31.528393] Call trace:
> > [   31.530840]  phy_error+0x10/0x54
> > [   31.534071]  dp83867_handle_interrupt+0x78/0x88
> > [   31.538605]  phy_interrupt+0x98/0xd8
> > [   31.542183]  handle_nested_irq+0xcc/0x148
> > [   31.546199]  pca953x_irq_handler+0xc8/0x154
> > [   31.550389]  irq_thread_fn+0x28/0xa0
> > [   31.553966]  irq_thread+0xcc/0x180
> > [   31.557371]  kthread+0xf4/0xf8
> > [   31.560429]  ret_from_fork+0x10/0x20
> > [   31.564009] ---[ end trace 0000000000000000 ]---
> > 
> > $ ./scripts/faddr2line build_arm64/vmlinux
> > dp83867_handle_interrupt+0x78/0x88 dp83867_handle_interrupt+0x78/0x88:
> > dp83867_handle_interrupt at drivers/net/phy/dp83867.c:332
> > 
> >  drivers/net/phy/dp83867.c | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> > index 89cd821f1f46..ed7e3df7dfd1 100644
> > --- a/drivers/net/phy/dp83867.c
> > +++ b/drivers/net/phy/dp83867.c
> > @@ -693,6 +693,32 @@ static int dp83867_of_init(struct phy_device *phydev)
> > 
> >  }
> >  #endif /* CONFIG_OF_MDIO */
> > 
> > +static int dp83867_suspend(struct phy_device *phydev)
> > +{
> > +	/* Disable PHY Interrupts */
> > +	if (phy_interrupt_is_valid(phydev)) {
> > +		phydev->interrupts = PHY_INTERRUPT_DISABLED;
> > +		if (phydev->drv->config_intr)
> > +			phydev->drv->config_intr(phydev);
> > +	}
> > +
> > +	return genphy_suspend(phydev);
> > +}
> > +
> > +static int dp83867_resume(struct phy_device *phydev)
> > +{
> > +	genphy_resume(phydev);
> > +
> > +	/* Enable PHY Interrupts */
> > +	if (phy_interrupt_is_valid(phydev)) {
> > +		phydev->interrupts = PHY_INTERRUPT_ENABLED;
> > +		if (phydev->drv->config_intr)
> > +			phydev->drv->config_intr(phydev);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  static int dp83867_probe(struct phy_device *phydev)
> >  {
> >  
> >  	struct dp83867_private *dp83867;
> > 
> > @@ -968,8 +994,8 @@ static struct phy_driver dp83867_driver[] = {
> > 
> >  		.config_intr	= dp83867_config_intr,
> >  		.handle_interrupt = dp83867_handle_interrupt,
> > 
> > -		.suspend	= genphy_suspend,
> > -		.resume		= genphy_resume,
> > +		.suspend	= dp83867_suspend,
> > +		.resume		= dp83867_resume,
> > 
> >  		.link_change_notify = dp83867_link_change_notify,
> >  		.set_loopback	= dp83867_loopback,


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/