Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Arnd Bergmann]

On Thu, Nov 5, 2020 at 8:34 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> This driver transports LAPB (X.25 link layer) frames over TTY links.
>
> I can safely say that this driver has no actual user because it was
> not working at all until:
> commit 8fdcabeac398 ("drivers/net/wan/x25_asy: Fix to make it work")
>
> The code in its current state still has problems:
>
> 1.
> The uses of "struct x25_asy" in x25_asy_unesc (when receiving) and in
> x25_asy_write_wakeup (when sending) are not protected by locks against
> x25_asy_change_mtu's changing of the transmitting/receiving buffers.
> Also, all "netif_running" checks in this driver are not protected by
> locks against the ndo_stop function.
>
> 2.
> The driver stops all TTY read/write when the netif is down.
> I think this is not right because this may cause the last outgoing frame
> before the netif goes down to be incompletely transmitted, and the first
> incoming frame after the netif goes up to be incompletely received.
>
> And there may also be other problems.
>
> I was planning to fix these problems but after recent discussions about
> deleting other old networking code, I think we may just delete this
> driver, too.
>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  Documentation/process/magic-number.rst        |   1 -
>  .../it_IT/process/magic-number.rst            |   1 -
>  .../zh_CN/process/magic-number.rst            |   1 -
>  arch/mips/configs/gpr_defconfig               |   1 -
>  arch/mips/configs/mtx1_defconfig              |   1 -
>  drivers/net/wan/Kconfig                       |  15 -
>  drivers/net/wan/Makefile                      |   1 -
>  drivers/net/wan/x25_asy.c                     | 836 ------------------
>  drivers/net/wan/x25_asy.h                     |  46 -
>  9 files changed, 903 deletions(-)
>  delete mode 100644 drivers/net/wan/x25_asy.c
>  delete mode 100644 drivers/net/wan/x25_asy.h

Adding Martin Schiller and Andrew Hendry, plus the linux-x25 mailing
list to Cc. When I last looked at the wan drivers, I think I concluded
that this should still be kept around, but I do not remember why.
OTOH if it was broken for a long time, that is a clear indication that
it was in fact unused.

Since you did the bugfix mentioned above, do you have an idea
when it could have last worked? I see it was originally merged in
linux-2.3.21, and Stephen Hemminger did a cleanup for
linux-2.6.0-rc3 that he apparently tested but also said "Not sure
if anyone ever uses this.".

Hopefully Martin or Andrew can provide a definite Ack or Nack on this.

      Arnd

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Xie He]

On Thu, Nov 5, 2020 at 7:07 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> Adding Martin Schiller and Andrew Hendry, plus the linux-x25 mailing
> list to Cc.

The linux-x25 mail list has stopped working for a long time.

> When I last looked at the wan drivers, I think I concluded
> that this should still be kept around, but I do not remember why.

I think we only decided that X.25 as a whole was still needed. We
didn't decide that this particular driver was necessary.

> Since you did the bugfix mentioned above, do you have an idea
> when it could have last worked? I see it was originally merged in
> linux-2.3.21, and Stephen Hemminger did a cleanup for
> linux-2.6.0-rc3 that he apparently tested but also said "Not sure
> if anyone ever uses this.".

I think this driver never worked. Looking at the original code in
Linux 2.1.31, it already has the problems I fixed in commit
8fdcabeac398.

I guess when people (or bots) say they "tested", they have not
necessarily used this driver to actually try transporting data. They
may just have tested open/close, etc. to confirm that the particular
problem/issue they saw had been fixed.

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Arnd Bergmann]

On Thu, Nov 5, 2020 at 9:06 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Thu, Nov 5, 2020 at 7:07 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > Adding Martin Schiller and Andrew Hendry, plus the linux-x25 mailing
> > list to Cc.
>
> The linux-x25 mail list has stopped working for a long time.
>
> > When I last looked at the wan drivers, I think I concluded
> > that this should still be kept around, but I do not remember why.
>
> I think we only decided that X.25 as a whole was still needed. We
> didn't decide that this particular driver was necessary.

Right, I agree that would be the most likely case given the
state of the driver.

> > Since you did the bugfix mentioned above, do you have an idea
> > when it could have last worked? I see it was originally merged in
> > linux-2.3.21, and Stephen Hemminger did a cleanup for
> > linux-2.6.0-rc3 that he apparently tested but also said "Not sure
> > if anyone ever uses this.".
>
> I think this driver never worked. Looking at the original code in
> Linux 2.1.31, it already has the problems I fixed in commit
> 8fdcabeac398.
>
> I guess when people (or bots) say they "tested", they have not
> necessarily used this driver to actually try transporting data. They
> may just have tested open/close, etc. to confirm that the particular
> problem/issue they saw had been fixed.

It didn't sound like that from the commit message, but it could be.
For reference:
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit?id=aa2b4427c355acaf86d2c7e6faea3472005e3cff

     Arnd

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Xie He]

On Thu, Nov 5, 2020 at 12:40 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> > I think this driver never worked. Looking at the original code in
> > Linux 2.1.31, it already has the problems I fixed in commit
> > 8fdcabeac398.
> >
> > I guess when people (or bots) say they "tested", they have not
> > necessarily used this driver to actually try transporting data. They
> > may just have tested open/close, etc. to confirm that the particular
> > problem/issue they saw had been fixed.
>
> It didn't sound like that from the commit message, but it could be.
> For reference:
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit?id=aa2b4427c355acaf86d2c7e6faea3472005e3cff

I see. The author of this commit said "I tested by bringing up an x.25
async line using a modified version of slattach." Maybe he only
switched TTY ports from the N_TTY line discipline to the N_X25 line
discipline. To actually test transporting data, we need to either use
AF_PACKET sockets, or use AF_X25 sockets with the X.25 routing table
properly set up. The author of this commit didn't clearly indicate
that he did these.

RE: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[David Laight]

From: Xie He
> Sent: 05 November 2020 22:47
> 
> On Thu, Nov 5, 2020 at 12:40 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > > I think this driver never worked. Looking at the original code in
> > > Linux 2.1.31, it already has the problems I fixed in commit
> > > 8fdcabeac398.
> > >
> > > I guess when people (or bots) say they "tested", they have not
> > > necessarily used this driver to actually try transporting data. They
> > > may just have tested open/close, etc. to confirm that the particular
> > > problem/issue they saw had been fixed.
> >
> > It didn't sound like that from the commit message, but it could be.
> > For reference:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit?id=aa2b4427c355acaf86d2c7e6fae
> a3472005e3cff
> 
> I see. The author of this commit said "I tested by bringing up an x.25
> async line using a modified version of slattach." Maybe he only
> switched TTY ports from the N_TTY line discipline to the N_X25 line
> discipline. To actually test transporting data, we need to either use
> AF_PACKET sockets, or use AF_X25 sockets with the X.25 routing table
> properly set up. The author of this commit didn't clearly indicate
> that he did these.

Hmmm.... LAPB would expect to have an X.25 level 3 and maybe ISO
transport (class 0, 2 or 3) sat on top of it.
The ISO networking service would normally be absent (more likely to
be used with class 4 transport over ethernet - but still optional).
Nothing at all like SLIP.

You could use LAPB unnumbered (UI) frames to carry IP frames
(as SLIP does) - but that is really just using HDLC instead of
async. Doing that using async is just silly.

Looks like this code should have been removed for 2.2 :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Xie He]

On Fri, Nov 6, 2020 at 1:03 AM David Laight <David.Laight@aculab.com> wrote:
>
> Hmmm.... LAPB would expect to have an X.25 level 3 and maybe ISO
> transport (class 0, 2 or 3) sat on top of it.

I actually used AF_PACKET sockets to transport data directly over LAPB
and it worked.

LAPB doesn't need anything from layer 3. It just sees the layer 3
packets as a sequence of packets, numbers them and reliably transports
them in order. It doesn't read the internal contents of the packets.

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Martin Schiller]

On 2020-11-05 16:06, Arnd Bergmann wrote:
> ...
> Adding Martin Schiller and Andrew Hendry, plus the linux-x25 mailing
> list to Cc. When I last looked at the wan drivers, I think I concluded
> that this should still be kept around, but I do not remember why.
> OTOH if it was broken for a long time, that is a clear indication that
> it was in fact unused.

As Xie has already mentioned, the linux-x25 mailing list unfortunately
is broken for a long time. Maybe David could have a look at this.

I still use the X.25 subsystem together with the hdlc-x25 driver and as
I wrote some time ago this will continue for some time.

I'm also still contributing patches for it (even if only drop by drop).

But I have never used the x25_asy driver.

> ...
> 
> Hopefully Martin or Andrew can provide a definite Ack or Nack on this.
> 

ACK from my side, even if I'm a bit sorry about the work of Xie.

Acked-by: Martin Schiller <ms@dev.tdt.de>

Re: [PATCH net-next] net: x25_asy: Delete the x25_asy driver

[Jakub Kicinski]

On Fri, 06 Nov 2020 06:43:53 +0100 Martin Schiller wrote:
> On 2020-11-05 16:06, Arnd Bergmann wrote:
> > ...
> > Adding Martin Schiller and Andrew Hendry, plus the linux-x25 mailing
> > list to Cc. When I last looked at the wan drivers, I think I concluded
> > that this should still be kept around, but I do not remember why.
> > OTOH if it was broken for a long time, that is a clear indication that
> > it was in fact unused.  
> 
> As Xie has already mentioned, the linux-x25 mailing list unfortunately
> is broken for a long time. Maybe David could have a look at this.
> 
> I still use the X.25 subsystem together with the hdlc-x25 driver and as
> I wrote some time ago this will continue for some time.
> 
> I'm also still contributing patches for it (even if only drop by drop).
> 
> But I have never used the x25_asy driver.
> 
> > Hopefully Martin or Andrew can provide a definite Ack or Nack on this.
> >   
> 
> ACK from my side, even if I'm a bit sorry about the work of Xie.
> 
> Acked-by: Martin Schiller <ms@dev.tdt.de>

Applied to net-next, thanks!