Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

[PATCH net v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

Posted by Johnathan Mantey 2 years, 1 month ago

This reverts commit 3780bb29311eccb7a1c9641032a112eed237f7e3.

The cited commit introduced unwanted behavior.

The intent for the commit was to be able to detect carrier loss/gain
for just the NIC connected to the BMC. The unwanted effect is a
carrier loss for auxiliary paths also causes the BMC to lose
carrier. The BMC never regains carrier despite the secondary NIC
regaining a link.

This change, when merged, needs to be backported to stable kernels.
5.4-stable, 5.10-stable, 5.15-stable, 6.1-stable, 6.5-stable

Fixes: 3780bb29311e ("ncsi: Propagate carrier gain/loss events to the NCSI controller")
CC: stable@vger.kernel.org
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
---
 net/ncsi/ncsi-aen.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index f8854bff286c..62fb1031763d 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -89,11 +89,6 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
 	if ((had_link == has_link) || chained)
 		return 0;
 
-	if (had_link)
-		netif_carrier_off(ndp->ndev.dev);
-	else
-		netif_carrier_on(ndp->ndev.dev);
-
 	if (!ndp->multi_package && !nc->package->multi_channel) {
 		if (had_link) {
 			ndp->flags |= NCSI_DEV_RESHUFFLE;
-- 
2.41.0

Re: [PATCH net v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

Posted by patchwork-bot+netdevbpf@kernel.org 2 years, 1 month ago

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 13 Nov 2023 08:30:29 -0800 you wrote:
> This reverts commit 3780bb29311eccb7a1c9641032a112eed237f7e3.
> 
> The cited commit introduced unwanted behavior.
> 
> The intent for the commit was to be able to detect carrier loss/gain
> for just the NIC connected to the BMC. The unwanted effect is a
> carrier loss for auxiliary paths also causes the BMC to lose
> carrier. The BMC never regains carrier despite the secondary NIC
> regaining a link.
> 
> [...]

Here is the summary with links:
  - [net,v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller
    https://git.kernel.org/netdev/net/c/9e2e7efbbbff

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

Re: [PATCH net v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

Posted by Simon Horman 2 years, 1 month ago

On Mon, Nov 13, 2023 at 08:30:29AM -0800, Johnathan Mantey wrote:
> This reverts commit 3780bb29311eccb7a1c9641032a112eed237f7e3.
> 
> The cited commit introduced unwanted behavior.
> 
> The intent for the commit was to be able to detect carrier loss/gain
> for just the NIC connected to the BMC. The unwanted effect is a
> carrier loss for auxiliary paths also causes the BMC to lose
> carrier. The BMC never regains carrier despite the secondary NIC
> regaining a link.
> 
> This change, when merged, needs to be backported to stable kernels.
> 5.4-stable, 5.10-stable, 5.15-stable, 6.1-stable, 6.5-stable
> 
> Fixes: 3780bb29311e ("ncsi: Propagate carrier gain/loss events to the NCSI controller")
> CC: stable@vger.kernel.org
> Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>

Hi Jonathan,

thanks for addressing my feedback on v2.

So far as addressing a regression goes, this looks good to me.
But I do wonder what can be done about the issue that
the cited commit was intended to address: will this patch regress things
on that front?

...

Re: [PATCH net v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

Posted by Johnathan Mantey 2 years, 1 month ago

Simon Horman <horms@kernel.org> writes:

> On Mon, Nov 13, 2023 at 08:30:29AM -0800, Johnathan Mantey 
> wrote:
>> This reverts commit 3780bb29311eccb7a1c9641032a112eed237f7e3.
>> 
>> The cited commit introduced unwanted behavior.
>> 
>> The intent for the commit was to be able to detect carrier 
>> loss/gain
>> for just the NIC connected to the BMC. The unwanted effect is a
>> carrier loss for auxiliary paths also causes the BMC to lose
>> carrier. The BMC never regains carrier despite the secondary 
>> NIC
>> regaining a link.
>> 
>> This change, when merged, needs to be backported to stable 
>> kernels.
>> 5.4-stable, 5.10-stable, 5.15-stable, 6.1-stable, 6.5-stable
>> 
>> Fixes: 3780bb29311e ("ncsi: Propagate carrier gain/loss events 
>> to the NCSI controller")
>> CC: stable@vger.kernel.org
>> Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
>
> Hi Jonathan,
>
> thanks for addressing my feedback on v2.
>
> So far as addressing a regression goes, this looks good to me.
> But I do wonder what can be done about the issue that
> the cited commit was intended to address: will this patch 
> regress things
> on that front?

Unfortunately the original issue will reoccur. I'm not sure which 
behavior is worse. What's been present for the lifespan of the 
ncsi driver, or this new issue I've introduced. In both instances 
a cable unplug causes undesirable behavior. I'm going to 
investigate solving this for Intel's specific use case ATM. NCSI 
has numerous modes in which it can be configured. I don't have a 
good feel for how to generalize the code given the side effect 
introduced by my change.

>
> ...

-- 
Johnathan Mantey

Re: [PATCH net v3] Revert ncsi: Propagate carrier gain/loss events to the NCSI controller

Posted by Simon Horman 2 years, 1 month ago

On Tue, Nov 14, 2023 at 12:20:52PM -0800, Johnathan Mantey wrote:
> 
> Simon Horman <horms@kernel.org> writes:
> 
> > On Mon, Nov 13, 2023 at 08:30:29AM -0800, Johnathan Mantey wrote:
> > > This reverts commit 3780bb29311eccb7a1c9641032a112eed237f7e3.
> > > 
> > > The cited commit introduced unwanted behavior.
> > > 
> > > The intent for the commit was to be able to detect carrier loss/gain
> > > for just the NIC connected to the BMC. The unwanted effect is a
> > > carrier loss for auxiliary paths also causes the BMC to lose
> > > carrier. The BMC never regains carrier despite the secondary NIC
> > > regaining a link.
> > > 
> > > This change, when merged, needs to be backported to stable kernels.
> > > 5.4-stable, 5.10-stable, 5.15-stable, 6.1-stable, 6.5-stable
> > > 
> > > Fixes: 3780bb29311e ("ncsi: Propagate carrier gain/loss events to
> > > the NCSI controller")
> > > CC: stable@vger.kernel.org
> > > Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
> > 
> > Hi Jonathan,
> > 
> > thanks for addressing my feedback on v2.
> > 
> > So far as addressing a regression goes, this looks good to me.
> > But I do wonder what can be done about the issue that
> > the cited commit was intended to address: will this patch regress things
> > on that front?
> 
> Unfortunately the original issue will reoccur. I'm not sure which behavior
> is worse. What's been present for the lifespan of the ncsi driver, or this
> new issue I've introduced. In both instances a cable unplug causes
> undesirable behavior. I'm going to investigate solving this for Intel's
> specific use case ATM. NCSI has numerous modes in which it can be
> configured. I don't have a good feel for how to generalize the code given
> the side effect introduced by my change.

Thanks Jonathan,

I agree that is a bit of a conundrum without a working fix available.
I would lean towards the old bug being somehow better than the new one -
better the devil you know than the one you don't.

So, FWIIW, from a pragmatic pov I'm happy with this patch.

Reviewed-by: Simon Horman <horms@kernel.org>