Message-ID: <74e86f5b-ee2e-4093-ab38-41cc52039601@kernel.org>
Date: Mon, 19 Aug 2024 11:55:57 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Rodolfo Zitellini <rwz@...ro.org>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jonathan Corbet <corbet@....net>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: netdev@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
 Arnd Bergmann <arnd@...db.de>, Doug Brown <doug@...morgal.com>
Subject: Re: [PATCH net-next 2/2] appletalk: tashtalk: Add LocalTalk line
 discipline driver for AppleTalk using a TashTalk adapter

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

Powered by blists - more mailing lists