Re: [PATCH 01/10] appletalk: remove localtalk and ppp support

From: Rodolfo Zitellini <rwz@xhero.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Arnd Bergmann <arnd@kernel.org>, Jakub Kicinski <kuba@kernel.org>,
	Netdev <netdev@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-wireless@vger.kernel.org,
	Johannes Berg <johannes@sipsolutions.net>,
	linux-wpan@vger.kernel.org,
	Michael Hennerich <michael.hennerich@analog.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	linux-kernel@vger.kernel.org, Doug Brown <doug@schmorgal.com>
Subject: Re: [PATCH 01/10] appletalk: remove localtalk and ppp support
Date: Tue, 10 Oct 2023 09:10:28 +0200	[thread overview]
Message-ID: <DE61EEA5-D560-40B6-8F4D-22F299AC61ED@xhero.org> (raw)
In-Reply-To: <3cb4bb96-1651-4179-9c32-507937282d7d@app.fastmail.com>

> Il giorno 9 ott 2023, alle ore 19:29, Arnd Bergmann <arnd@arndb.de> ha scritto:
> 
> On Mon, Oct 9, 2023, at 18:49, Rodolfo Zitellini wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>> 
>>> The last localtalk driver is gone now, and ppp support was never fully
>>> merged, so clean up the appletalk code by removing the obvious dead
>>> code paths.
>>> 
>>> Notably, this removes one of the two callers of the old .ndo_do_ioctl()
>>> callback that was abused for getting device addresses and is now
>>> only used in the ieee802154 subsystem, which still uses the same trick.
>>> 
>>> The include/uapi/linux/if_ltalk.h header might still be required
>>> for building userspace programs, but I made sure that debian code
>>> search and the netatalk upstream have no references it it, so it
>>> should be fine to remove.
>>> 
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> 
>> Hi!
>> I’ve been working on a new LocalTalk interface driver for the last 
>> couple months, do you think it would be possible to at least postpone 
>> the removal of LT a bit?
>> 
>> It is a driver for an open source device called TashTalk 
>> (https://github.com/lampmerchant/tashtalk), which runs on a PIC micro 
>> that does all the LT interfacing, and communicates back via serial to 
>> the host system. My driver is relatively simple and works very well 
>> with netatalk 2.2 (which is still maintained and still has support for 
>> AppleTalk). The driver is basically complete and trsted and I was 
>> preparing to submit a patch.
>> 
>> Still having LocalTalk in my view has many advantages for us 
>> enthusiasts that still want to bridge old machines to the current world 
>> without modifications, for example for printing on modern printers, 
>> netbooting, sharing files and even tcp/ip. All this basically works out 
>> of the box via the driver, Linux and available userspace tools 
>> (netatalk, macipgw).
>> 
>> The old ISA cards supported by COPS were basically unobtanium even 20 
>> years ago, but the solution of using a PIC and a serial port is very 
>> robust and much more furure-proof. We also already have a device that 
>> can interface a modern machine directly via USB to LocalTalk.
>> 
>> The development of the TashTalk has been also extensively discussed on 
>> thr 68KMLA forum 
>> (https://68kmla.org/bb/index.php?threads/modtashtalk-lt0-driver-for-linux.45031/)
>> 
>> I hope the decision to remove LocalTalk can be reconsidered at least 
>> for the time being so there is a chance to submit a new, modern device 
>> making use of this stack.
> 
> Nothing is decided, I'm just proposing my patch as a cleanup
> for now. It would be nice to still drop the ndo_do_ioctl function
> though, at least in some form. When your driver actually makes
> it into the kernel, you can find a different method of communicating
> the address between the socket interface and the device driver.

Yes I too think it is good to remove ndo_do_ioctl, I designed the TashTalk driver to be a drop-in replacement for COPS mostly for compatibility with netatalk 2.2. My plan was to propose it like this (so nothing else needed to be changed) and the propose some patches in the kernel part and userspace part (netatalk).

> I can see a few ways this could work out:
> 
> - add a custom callback pointer to struct atalk_iface to
>  get and set the address for phase1 probing instead of going
>  through the ioctl

This was my initial thought, at least for the moment, mostly to keep netatalk happy and make sure I don’t break other stuff that makes assumptions on how the address probing worked. There are other bits I would like to improve, for example tcpdump (which parses correctly appetalk packets!) is broken in the current implementation.

> - rewrite the probing logic in aarp.c more widely, and improve
>  the userspace interface in the process by introducing a netlink
>  interface

This is sorta the “second step” I was planning, I think the logic for probing could be redesigned and simplified (it also does not work 100% correctly), and it could be a good chance to improve the interface with netatalk too.

> - Move your entire driver into userspace and go to the kernel
>  using tun/tap. This has the added benefit of avoiding a lot
>  of the complexity of the tty line discipline code you have.

We had some discussion too if to just make the lt an userspace stack, I personally like how it is currently implemented because existing code can run basically without modification.

I would propose at this stage to change the TashTalk driver to remove ndo_do_ioctl and to use a custom callback, if this ok.

Many thanks,
Rodolfo