Re: [PATCH net-next 2/2] appletalk: tashtalk: Add LocalTalk line discipline driver for AppleTalk using a TashTalk adapter
From: Jiri Slaby
Date: Mon Aug 19 2024 - 05:56:18 EST
On 17. 08. 24, 11:33, Rodolfo Zitellini wrote:
--- /dev/null
+++ b/drivers/net/appletalk/tashtalk.c
@@ -0,0 +1,1003 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* tashtalk.c: TashTalk LocalTalk driver for Linux.
+ *
+ * Authors:
+ * Rodolfo Zitellini (twelvetone12)
+ *
+ * Derived from:
+ * - slip.c: A network driver outline for linux.
+ * written by Laurence Culhane and Fred N. van Kempen
+ *
+ * This software may be used and distributed according to the terms
+ * of the GNU General Public License, incorporated herein by reference.
+ *
+ */
+
+#include <linux/compat.h>
What is this used for?
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/version.h>
+
+#include <linux/uaccess.h>
+#include <linux/bitops.h>
+#include <linux/sched/signal.h>
+#include <linux/string.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/in.h>
+#include <linux/tty.h>
+#include <linux/errno.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/if_arp.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <linux/if_ltalk.h>
+#include <linux/atalk.h>
Are all those needed? I doubt that.
+#ifndef TASH_DEBUG
+#define TASH_DEBUG 0
+#endif
+static unsigned int tash_debug = TASH_DEBUG;
Can we use dyn dbg instead?
+/* Max number of channels
+ * override with insmod -otash_maxdev=nnn
+ */
+#define TASH_MAX_CHAN 32
+#define TT_MTU 605
+/* The buffer should be double since potentially
+ * all bytes inside are escaped.
+ */
+#define BUF_LEN (TT_MTU * 2 + 4)
+
+struct tashtalk {
+ int magic;
Where did you dig this driver from? Drop this.
+ struct tty_struct *tty; /* ptr to TTY structure */
+ struct net_device *dev; /* easy for intr handling */
+ spinlock_t lock;
+ wait_queue_head_t addr_wait;
+ struct work_struct tx_work; /* Flushes transmit buffer */
+
+ /* These are pointers to the malloc()ed frame buffers. */
+ unsigned char *rbuff; /* receiver buffer */
+ int rcount; /* received chars counter */
uint?
+ unsigned char *xbuff; /* transmitter buffer */
+ unsigned char *xhead; /* pointer to next byte to XMIT */
Switch to u8's.
+ int xleft; /* bytes left in XMIT queue */
+ int mtu;
+ int buffsize; /* Max buffers sizes */
So uint?
+ unsigned long flags; /* Flag values/ mode etc */
+ unsigned char mode; /* really not used */
+ pid_t pid;
Storing pid is usually wrong. So is here.
Many of the above is ancient material.
+ struct atalk_addr node_addr; /* Full node address */
+};
...
+static void tash_setbits(struct tashtalk *tt, unsigned char addr)
+{
+ unsigned char bits[33];
u8
+ unsigned int byte, pos;
+
+ /* 0, 255 and anything else are invalid */
+ if (addr == 0 || addr >= 255)
+ return;
+
+ memset(bits, 0, sizeof(bits));
+
+ /* in theory we can respond to many addresses */
+ byte = addr / 8 + 1; /* skip initial command byte */
+ pos = (addr % 8);
+
+ if (tash_debug)
+ netdev_dbg(tt->dev,
+ "Setting address %i (byte %i bit %i) for you.",
+ addr, byte - 1, pos);
+
+ bits[0] = TT_CMD_SET_NIDS;
+ bits[byte] = (1 << pos);
BIT()
+
+ set_bit(TTY_DO_WRITE_WAKEUP, &tt->tty->flags);
+ tt->tty->ops->write(tt->tty, bits, sizeof(bits));
+}
+
+static u16 tt_crc_ccitt_update(u16 crc, u8 data)
+{
+ data ^= (u8)(crc) & (u8)(0xFF);
+ data ^= data << 4;
+ return ((((u16)data << 8) | ((crc & 0xFF00) >> 8)) ^ (u8)(data >> 4) ^
+ ((u16)data << 3));
Is this real ccitt? Won't crc_ccitt_byte() work then?
+}
+
+static u16 tash_crc(const unsigned char *data, int len)
+{
+ u16 crc = 0xFFFF;
+
+ for (int i = 0; i < len; i++)
+ crc = tt_crc_ccitt_update(crc, data[i]);
+
+ return crc;
Or even crc_ccitt()?
+}
...
+/* Write out any remaining transmit buffer. Scheduled when tty is writable */
+static void tash_transmit_worker(struct work_struct *work)
+{
+ struct tashtalk *tt = container_of(work, struct tashtalk, tx_work);
+ int actual;
+
+ spin_lock_bh(&tt->lock);
+ /* First make sure we're connected. */
+ if (!tt->tty || tt->magic != TASH_MAGIC || !netif_running(tt->dev)) {
Can the former two happen?
+ spin_unlock_bh(&tt->lock);
+ return;
+ }
+
+ /* We always get here after all transmissions
+ * No more data?
+ */
+ if (tt->xleft <= 0) {
+ /* reset the flags for transmission
+ * and re-wake the netif queue
+ */
+ tt->dev->stats.tx_packets++;
+ clear_bit(TTY_DO_WRITE_WAKEUP, &tt->tty->flags);
+ spin_unlock_bh(&tt->lock);
+ netif_wake_queue(tt->dev);
+
+ return;
+ }
+
+ /* Send whatever is there to send
+ * This function will be called again if xleft <= 0
+ */
+ actual = tt->tty->ops->write(tt->tty, tt->xhead, tt->xleft);
returns ssize_t
+ tt->xleft -= actual;
+ tt->xhead += actual;
+
+ spin_unlock_bh(&tt->lock);
+}
+
+/* Called by the driver when there's room for more data.
+ * Schedule the transmit.
Is this a valid multiline comment in net/?
+ */
...
+static void tt_tx_timeout(struct net_device *dev, unsigned int txqueue)
+{
+ struct tashtalk *tt = netdev_priv(dev);
+
+ spin_lock(&tt->lock);
guard()?
+
+ if (netif_queue_stopped(dev)) {
+ if (!netif_running(dev) || !tt->tty)
+ goto out;
+ }
+out:
+ spin_unlock(&tt->lock);
+}
...
+/* Netdevice DOWN -> UP routine */
+
+static int tt_open(struct net_device *dev)
+{
+ struct tashtalk *tt = netdev_priv(dev);
+
+ if (!tt->tty) {
No lock?
+ netdev_err(dev, "TTY not open");
+ return -ENODEV;
+ }
+
+ tt->flags &= (1 << TT_FLAG_INUSE);
so clear_bit()?
+ netif_start_queue(dev);
+ return 0;
+}
+static void tashtalk_send_ctrl_packet(struct tashtalk *tt, unsigned char dst,
+ unsigned char src, unsigned char type)
+{
+ unsigned char cmd = TT_CMD_TX;
+ unsigned char buf[5];
u8
+ int actual;
+ u16 crc;
+
+ buf[LLAP_DST_POS] = dst;
+ buf[LLAP_SRC_POS] = src;
+ buf[LLAP_TYP_POS] = type;
+
+ crc = tash_crc(buf, 3);
+ buf[3] = ~(crc & 0xFF);
+ buf[4] = ~(crc >> 8);
+
+ actual = tt->tty->ops->write(tt->tty, &cmd, 1);
+ actual += tt->tty->ops->write(tt->tty, buf, sizeof(buf));
What is actual used for? And why is this not a single write (using
buf[6] instead).
+}
...
+static void tashtalk_receive_buf(struct tty_struct *tty,
+ const u8 *cp, const u8 *fp,
+ size_t count)
+{
+ struct tashtalk *tt = tty->disc_data;
+ int i;
+
+ if (!tt || tt->magic != TASH_MAGIC || !netif_running(tt->dev))
How can that happen?
+ return;
+
+ if (tash_debug)
+ netdev_dbg(tt->dev, "(1) TashTalk read %li", count);
+
+ print_hex_dump_bytes("Tash read: ", DUMP_PREFIX_NONE, cp, count);
+
+ if (!test_bit(TT_FLAG_INFRAME, &tt->flags)) {
+ tt->rcount = 0;
+ if (tash_debug)
+ netdev_dbg(tt->dev, "(2) TashTalk start new frame");
+ } else if (tash_debug) {
+ netdev_dbg(tt->dev, "(2) TashTalk continue frame");
+ }
+
+ set_bit(TT_FLAG_INFRAME, &tt->flags);
So test_and_set_bit()?
+
+ for (i = 0; i < count; i++) {
+ set_bit(TT_FLAG_INFRAME, &tt->flags);
Why again?
+
+ if (cp[i] == 0x00) {
+ set_bit(TT_FLAG_ESCAPE, &tt->flags);
+ continue;
+ }
+
+ if (test_and_clear_bit(TT_FLAG_ESCAPE, &tt->flags)) {
+ if (cp[i] == 0xFF) {
+ tt->rbuff[tt->rcount] = 0x00;
+ tt->rcount++;
+ } else {
+ tashtalk_manage_escape(tt, cp[i]);
+ }
+ } else {
+ tt->rbuff[tt->rcount] = cp[i];
+ tt->rcount++;
+ }
+ }
+
+ if (tash_debug)
+ netdev_dbg(tt->dev, "(5) Done read, pending frame=%i",
+ test_bit(TT_FLAG_INFRAME, &tt->flags));
+}
...
+static void tashtalk_close(struct tty_struct *tty)
+{
+ struct tashtalk *tt = tty->disc_data;
+
+ /* First make sure we're connected. */
+ if (!tt || tt->magic != TASH_MAGIC || tt->tty != tty)
How can these happen?
+ return;
+
+ spin_lock_bh(&tt->lock);
+ rcu_assign_pointer(tty->disc_data, NULL);
+ tt->tty = NULL;
+ spin_unlock_bh(&tt->lock);
+
+ synchronize_rcu();
+ flush_work(&tt->tx_work);
+
+ /* Flush network side */
+ unregister_netdev(tt->dev);
+ /* This will complete via tt_free_netdev */
+}
...
+static int __init tashtalk_init(void)
+{
+ int status;
+
+ if (tash_maxdev < 1)
+ tash_maxdev = 1;
+
+ pr_info("TashTalk Interface (dynamic channels, max=%d)",
+ tash_maxdev);
No info messages, please.
+ tashtalk_devs =
+ kcalloc(tash_maxdev, sizeof(struct net_device *), GFP_KERNEL);
+ if (!tashtalk_devs)
+ return -ENOMEM;
Something more modern? Like idr or a list?
+ /* Fill in our line protocol discipline, and register it */
+ status = tty_register_ldisc(&tashtalk_ldisc);
+ if (status != 0) {
+ pr_err("TaskTalk: can't register line discipline (err = %d)\n",
+ status);
+ kfree(tashtalk_devs);
+ }
+ return status;
+}
+
+static void __exit tashtalk_exit(void)
+{
+ unsigned long timeout = jiffies + HZ;
+ struct net_device *dev;
+ struct tashtalk *tt;
+ int busy = 0;
+ int i;
+
+ if (!tashtalk_devs)
+ return;
+
+ /* First of all: check for active disciplines and hangup them. */
+ do {
+ if (busy)
+ msleep_interruptible(100);
+
+ busy = 0;
+ for (i = 0; i < tash_maxdev; i++) {
+ dev = tashtalk_devs[i];
+ if (!dev)
+ continue;
+ tt = netdev_priv(dev);
+ spin_lock_bh(&tt->lock);
+ if (tt->tty) {
+ busy++;
+ tty_hangup(tt->tty);
+ }
+ spin_unlock_bh(&tt->lock);
+ }
+ } while (busy && time_before(jiffies, timeout));
Is this neeeded at all? You cannot unload the module while the ldisc is
active, right? (Unlike NET, TTY increases module count.)
+ for (i = 0; i < tash_maxdev; i++) {
+ dev = tashtalk_devs[i];
+ if (!dev)
+ continue;
+ tashtalk_devs[i] = NULL;
+
+ tt = netdev_priv(dev);
+ if (tt->tty) {
+ pr_err("%s: tty discipline still running\n",
+ dev->name);
+ }
+
+ unregister_netdev(dev);
Those should be unregistered in tty ldisc closes, no?
+ }
+
+ kfree(tashtalk_devs);
+ tashtalk_devs = NULL;
+
+ tty_unregister_ldisc(&tashtalk_ldisc);
+}
thanks,
--
js
suse labs