[PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[Csókás, Bence]

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(-)

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;
 	}
-- 
2.34.1

Re: [PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[Andrew Lunn]

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

Re: [PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[Jakub Kicinski]

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?

Re: [PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[Russell King (Oracle)]

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...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[Andrew Lunn]

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

Re: [PATCH] net: sfp: Always call `sfp_sm_mod_remove()` on remove

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html