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

["Arnd Bergmann" <arnd@arndb.de>]

On Sat, Aug 17, 2024, at 11:33, Rodolfo Zitellini wrote:
> This is the TashTalk driver, it perits for a modern machine to
> participate in a Apple LocalTalk network and is compatibile with
> Netatalk.
>
> Please see the included documentation for details:
> Documentation/networking/device_drivers/appletalk/index.rst
>
> Signed-off-by: Rodolfo Zitellini <rwz@xhero.org>

Hi Rodolfo,

Nice to see you got this into a working state! I vaguely
remember discussing this in the past, and suggesting you
try a user space solution, so it would be good if you can
add in the patch description why you ended up with a kernel
driver after all.

My main concern at this point is the usage of .ndo_do_ioctl.
I had previously sent patches to completely remove that
from the kernel, but never got around to send a new version
after the previous review. I still have them in my tree
and should be able to send them again, but that will obviously
conflict with your added use.

> +static struct net_device **tashtalk_devs;
> +
> +static int tash_maxdev = TASH_MAX_CHAN;
> +module_param(tash_maxdev, int, 0);
> +MODULE_PARM_DESC(tash_maxdev, "Maximum number of tashtalk devices");

You should not need to keep a list of the devices
or a module parameter to limit the number. I'm fairly sure
the devices are already tracked by the network stack in a
way that lets you enumerate them later.

> +static void tashtalk_send_ctrl_packet(struct tashtalk *tt, unsigned 
> char dst,
> +				      unsigned char src, unsigned char type);
> +
> +static unsigned char tt_arbitrate_addr_blocking(struct tashtalk *tt, 
> unsigned char addr);

Please try to avoid forward declations and instead reorder the
functions to put the callers after the calles.

> +static void tash_setbits(struct tashtalk *tt, unsigned char addr)
> +{
> +	unsigned char bits[33];
> +	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);
> +	bits[byte] = (1 << pos);

This is basically set_bit_le(), so you could use that
for clarity and use an array of 'unsigned long' words.

> +	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));
> +}

Can you use the global crc_ccitt() function instead of implementing
your own?

> +static int tt_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	struct sockaddr_at *sa = (struct sockaddr_at *)&ifr->ifr_addr;
> +	struct tashtalk *tt = netdev_priv(dev);
> +	struct atalk_addr *aa = &tt->node_addr;
> +
> +	switch (cmd) {
> +	case SIOCSIFADDR:
> +
> +		sa->sat_addr.s_node =
> +			tt_arbitrate_addr_blocking(tt, sa->sat_addr.s_node);
> +
> +		aa->s_net = sa->sat_addr.s_net;
> +		aa->s_node = sa->sat_addr.s_node;
> +
> +		/* Set broadcast address. */
> +		dev->broadcast[0] = 0xFF;
> +
> +		/* Set hardware address. */
> +		dev->addr_len = 1;
> +		dev_addr_set(dev, &aa->s_node);
> +
> +		/* Setup tashtalk to respond to that addr */
> +		tash_setbits(tt, aa->s_node);
> +
> +		return 0;
> +
> +	case SIOCGIFADDR:
> +		sa->sat_addr.s_net = aa->s_net;
> +		sa->sat_addr.s_node = aa->s_node;
> +
> +		return 0;

As we discussed in the past, I think this really should
not use ndo_do_ioctl(), which instead should just disappear.

Please change the caller to use some other method of
setting the address in the driver.

> +static int tashtalk_ioctl(struct tty_struct *tty, unsigned int cmd,
> +			  unsigned long arg)
> +{
> +	struct tashtalk *tt = tty->disc_data;
> +	int __user *p = (int __user *)arg;
> +	unsigned int tmp;
> +
> +	/* First make sure we're connected. */
> +	if (!tt || tt->magic != TASH_MAGIC)
> +		return -EINVAL;
> +
> +	switch (cmd) {
> +	case SIOCGIFNAME:
> +		tmp = strlen(tt->dev->name) + 1;
> +		if (copy_to_user((void __user *)arg, tt->dev->name, tmp))
> +			return -EFAULT;
> +		return 0;
> +
> +	case SIOCGIFENCAP:
> +		if (put_user(tt->mode, p))
> +			return -EFAULT;
> +		return 0;
> +
> +	case SIOCSIFENCAP:
> +		if (get_user(tmp, p))
> +			return -EFAULT;
> +		tt->mode = tmp;
> +		return 0;
> +
> +	case SIOCSIFHWADDR:
> +		return -EINVAL;
> +
> +	default:
> +		return tty_mode_ioctl(tty, cmd, arg);
> +	}

I'm also not a bit fan of using the SIOC* command codes
in a tty device with incompatible argument types. I do
see that slip and x25 do the same, but it would be nice
to find a better interface for these. I have not looked
at all the other line disciplines, but maybe you can
find a better example to copy.

     Arnd