| Message ID | 20240605084251.63502-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e96b2933152fd87b6a41765b2f58b158fde855b6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sfp: Always call `sfp_sm_mod_remove()` on remove | expand |
On Wed, Jun 05, 2024 at 10:42:51AM +0200, Csókás, Bence wrote: > If the module is in SFP_MOD_ERROR, `sfp_sm_mod_remove()` will > not be run. As a consequence, `sfp_hwmon_remove()` is not getting > run either, leaving a stale `hwmon` device behind. `sfp_sm_mod_remove()` > itself checks `sfp->sm_mod_state` anyways, so this check was not > really needed in the first place. > > Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu> I was expecting Russell to review this. Maybe he missed it. This looks O.K. to me: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, 6 Jun 2024 17:21:45 +0200 Andrew Lunn wrote:
> I was expecting Russell to review this. Maybe he missed it.
While it's fresh in your mind - does it look like a fix to you?
From a quick look - we're failing to unregister a device?
On Thu, Jun 06, 2024 at 05:21:45PM +0200, Andrew Lunn wrote: > On Wed, Jun 05, 2024 at 10:42:51AM +0200, Csókás, Bence wrote: > > If the module is in SFP_MOD_ERROR, `sfp_sm_mod_remove()` will > > not be run. As a consequence, `sfp_hwmon_remove()` is not getting > > run either, leaving a stale `hwmon` device behind. `sfp_sm_mod_remove()` > > itself checks `sfp->sm_mod_state` anyways, so this check was not > > really needed in the first place. > > > > Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu> > > I was expecting Russell to review this. Maybe he missed it. I haven't missed it, I just haven't had the time to review and respond. Work stuff is still very busy. I know this has been going on for well over a month, but as it's partly coming from my employer and partly due to medical stuff taking hours out of my working week, there's nothing much I can do about it. I'm doing the best I can, but I know that I can't keep up with what people expect of me at the moment. I've had to tell Kory that I won't be able to review and test his patch series - it's just totally impossible for me to be near the hardware I need to test with his series _and_ have the time to do so. I feel bad about that, because it's addressing the issue I raised a number of years ago with PTP, yet I don't have the time to be involved in that right now. It's frustrating for me that I'm not able to do everything that I'd like to...
On Thu, Jun 06, 2024 at 08:28:30AM -0700, Jakub Kicinski wrote: > On Thu, 6 Jun 2024 17:21:45 +0200 Andrew Lunn wrote: > > I was expecting Russell to review this. Maybe he missed it. > > While it's fresh in your mind - does it look like a fix to you? > From a quick look - we're failing to unregister a device? Yes. A Fixes of: commit d2e816c0293fc263b3f168c14992a5f1a50d7593 Author: Russell King <rmk+kernel@armlinux.org.uk> Date: Sun Nov 10 14:06:28 2019 +0000 net: sfp: handle module remove outside state machine Removing a module resets the module state machine back to its initial state. Rather than explicitly handling this in every state, handle it early on outside of the state machine. is probably best, since that refactored to code. Andrew
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 5 Jun 2024 10:42:51 +0200 you wrote: > If the module is in SFP_MOD_ERROR, `sfp_sm_mod_remove()` will > not be run. As a consequence, `sfp_hwmon_remove()` is not getting > run either, leaving a stale `hwmon` device behind. `sfp_sm_mod_remove()` > itself checks `sfp->sm_mod_state` anyways, so this check was not > really needed in the first place. > > Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu> > > [...] Here is the summary with links: - net: sfp: Always call `sfp_sm_mod_remove()` on remove https://git.kernel.org/netdev/net/c/e96b2933152f You are awesome, thank you!
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index f75c9eb3958e..d999d9baadb2 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -2418,8 +2418,7 @@ static void sfp_sm_module(struct sfp *sfp, unsigned int event) /* Handle remove event globally, it resets this state machine */ if (event == SFP_E_REMOVE) { - if (sfp->sm_mod_state > SFP_MOD_PROBE) - sfp_sm_mod_remove(sfp); + sfp_sm_mod_remove(sfp); sfp_sm_mod_next(sfp, SFP_MOD_EMPTY, 0); return; }
If the module is in SFP_MOD_ERROR, `sfp_sm_mod_remove()` will not be run. As a consequence, `sfp_hwmon_remove()` is not getting run either, leaving a stale `hwmon` device behind. `sfp_sm_mod_remove()` itself checks `sfp->sm_mod_state` anyways, so this check was not really needed in the first place. Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu> --- drivers/net/phy/sfp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)