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@xxxxxxxxx>
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
