Re: [PATCH net-next 2/2] appletalk: tashtalk: Add LocalTalk line discipline driver for AppleTalk using a TashTalk adapter

Jiri Slaby
2024-08-19T09:56:00Z

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