| Message ID | 20221206094419.19478-1-rogerq@kernel.org |
|---|---|
| Headers | show |
| Series | net: ethernet: ti: am65-cpsw: Fix set channel operation | expand |
On Tue, Dec 06, 2022 at 11:44:13AM +0200, Roger Quadros wrote: > Hi, > > This contains a critical bug fix for the recently merged suspend/resume > support [1] that broke set channel operation. (ethtool -L eth0 tx <n>) > > As there were 2 dependent patches on top of the offending commit [1] > first revert them and then apply them back after the correct fix. Why did you chose revert and reapply almost same patch instead of simply fixing what is missing? Thanks > > [1] fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support") > > cheers, > -roger > > Changelog: > > v5: > - Change reset failure error code from -EBUSY to -ETIMEDOUT > > v4: > - move am65_cpsw_nuss_ndev_add_tx_napi() earlier to avoid declaration. > - print error and error out if soft RESET failed in > am65_cpsw_nuss_ndo_slave_open() > - move struct 'am65_cpsw_host *host' where 'common' is defined. > > v3: > - revert offending commit before applying the updated patch. > - drop optimization patch to be sent separately. > > v2: > - Fix build warning > drivers/net/ethernet/ti/am65-cpsw-nuss.c:562:13: warning: variable 'tmo' set but not used [-Wunused-but-set-variable] > > Roger Quadros (6): > Revert "net: ethernet: ti: am65-cpsw: Fix hardware switch mode on > suspend/resume" > Revert "net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after > suspend/resume" > Revert "net: ethernet: ti: am65-cpsw: Add suspend/resume support" > net: ethernet: ti: am65-cpsw: Add suspend/resume support > net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after > suspend/resume > net: ethernet: ti: am65-cpsw: Fix hardware switch mode on > suspend/resume > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 197 ++++++++++++----------- > 1 file changed, 105 insertions(+), 92 deletions(-) > > -- > 2.17.1 >
On Tue, Dec 06, 2022 at 12:15:17PM +0200, Roger Quadros wrote: > On 06/12/2022 12:05, Leon Romanovsky wrote: > > On Tue, Dec 06, 2022 at 11:44:13AM +0200, Roger Quadros wrote: > >> Hi, > >> > >> This contains a critical bug fix for the recently merged suspend/resume > >> support [1] that broke set channel operation. (ethtool -L eth0 tx <n>) > >> > >> As there were 2 dependent patches on top of the offending commit [1] > >> first revert them and then apply them back after the correct fix. > > > > Why did you chose revert and reapply almost same patch instead of simply > > fixing what is missing? > > v1 & 2 of this series were doing that but it was difficult to review. > This is because we are taking a different approach so we have to undo > most of the things done earlier. > > It was suggested during review that reverting and fresh patch was better. Thanks.
On Tue, 6 Dec 2022 11:44:17 +0200 Roger Quadros wrote: > ret = pm_runtime_resume_and_get(common->dev); > if (ret < 0) > return ret; > > + /* Idle MAC port */ > + cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE); > + cpsw_sl_wait_for_idle(port->slave.mac_sl, 100); > + cpsw_sl_ctl_reset(port->slave.mac_sl); > + > + /* soft reset MAC */ > + cpsw_sl_reg_write(port->slave.mac_sl, CPSW_SL_SOFT_RESET, 1); > + mdelay(1); > + reg = cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_SOFT_RESET); > + if (reg) { > + dev_err(common->dev, "soft RESET didn't complete\n"); > + return -ETIMEDOUT; Doesn't this function leak power management references on almost all error paths? Not really related to this set, tho.
Hello: This series was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 6 Dec 2022 11:44:13 +0200 you wrote: > Hi, > > This contains a critical bug fix for the recently merged suspend/resume > support [1] that broke set channel operation. (ethtool -L eth0 tx <n>) > > As there were 2 dependent patches on top of the offending commit [1] > first revert them and then apply them back after the correct fix. > > [...] Here is the summary with links: - [v5,net-next,1/6] Revert "net: ethernet: ti: am65-cpsw: Fix hardware switch mode on suspend/resume" https://git.kernel.org/netdev/net-next/c/1a352596722a - [v5,net-next,2/6] Revert "net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after suspend/resume" https://git.kernel.org/netdev/net-next/c/1bae8fa8c4f3 - [v5,net-next,3/6] Revert "net: ethernet: ti: am65-cpsw: Add suspend/resume support" https://git.kernel.org/netdev/net-next/c/1a014663e7dd - [v5,net-next,4/6] net: ethernet: ti: am65-cpsw: Add suspend/resume support https://git.kernel.org/netdev/net-next/c/24bc19b05f1f - [v5,net-next,5/6] net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after suspend/resume https://git.kernel.org/netdev/net-next/c/1581cd8b1174 - [v5,net-next,6/6] net: ethernet: ti: am65-cpsw: Fix hardware switch mode on suspend/resume https://git.kernel.org/netdev/net-next/c/020b232f79e9 You are awesome, thank you!
On 08/12/2022 06:21, Jakub Kicinski wrote: > On Tue, 6 Dec 2022 11:44:17 +0200 Roger Quadros wrote: >> ret = pm_runtime_resume_and_get(common->dev); >> if (ret < 0) >> return ret; >> >> + /* Idle MAC port */ >> + cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE); >> + cpsw_sl_wait_for_idle(port->slave.mac_sl, 100); >> + cpsw_sl_ctl_reset(port->slave.mac_sl); >> + >> + /* soft reset MAC */ >> + cpsw_sl_reg_write(port->slave.mac_sl, CPSW_SL_SOFT_RESET, 1); >> + mdelay(1); >> + reg = cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_SOFT_RESET); >> + if (reg) { >> + dev_err(common->dev, "soft RESET didn't complete\n"); >> + return -ETIMEDOUT; > > Doesn't this function leak power management references on almost all > error paths? Not really related to this set, tho. Oh yes it does. I'll send a separate fix. cheers, -roger
Hi, This contains a critical bug fix for the recently merged suspend/resume support [1] that broke set channel operation. (ethtool -L eth0 tx <n>) As there were 2 dependent patches on top of the offending commit [1] first revert them and then apply them back after the correct fix. [1] fd23df72f2be ("net: ethernet: ti: am65-cpsw: Add suspend/resume support") cheers, -roger Changelog: v5: - Change reset failure error code from -EBUSY to -ETIMEDOUT v4: - move am65_cpsw_nuss_ndev_add_tx_napi() earlier to avoid declaration. - print error and error out if soft RESET failed in am65_cpsw_nuss_ndo_slave_open() - move struct 'am65_cpsw_host *host' where 'common' is defined. v3: - revert offending commit before applying the updated patch. - drop optimization patch to be sent separately. v2: - Fix build warning drivers/net/ethernet/ti/am65-cpsw-nuss.c:562:13: warning: variable 'tmo' set but not used [-Wunused-but-set-variable] Roger Quadros (6): Revert "net: ethernet: ti: am65-cpsw: Fix hardware switch mode on suspend/resume" Revert "net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after suspend/resume" Revert "net: ethernet: ti: am65-cpsw: Add suspend/resume support" net: ethernet: ti: am65-cpsw: Add suspend/resume support net: ethernet: ti: am65-cpsw: retain PORT_VLAN_REG after suspend/resume net: ethernet: ti: am65-cpsw: Fix hardware switch mode on suspend/resume drivers/net/ethernet/ti/am65-cpsw-nuss.c | 197 ++++++++++++----------- 1 file changed, 105 insertions(+), 92 deletions(-)