| Message ID | 20240206005928.15703-2-sinthu.raja@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CPSW: enable mac_managed_pm to fix mdio | expand |
On 2/6/24 6:29 AM, Sinthu Raja wrote: > From: Sinthu Raja <sinthu.raja@ti.com> > > The below commit introduced a WARN when phy state is not in the states: > PHY_HALTED, PHY_READY and PHY_UP. > commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") > > When cpsw_new resumes, there have port in PHY_NOLINK state, so the below > warning comes out. Set mac_managed_pm be true to tell mdio that the phy > resume/suspend is managed by the mac, to fix the following warning: > > WARNING: CPU: 0 PID: 965 at drivers/net/phy/phy_device.c:326 mdio_bus_phy_resume+0x140/0x144 > CPU: 0 PID: 965 Comm: sh Tainted: G O 6.1.46-g247b2535b2 #1 > Hardware name: Generic AM33XX (Flattened Device Tree) > unwind_backtrace from show_stack+0x18/0x1c > show_stack from dump_stack_lvl+0x24/0x2c > dump_stack_lvl from __warn+0x84/0x15c > __warn from warn_slowpath_fmt+0x1a8/0x1c8 > warn_slowpath_fmt from mdio_bus_phy_resume+0x140/0x144 > mdio_bus_phy_resume from dpm_run_callback+0x3c/0x140 > dpm_run_callback from device_resume+0xb8/0x2b8 > device_resume from dpm_resume+0x144/0x314 > dpm_resume from dpm_resume_end+0x14/0x20 > dpm_resume_end from suspend_devices_and_enter+0xd0/0x924 > suspend_devices_and_enter from pm_suspend+0x2e0/0x33c > pm_suspend from state_store+0x74/0xd0 > state_store from kernfs_fop_write_iter+0x104/0x1ec > kernfs_fop_write_iter from vfs_write+0x1b8/0x358 > vfs_write from ksys_write+0x78/0xf8 > ksys_write from ret_fast_syscall+0x0/0x54 > Exception stack(0xe094dfa8 to 0xe094dff0) > dfa0: 00000004 005c3fb8 00000001 005c3fb8 00000004 00000001 > dfc0: 00000004 005c3fb8 b6f6bba0 00000004 00000004 0059edb8 00000000 00000000 > dfe0: 00000004 bed918f0 b6f09bd3 b6e89a66 > > Cc: <stable@vger.kernel.org> # v6.0+ > Fixes: 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") In v1, you received a comment to add the fixes tag. The reference stmmac patch also points to this commit as Fixes tag. But as Paolo pointed out in v2, this is not the right fixes tag for your patch series. I did a git blame on few drivers where PHY is managed by MAC. These have Fixes: fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") which seems to be more appropriate, as this is the commit that introduced the 'mac_managed_pm' flag. I have Cc'ed Paolo in this reply. But in future, please take care of adding the people who provided review comments in To/Cc when sending reworked patch/series. > Signed-off-by: Sinthu Raja <sinthu.raja@ti.com> > --- > > Changes in V3: > - No Change > > Changes in V2: > - Add fixes tag. > > drivers/net/ethernet/ti/cpsw_new.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c > index 498c50c6d1a7..087dcb67505a 100644 > --- a/drivers/net/ethernet/ti/cpsw_new.c > +++ b/drivers/net/ethernet/ti/cpsw_new.c > @@ -773,6 +773,9 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv) > slave->slave_num); > return; > } > + > + phy->mac_managed_pm = true; > + > slave->phy = phy; > > phy_attached_info(slave->phy);
On Tue, Feb 6, 2024 at 11:31 AM Ravi Gunasekaran <r-gunasekaran@ti.com> wrote: > > > > On 2/6/24 6:29 AM, Sinthu Raja wrote: > > From: Sinthu Raja <sinthu.raja@ti.com> > > > > The below commit introduced a WARN when phy state is not in the states: > > PHY_HALTED, PHY_READY and PHY_UP. > > commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") > > > > When cpsw_new resumes, there have port in PHY_NOLINK state, so the below > > warning comes out. Set mac_managed_pm be true to tell mdio that the phy > > resume/suspend is managed by the mac, to fix the following warning: > > > > WARNING: CPU: 0 PID: 965 at drivers/net/phy/phy_device.c:326 mdio_bus_phy_resume+0x140/0x144 > > CPU: 0 PID: 965 Comm: sh Tainted: G O 6.1.46-g247b2535b2 #1 > > Hardware name: Generic AM33XX (Flattened Device Tree) > > unwind_backtrace from show_stack+0x18/0x1c > > show_stack from dump_stack_lvl+0x24/0x2c > > dump_stack_lvl from __warn+0x84/0x15c > > __warn from warn_slowpath_fmt+0x1a8/0x1c8 > > warn_slowpath_fmt from mdio_bus_phy_resume+0x140/0x144 > > mdio_bus_phy_resume from dpm_run_callback+0x3c/0x140 > > dpm_run_callback from device_resume+0xb8/0x2b8 > > device_resume from dpm_resume+0x144/0x314 > > dpm_resume from dpm_resume_end+0x14/0x20 > > dpm_resume_end from suspend_devices_and_enter+0xd0/0x924 > > suspend_devices_and_enter from pm_suspend+0x2e0/0x33c > > pm_suspend from state_store+0x74/0xd0 > > state_store from kernfs_fop_write_iter+0x104/0x1ec > > kernfs_fop_write_iter from vfs_write+0x1b8/0x358 > > vfs_write from ksys_write+0x78/0xf8 > > ksys_write from ret_fast_syscall+0x0/0x54 > > Exception stack(0xe094dfa8 to 0xe094dff0) > > dfa0: 00000004 005c3fb8 00000001 005c3fb8 00000004 00000001 > > dfc0: 00000004 005c3fb8 b6f6bba0 00000004 00000004 0059edb8 00000000 00000000 > > dfe0: 00000004 bed918f0 b6f09bd3 b6e89a66 > > > > Cc: <stable@vger.kernel.org> # v6.0+ > > Fixes: 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") > > In v1, you received a comment to add the fixes tag. The reference stmmac patch also points > to this commit as Fixes tag. But as Paolo pointed out in v2, this is not the right > fixes tag for your patch series. > > I did a git blame on few drivers where PHY is managed by MAC. These have > Fixes: fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") Thanks, Ravi for pointing this out. But the warning message was caused only after the below commit had been added. 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") With the below commit the warning didn't pop up. fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") That is the reason I have not changed the Fixes tag. Let's wait for Paolo's comment on this Fixes: fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") > which seems to be more appropriate, as this is the commit that introduced the > 'mac_managed_pm' flag. > > I have Cc'ed Paolo in this reply. But in future, please take care of adding the people > who provided review comments in To/Cc when sending reworked patch/series. Noted. Regards, Sinthu Raja > > > Signed-off-by: Sinthu Raja <sinthu.raja@ti.com> > > --- > > > > Changes in V3: > > - No Change > > > > Changes in V2: > > - Add fixes tag. > > > > drivers/net/ethernet/ti/cpsw_new.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c > > index 498c50c6d1a7..087dcb67505a 100644 > > --- a/drivers/net/ethernet/ti/cpsw_new.c > > +++ b/drivers/net/ethernet/ti/cpsw_new.c > > @@ -773,6 +773,9 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv) > > slave->slave_num); > > return; > > } > > + > > + phy->mac_managed_pm = true; > > + > > slave->phy = phy; > > > > phy_attached_info(slave->phy); > > -- > Regards, > Ravi -- With Regards Sinthu Raja
On Tue, 2024-02-06 at 12:49 +0530, Sinthu Raja M wrote: > On Tue, Feb 6, 2024 at 11:31 AM Ravi Gunasekaran <r-gunasekaran@ti.com> wrote: > > In v1, you received a comment to add the fixes tag. The reference stmmac patch also points > > to this commit as Fixes tag. But as Paolo pointed out in v2, this is not the right > > fixes tag for your patch series. > > > > I did a git blame on few drivers where PHY is managed by MAC. These have > > Fixes: fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") > > Thanks, Ravi for pointing this out. > But the warning message was caused only after the below commit had been added. > 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") > > With the below commit the warning didn't pop up. > fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM") > That is the reason I have not changed the Fixes tag. > > Let's wait for Paolo's comment on this Fixes: fba863b81604 ("net: phy: > make PHY PM ops a no-op if MAC driver manages PHY PM") The main point of fixes tag is helping stable teams to backport the fixes if and where relevant. In this case the issue addressed is arguably an inconsistent state, which may lead to later functional issue. The warning emitted by the phy core is just an head-up, but it does not look like the 'real' problem. An older kernel including fba863b81604 but not including 744d23c71af3 will likely be prone to the issue. TL;DR: I think it's better to include both fixes tag. I'll add the 2nd one while applying the patch, no need to repost. Cheers, Paolo
diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c index 498c50c6d1a7..087dcb67505a 100644 --- a/drivers/net/ethernet/ti/cpsw_new.c +++ b/drivers/net/ethernet/ti/cpsw_new.c @@ -773,6 +773,9 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv) slave->slave_num); return; } + + phy->mac_managed_pm = true; + slave->phy = phy; phy_attached_info(slave->phy);