| Message ID | 20230228133412.7662-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net: phy: dp83867: Disable IRQs on suspend | expand |
> +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); It seems odd going via phydev->drv inside the driver to call functions which are also inside the driver. Why do you not directly call dp83867_config_intr()? > +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); > + } Is there a race here? Say the PHY is in a fixed mode, not autoneg. Could it be, that as soon as you clear the power down bit in genphy_resume() it signals a link up interrupt? dp83867_config_intr() then acknowledged and clears that interrupt, before enabling the interrupt, so the link up event never gets passed to phylib? Maybe the order needs reversing here? Andrew
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? Which MAC driver do you use with this PHY? > [ 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,
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,
Hi Andrew, thanks for the response. Am Dienstag, 28. Februar 2023, 16:05:27 CET schrieb Andrew Lunn: > > +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); > > It seems odd going via phydev->drv inside the driver to call functions > which are also inside the driver. Why do you not directly call > dp83867_config_intr()? I was going the same way kszphy_suspend() (micrel.c) is doing. Maybe that was a bad example and it should be changed as well. There should be no reason to not call dp83867_config_intr directly. > > +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); > > + } > > Is there a race here? Say the PHY is in a fixed mode, not > autoneg. Could it be, that as soon as you clear the power down bit in > genphy_resume() it signals a link up interrupt? dp83867_config_intr() > then acknowledged and clears that interrupt, before enabling the > interrupt, so the link up event never gets passed to phylib? Maybe the > order needs reversing here? Yes, your explanation sounds reasonable, unfortunately I can't test this right now, as there is some other error regarding PMIC power domains. If your reasoning is true then micrel.c has the same issue. Best regards, Alexander
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,
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. [ 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(-)