[PATCH net] bonding: fix mii_status when slave is down

[PATCH net] bonding: fix mii_status when slave is down

[Nicolas Dichtel]

netif_carrier_ok() doesn't check if the slave is up. Before the below
commit, netif_running() was also checked.

Fixes: 23a6037ce76c ("bonding: Remove support for use_carrier")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/net/bonding/bond_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e95e593cd12d..5abef8a3b775 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2120,7 +2120,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	/* check for initial state */
 	new_slave->link = BOND_LINK_NOCHANGE;
 	if (bond->params.miimon) {
-		if (netif_carrier_ok(slave_dev)) {
+		if (netif_running(slave_dev) && netif_carrier_ok(slave_dev)) {
 			if (bond->params.updelay) {
 				bond_set_slave_link_state(new_slave,
 							  BOND_LINK_BACK,
@@ -2665,7 +2665,8 @@ static int bond_miimon_inspect(struct bonding *bond)
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
-		link_state = netif_carrier_ok(slave->dev);
+		link_state = netif_running(slave->dev) &&
+			     netif_carrier_ok(slave->dev);
 
 		switch (slave->link) {
 		case BOND_LINK_UP:
-- 
2.47.1

Re: [PATCH net] bonding: fix mii_status when slave is down

[Andrew Lunn]

On Thu, Nov 06, 2025 at 07:02:52PM +0100, Nicolas Dichtel wrote:
> netif_carrier_ok() doesn't check if the slave is up. Before the below
> commit, netif_running() was also checked.

I assume you have a device which is reporting carrier, despite being
admin down? That is pretty unusual, and suggests its PHY handing is
broken. What device is it? You might want to also fix it.

	Andrew

Re: [PATCH net] bonding: fix mii_status when slave is down

[Nicolas Dichtel]

Le 07/11/2025 à 03:36, Andrew Lunn a écrit :
> On Thu, Nov 06, 2025 at 07:02:52PM +0100, Nicolas Dichtel wrote:
>> netif_carrier_ok() doesn't check if the slave is up. Before the below
>> commit, netif_running() was also checked.
> 
> I assume you have a device which is reporting carrier, despite being
> admin down? That is pretty unusual, and suggests its PHY handing is
> broken. What device is it? You might want to also fix it.
Yes, one slave is put down administratively. Before the mentioned commit, the
status was correctly reported; it's no longer the case.
It's a regression.

Thanks,
Nicolas

Re: [PATCH net] bonding: fix mii_status when slave is down

[Jay Vosburgh]

Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:

>netif_carrier_ok() doesn't check if the slave is up. Before the below
>commit, netif_running() was also checked.
>
>Fixes: 23a6037ce76c ("bonding: Remove support for use_carrier")
>Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Acked-by: Jay Vosburgh <jv@jvosburgh.net>


>---
> drivers/net/bonding/bond_main.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e95e593cd12d..5abef8a3b775 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2120,7 +2120,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 	/* check for initial state */
> 	new_slave->link = BOND_LINK_NOCHANGE;
> 	if (bond->params.miimon) {
>-		if (netif_carrier_ok(slave_dev)) {
>+		if (netif_running(slave_dev) && netif_carrier_ok(slave_dev)) {
> 			if (bond->params.updelay) {
> 				bond_set_slave_link_state(new_slave,
> 							  BOND_LINK_BACK,
>@@ -2665,7 +2665,8 @@ static int bond_miimon_inspect(struct bonding *bond)
> 	bond_for_each_slave_rcu(bond, slave, iter) {
> 		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
> 
>-		link_state = netif_carrier_ok(slave->dev);
>+		link_state = netif_running(slave->dev) &&
>+			     netif_carrier_ok(slave->dev);
> 
> 		switch (slave->link) {
> 		case BOND_LINK_UP:
>-- 
>2.47.1
>

Re: [PATCH net] bonding: fix mii_status when slave is down

[Andrew Lunn]

On Fri, Nov 07, 2025 at 09:10:58AM +0100, Nicolas Dichtel wrote:
> Le 07/11/2025 à 03:36, Andrew Lunn a écrit :
> > On Thu, Nov 06, 2025 at 07:02:52PM +0100, Nicolas Dichtel wrote:
> >> netif_carrier_ok() doesn't check if the slave is up. Before the below
> >> commit, netif_running() was also checked.
> > 
> > I assume you have a device which is reporting carrier, despite being
> > admin down? That is pretty unusual, and suggests its PHY handing is
> > broken. What device is it? You might want to also fix it.
> Yes, one slave is put down administratively. Before the mentioned commit, the
> status was correctly reported; it's no longer the case.
> It's a regression.

I agree with your fix, but i would also like to know more about the
interfaces you are testing on. We should probably fix that device as
well. What is it?

Things do get 'interesting' when there is a BMC also sharing the
link. Linux cannot tell the PHY to drop the carrier because that would
also disconnect the BMC, but there should be some separation between
netif_carrier_ok() returns and the state of the PHY.

	Andrew

Re: [PATCH net] bonding: fix mii_status when slave is down

[Nicolas Dichtel]

Le 07/11/2025 à 14:55, Andrew Lunn a écrit :
> On Fri, Nov 07, 2025 at 09:10:58AM +0100, Nicolas Dichtel wrote:
>> Le 07/11/2025 à 03:36, Andrew Lunn a écrit :
>>> On Thu, Nov 06, 2025 at 07:02:52PM +0100, Nicolas Dichtel wrote:
>>>> netif_carrier_ok() doesn't check if the slave is up. Before the below
>>>> commit, netif_running() was also checked.
>>>
>>> I assume you have a device which is reporting carrier, despite being
>>> admin down? That is pretty unusual, and suggests its PHY handing is
>>> broken. What device is it? You might want to also fix it.
>> Yes, one slave is put down administratively. Before the mentioned commit, the
>> status was correctly reported; it's no longer the case.
>> It's a regression.
> 
> I agree with your fix, but i would also like to know more about the
> interfaces you are testing on. We should probably fix that device as
> well. What is it?
There is no bug. It is manually put down by one of our internal tests.

Nicolas

Re: [PATCH net] bonding: fix mii_status when slave is down

[Andrew Lunn]

> > I agree with your fix, but i would also like to know more about the
> > interfaces you are testing on. We should probably fix that device as
> > well. What is it?
> There is no bug. It is manually put down by one of our internal tests.

Are you sure?

admin down should cause the link to be dropped. You want the local
device to drop the carrier so the remote device knows you are gone,
etc. Also, dropping the carrier saves power. For a typical 1G copper
PHY, that is 1W. For a data centre connection, it can be many watts.

	Andrew

Re: [PATCH net] bonding: fix mii_status when slave is down

[Nicolas Dichtel]

Le 07/11/2025 à 18:32, Andrew Lunn a écrit :
>>> I agree with your fix, but i would also like to know more about the
>>> interfaces you are testing on. We should probably fix that device as
>>> well. What is it?
>> There is no bug. It is manually put down by one of our internal tests.
> 
> Are you sure?
Hmm, ok ok.

> 
> admin down should cause the link to be dropped. You want the local
> device to drop the carrier so the remote device knows you are gone,
> etc. Also, dropping the carrier saves power. For a typical 1G copper
> PHY, that is 1W. For a data centre connection, it can be many watts.
It's a virtual interface, a tuntap.

Nicolas

Re: [PATCH net] bonding: fix mii_status when slave is down

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  6 Nov 2025 19:02:52 +0100 you wrote:
> netif_carrier_ok() doesn't check if the slave is up. Before the below
> commit, netif_running() was also checked.
> 
> Fixes: 23a6037ce76c ("bonding: Remove support for use_carrier")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  drivers/net/bonding/bond_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Here is the summary with links:
  - [net] bonding: fix mii_status when slave is down
    https://git.kernel.org/netdev/net/c/2554559aba88

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html