2024-06-05 08:44:04

by Csókás Bence

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

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" <[email protected]>
---
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




2024-06-06 15:23:41

by Andrew Lunn

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

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" <[email protected]>

I was expecting Russell to review this. Maybe he missed it.

This looks O.K. to me:

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2024-06-06 15:28:43

by Jakub Kicinski

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

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?

2024-06-06 15:48:49

by Russell King (Oracle)

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

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" <[email protected]>
>
> 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!

2024-06-06 16:20:05

by Andrew Lunn

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

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 <[email protected]>
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

2024-06-07 01:10:43

by patchwork-bot+netdevbpf

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

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

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" <[email protected]>
>
> [...]

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