From: "Russell King (Oracle)" <linux@armlinux•org.uk>
To: Jijie Shao <shaojijie@huawei•com>
Cc: Andrew Lunn <andrew@lunn•ch>,
f.fainelli@gmail•com, davem@davemloft•net, edumazet@google•com,
hkallweit1@gmail•com, kuba@kernel•org, netdev@vger•kernel.org,
pabeni@redhat•com,
"shenjian15@huawei•com" <shenjian15@huawei•com>,
"liuyonglong@huawei•com" <liuyonglong@huawei•com>,
wangjie125@huawei•com, chenhao418@huawei•com,
Hao Lan <lanhao@huawei•com>,
"wangpeiyang1@huawei•com" <wangpeiyang1@huawei•com>
Subject: Re: [PATCH net-next] net: phy: avoid kernel warning dump when stopping an errored PHY
Date: Tue, 5 Sep 2023 16:24:57 +0100 [thread overview]
Message-ID: <ZPdISTBrrX345RCz@shell.armlinux.org.uk> (raw)
In-Reply-To: <ZPcxnHjDJIMe3xt5@shell.armlinux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]
On Tue, Sep 05, 2023 at 02:48:13PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 05, 2023 at 04:49:31PM +0800, Jijie Shao wrote:
> > We note there are several times lock during phy_state_machine(). The first
> > is to handle phydev state. It's noting that a competition of phydev lock
> > happend again if phy_check_link_status() returns an error. Why we don't
> > held lock until changing state to PHY_ERROR if phy_check_link_status()
> > returns an error?
>
> You are quite correct that isn't very good. We can easily get rid of
> some of this mess, but I don't think all which leaves it still open to
> the race you describe.
>
> The problem is phy_suspend().
>
> First, it calls phy_ethtool_get_wol() which takes the phydev lock. This
> can be dealt with if we save the state at probe time, and then update
> the state when phy_ethtool_set_wol() is called.
>
> Second, phy_suspend() calls ->suspend without holding the phydev lock,
> and holding the lock while calling that may not be safe. Having had a
> brief look over the implementations (but not delving into any PTP
> function they may call) does seem to suggest that shouldn't be a big
> problem, but I don't know whether holding the phydev lock while calling
> PTP functions is likely to cause issues.
>
> However, looking at that has lead me to the conclusion that there is a
> lot of duplication of WoL condition testing. phy_suspend() already
> avoids calling ->suspend() if either phy_ethtool_get_wol() indicates
> that WoL is enabled, or if the netdev says WoL is enabled.
>
> Many of the ->suspend() implementations, for example,
> lan88xx_suspend(), dp83822_suspend(), etc test some kind of flag to
> determine whether WoL is enabled and thus seem to be re-implementing
> what phy_suspend() is already doing. I think there is scope to delete
> this code from several drivers.
>
> The easy bits are the patches I've attached to this email. These
> won't on their own be sufficient to close the race you've identified
> due to the phy_suspend() issue, but are the best we can do until we
> can work out what to do about that.
Having looked deeper at this, I think there may be a solution. See
these follow-on patches.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
[-- Attachment #2: 0005-net-phy-move-phy_suspend-to-end-of-phy_state_machine.patch --]
[-- Type: text/x-diff, Size: 1430 bytes --]
From: "Russell King (Oracle)" <rmk+kernel@armlinux•org.uk>
To: Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>
Bcc: linux@mail•armlinux.org.uk
Subject: [PATCH net-next 5/8] net: phy: move phy_suspend() to end of
phy_state_machine()
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
Move the call to phy_suspend() to the end of phy_state_machine() after
we release the lock.
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux•org.uk>
---
drivers/net/phy/phy.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2324544aae0d..010e54d7ccb2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1494,15 +1494,11 @@ void phy_state_machine(struct work_struct *work)
func = &_phy_start_aneg;
}
- mutex_unlock(&phydev->lock);
-
- if (do_suspend)
- phy_suspend(phydev);
-
- if (err == -ENODEV)
+ if (err == -ENODEV) {
+ mutex_unlock(&phydev->lock);
return;
+ }
- mutex_lock(&phydev->lock);
if (err < 0)
phy_error_precise(phydev, func, err);
@@ -1519,6 +1515,9 @@ void phy_state_machine(struct work_struct *work)
if (phy_polling_mode(phydev) && phy_is_started(phydev))
phy_queue_state_machine(phydev, PHY_STATE_TIME);
mutex_unlock(&phydev->lock);
+
+ if (do_suspend)
+ phy_suspend(phydev);
}
/**
--
2.30.2
[-- Attachment #3: 0006-net-phy-move-phy_state_machine.patch --]
[-- Type: text/x-diff, Size: 5140 bytes --]
From: "Russell King (Oracle)" <rmk+kernel@armlinux•org.uk>
To: Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>
Bcc: linux@mail•armlinux.org.uk
Subject: [PATCH net-next 6/8] net: phy: move phy_state_machine()
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
Move phy_state_machine() before phy_stop() to avoid subsequent patches
introducing forward references.
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux•org.uk>
---
drivers/net/phy/phy.c | 152 +++++++++++++++++++++---------------------
1 file changed, 76 insertions(+), 76 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 010e54d7ccb2..508a5434684a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1353,82 +1353,6 @@ void phy_free_interrupt(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_free_interrupt);
-/**
- * phy_stop - Bring down the PHY link, and stop checking the status
- * @phydev: target phy_device struct
- */
-void phy_stop(struct phy_device *phydev)
-{
- struct net_device *dev = phydev->attached_dev;
- enum phy_state old_state;
-
- if (!phy_is_started(phydev) && phydev->state != PHY_DOWN &&
- phydev->state != PHY_ERROR) {
- WARN(1, "called from state %s\n",
- phy_state_to_str(phydev->state));
- return;
- }
-
- mutex_lock(&phydev->lock);
- old_state = phydev->state;
-
- if (phydev->state == PHY_CABLETEST) {
- phy_abort_cable_test(phydev);
- netif_testing_off(dev);
- }
-
- if (phydev->sfp_bus)
- sfp_upstream_stop(phydev->sfp_bus);
-
- phydev->state = PHY_HALTED;
- phy_process_state_change(phydev, old_state);
-
- mutex_unlock(&phydev->lock);
-
- phy_state_machine(&phydev->state_queue.work);
- phy_stop_machine(phydev);
-
- /* Cannot call flush_scheduled_work() here as desired because
- * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
- * will not reenable interrupts.
- */
-}
-EXPORT_SYMBOL(phy_stop);
-
-/**
- * phy_start - start or restart a PHY device
- * @phydev: target phy_device struct
- *
- * Description: Indicates the attached device's readiness to
- * handle PHY-related work. Used during startup to start the
- * PHY, and after a call to phy_stop() to resume operation.
- * Also used to indicate the MDIO bus has cleared an error
- * condition.
- */
-void phy_start(struct phy_device *phydev)
-{
- mutex_lock(&phydev->lock);
-
- if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
- WARN(1, "called from state %s\n",
- phy_state_to_str(phydev->state));
- goto out;
- }
-
- if (phydev->sfp_bus)
- sfp_upstream_start(phydev->sfp_bus);
-
- /* if phy was suspended, bring the physical link up again */
- __phy_resume(phydev);
-
- phydev->state = PHY_UP;
-
- phy_start_machine(phydev);
-out:
- mutex_unlock(&phydev->lock);
-}
-EXPORT_SYMBOL(phy_start);
-
/**
* phy_state_machine - Handle the state machine
* @work: work_struct that describes the work to be done
@@ -1520,6 +1444,82 @@ void phy_state_machine(struct work_struct *work)
phy_suspend(phydev);
}
+/**
+ * phy_stop - Bring down the PHY link, and stop checking the status
+ * @phydev: target phy_device struct
+ */
+void phy_stop(struct phy_device *phydev)
+{
+ struct net_device *dev = phydev->attached_dev;
+ enum phy_state old_state;
+
+ if (!phy_is_started(phydev) && phydev->state != PHY_DOWN &&
+ phydev->state != PHY_ERROR) {
+ WARN(1, "called from state %s\n",
+ phy_state_to_str(phydev->state));
+ return;
+ }
+
+ mutex_lock(&phydev->lock);
+ old_state = phydev->state;
+
+ if (phydev->state == PHY_CABLETEST) {
+ phy_abort_cable_test(phydev);
+ netif_testing_off(dev);
+ }
+
+ if (phydev->sfp_bus)
+ sfp_upstream_stop(phydev->sfp_bus);
+
+ phydev->state = PHY_HALTED;
+ phy_process_state_change(phydev, old_state);
+
+ mutex_unlock(&phydev->lock);
+
+ phy_state_machine(&phydev->state_queue.work);
+ phy_stop_machine(phydev);
+
+ /* Cannot call flush_scheduled_work() here as desired because
+ * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
+ * will not reenable interrupts.
+ */
+}
+EXPORT_SYMBOL(phy_stop);
+
+/**
+ * phy_start - start or restart a PHY device
+ * @phydev: target phy_device struct
+ *
+ * Description: Indicates the attached device's readiness to
+ * handle PHY-related work. Used during startup to start the
+ * PHY, and after a call to phy_stop() to resume operation.
+ * Also used to indicate the MDIO bus has cleared an error
+ * condition.
+ */
+void phy_start(struct phy_device *phydev)
+{
+ mutex_lock(&phydev->lock);
+
+ if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
+ WARN(1, "called from state %s\n",
+ phy_state_to_str(phydev->state));
+ goto out;
+ }
+
+ if (phydev->sfp_bus)
+ sfp_upstream_start(phydev->sfp_bus);
+
+ /* if phy was suspended, bring the physical link up again */
+ __phy_resume(phydev);
+
+ phydev->state = PHY_UP;
+
+ phy_start_machine(phydev);
+out:
+ mutex_unlock(&phydev->lock);
+}
+EXPORT_SYMBOL(phy_start);
+
/**
* phy_mac_interrupt - MAC says the link has changed
* @phydev: phy_device struct with changed link
--
2.30.2
[-- Attachment #4: 0007-net-phy-split-locked-and-unlocked-section-of-phy_sta.patch --]
[-- Type: text/x-diff, Size: 4220 bytes --]
From: "Russell King (Oracle)" <rmk+kernel@armlinux•org.uk>
To: Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>
Bcc: linux@mail•armlinux.org.uk
Subject: [PATCH net-next 7/8] net: phy: split locked and unlocked section of
phy_state_machine()
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
Split out the locked and unlocked sections of phy_state_machine() into
two separate functions which can be called inside the phydev lock and
outside the phydev lock as appropriate.
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux•org.uk>
---
drivers/net/phy/phy.c | 68 ++++++++++++++++++++++++++-----------------
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 508a5434684a..b63d9f9320d7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1353,33 +1353,27 @@ void phy_free_interrupt(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_free_interrupt);
-/**
- * phy_state_machine - Handle the state machine
- * @work: work_struct that describes the work to be done
- */
-void phy_state_machine(struct work_struct *work)
+enum phy_state_work {
+ PHY_STATE_WORK_NONE,
+ PHY_STATE_WORK_ANEG,
+ PHY_STATE_WORK_SUSPEND,
+};
+
+static enum phy_state_work _phy_state_machine(struct phy_device *phydev)
{
- struct delayed_work *dwork = to_delayed_work(work);
- struct phy_device *phydev =
- container_of(dwork, struct phy_device, state_queue);
+ enum phy_state_work state_work = PHY_STATE_WORK_NONE;
struct net_device *dev = phydev->attached_dev;
- bool needs_aneg = false, do_suspend = false;
- enum phy_state old_state;
+ enum phy_state old_state = phydev->state;
const void *func = NULL;
bool finished = false;
int err = 0;
- mutex_lock(&phydev->lock);
-
- old_state = phydev->state;
-
switch (phydev->state) {
case PHY_DOWN:
case PHY_READY:
break;
case PHY_UP:
- needs_aneg = true;
-
+ state_work = PHY_STATE_WORK_ANEG;
break;
case PHY_NOLINK:
case PHY_RUNNING:
@@ -1391,7 +1385,7 @@ void phy_state_machine(struct work_struct *work)
if (err) {
phy_abort_cable_test(phydev);
netif_testing_off(dev);
- needs_aneg = true;
+ state_work = PHY_STATE_WORK_ANEG;
phydev->state = PHY_UP;
break;
}
@@ -1399,7 +1393,7 @@ void phy_state_machine(struct work_struct *work)
if (finished) {
ethnl_cable_test_finished(phydev);
netif_testing_off(dev);
- needs_aneg = true;
+ state_work = PHY_STATE_WORK_ANEG;
phydev->state = PHY_UP;
}
break;
@@ -1409,19 +1403,17 @@ void phy_state_machine(struct work_struct *work)
phydev->link = 0;
phy_link_down(phydev);
}
- do_suspend = true;
+ state_work = PHY_STATE_WORK_SUSPEND;
break;
}
- if (needs_aneg) {
+ if (state_work == PHY_STATE_WORK_ANEG) {
err = _phy_start_aneg(phydev);
func = &_phy_start_aneg;
}
- if (err == -ENODEV) {
- mutex_unlock(&phydev->lock);
- return;
- }
+ if (err == -ENODEV)
+ return state_work;
if (err < 0)
phy_error_precise(phydev, func, err);
@@ -1438,12 +1430,36 @@ void phy_state_machine(struct work_struct *work)
*/
if (phy_polling_mode(phydev) && phy_is_started(phydev))
phy_queue_state_machine(phydev, PHY_STATE_TIME);
- mutex_unlock(&phydev->lock);
- if (do_suspend)
+ return state_work;
+}
+
+/* unlocked part of the PHY state machine */
+static void _phy_state_machine_post_work(struct phy_device *phydev,
+ enum phy_state_work state_work)
+{
+ if (state_work == PHY_STATE_WORK_SUSPEND)
phy_suspend(phydev);
}
+/**
+ * phy_state_machine - Handle the state machine
+ * @work: work_struct that describes the work to be done
+ */
+void phy_state_machine(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct phy_device *phydev =
+ container_of(dwork, struct phy_device, state_queue);
+ enum phy_state_work state_work;
+
+ mutex_lock(&phydev->lock);
+ state_work = _phy_state_machine(phydev);
+ mutex_unlock(&phydev->lock);
+
+ _phy_state_machine_post_work(phydev, state_work);
+}
+
/**
* phy_stop - Bring down the PHY link, and stop checking the status
* @phydev: target phy_device struct
--
2.30.2
[-- Attachment #5: 0008-net-phy-convert-phy_stop-to-use-split-state-machine.patch --]
[-- Type: text/x-diff, Size: 1498 bytes --]
From: "Russell King (Oracle)" <rmk+kernel@armlinux•org.uk>
To: Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>
Bcc: linux@mail•armlinux.org.uk
Subject: [PATCH net-next 8/8] net: phy: convert phy_stop() to use split state
machine
MIME-Version: 1.0
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
Convert phy_stop() to use the new locked-section and unlocked-section
parts of the PHY state machine.
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux•org.uk>
---
drivers/net/phy/phy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b63d9f9320d7..5a5156f9788b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1467,6 +1467,7 @@ void phy_state_machine(struct work_struct *work)
void phy_stop(struct phy_device *phydev)
{
struct net_device *dev = phydev->attached_dev;
+ enum phy_state_work state_work;
enum phy_state old_state;
if (!phy_is_started(phydev) && phydev->state != PHY_DOWN &&
@@ -1490,9 +1491,10 @@ void phy_stop(struct phy_device *phydev)
phydev->state = PHY_HALTED;
phy_process_state_change(phydev, old_state);
+ state_work = _phy_state_machine(phydev);
mutex_unlock(&phydev->lock);
- phy_state_machine(&phydev->state_queue.work);
+ _phy_state_machine_post_work(phydev, state_work);
phy_stop_machine(phydev);
/* Cannot call flush_scheduled_work() here as desired because
--
2.30.2
next prev parent reply other threads:[~2023-09-05 15:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 15:58 [PATCH net-next] net: phy: avoid kernel warning dump when stopping an errored PHY Russell King (Oracle)
2023-05-22 16:06 ` Russell King (Oracle)
2023-05-22 19:03 ` Florian Fainelli
2023-09-04 9:50 ` Jijie Shao
2023-09-04 13:43 ` Andrew Lunn
2023-09-05 8:49 ` Jijie Shao
2023-09-05 12:09 ` Andrew Lunn
2023-09-05 14:00 ` Russell King (Oracle)
2023-09-05 13:48 ` Russell King (Oracle)
2023-09-05 15:24 ` Russell King (Oracle) [this message]
2023-09-06 12:59 ` Andrew Lunn
2023-09-04 14:42 ` Russell King (Oracle)
2023-09-05 8:59 ` Jijie Shao
2023-05-24 7:30 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZPdISTBrrX345RCz@shell.armlinux.org.uk \
--to=linux@armlinux$(echo .)org.uk \
--cc=andrew@lunn$(echo .)ch \
--cc=chenhao418@huawei$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=f.fainelli@gmail$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=lanhao@huawei$(echo .)com \
--cc=liuyonglong@huawei$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=shaojijie@huawei$(echo .)com \
--cc=shenjian15@huawei$(echo .)com \
--cc=wangjie125@huawei$(echo .)com \
--cc=wangpeiyang1@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox