* [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend
@ 2023-02-28 13:34 Alexander Stein
2023-02-28 15:05 ` Andrew Lunn
2023-03-01 6:29 ` Heiner Kallweit
0 siblings, 2 replies; 5+ messages in thread
From: Alexander Stein @ 2023-02-28 13:34 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Alexander Stein, netdev
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(-)
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,
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend
2023-02-28 13:34 [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend Alexander Stein
@ 2023-02-28 15:05 ` Andrew Lunn
2023-03-09 10:52 ` Alexander Stein
2023-03-01 6:29 ` Heiner Kallweit
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2023-02-28 15:05 UTC (permalink / raw)
To: Alexander Stein
Cc: Heiner Kallweit, Russell King, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
> +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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend
2023-02-28 13:34 [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend Alexander Stein
2023-02-28 15:05 ` Andrew Lunn
@ 2023-03-01 6:29 ` Heiner Kallweit
2023-03-09 10:34 ` Alexander Stein
1 sibling, 1 reply; 5+ messages in thread
From: Heiner Kallweit @ 2023-03-01 6:29 UTC (permalink / raw)
To: Alexander Stein, Andrew Lunn, Russell King, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Lukas Wunner
Cc: netdev
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend
2023-03-01 6:29 ` Heiner Kallweit
@ 2023-03-09 10:34 ` Alexander Stein
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Stein @ 2023-03-09 10:34 UTC (permalink / raw)
To: Andrew Lunn, Russell King, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Lukas Wunner, Heiner Kallweit
Cc: netdev
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend
2023-02-28 15:05 ` Andrew Lunn
@ 2023-03-09 10:52 ` Alexander Stein
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Stein @ 2023-03-09 10:52 UTC (permalink / raw)
To: Andrew Lunn
Cc: Heiner Kallweit, Russell King, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
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
--
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-09 10:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 13:34 [PATCH 1/1] net: phy: dp83867: Disable IRQs on suspend Alexander Stein
2023-02-28 15:05 ` Andrew Lunn
2023-03-09 10:52 ` Alexander Stein
2023-03-01 6:29 ` Heiner Kallweit
2023-03-09 10:34 ` Alexander Stein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).