Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Krzysztof Halasa <khc@pm.waw.pl>
To:     Denis Du <dudenis2000@yahoo.ca>
Cc:     David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Carrier detect ok, don't turn off negotiation
References: <751079597.1884905.1516121905374.ref@mail.yahoo.com>
        <751079597.1884905.1516121905374@mail.yahoo.com>
        <20180122.152513.1108868799788445512.davem@davemloft.net>
        <998451043.3408644.1516727290310@mail.yahoo.com>
Date:   Sun, 28 Jan 2018 15:24:16 +0100
In-Reply-To: <998451043.3408644.1516727290310@mail.yahoo.com> (Denis Du's
        message of "Tue, 23 Jan 2018 17:08:10 +0000 (UTC)")
Message-ID: <m3r2qavz73.fsf@pm.waw.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Denis Du <dudenis2000@yahoo.ca> writes:

>>From the above code, I can get that only Carrier have some change, it
> will restart the protocol by hdlc_proto_start(dev);and thus the timer,
> the previous timer expired due to protocol fail.
>
> If carrier keep no change by if (hdlc->carrier == on)
>         goto carrier_exit; /* no change in DCD line level */It will do
> nothing, not start any new protocol and thus the timer.

Sorry about being late, just returned home and am trying to get all the
backlogs under control.

I remember the PPP standard is a bit cloudy about the possible issue,
but the latter indeed exists (the PPP state machine was written directly
to STD-51). There is related (more visible in practice, though we aren't
affected) issue of "active" vs "passive" mode (hdlc_ppp.c is "active",
and two "passives" wouldn't negotiate at all).

Anyway the problem is real (though not very visible in practice,
especially on relatively modern links rather than 300 or 1200 bps dialup
connections) and should be fixed. Looking at the patch, my first
impression is it makes the code differ from STD-51 a little bit.
On the other hand, perhaps applying it as is and forgetting about the
issue is the way to go.

Ideally, I think the negotiation failure should end up (optionally, in
addition to the current behavior) in some configurable sleep, then
the negotiation should restart. If it's worth the effort at this point,
I don't know.

Perhaps I could look at this later, but no promises (this requires
pulling on and setting up some legacy hardware).

Anyway, since the patch is safe and can solve an existing problem:

Acked-by: Krzysztof Halasa <khc@pm.waw.pl>
-- 
Krzysztof Halasa