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

[Simon Horman <horms@kernel.org>]

On Sat, Aug 17, 2024 at 11:33:16AM +0200, 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

nit: compatible

     Flagged by checkpatch.pl --codespell

> Netatalk.
> 
> Please see the included documentation for details:
> Documentation/networking/device_drivers/appletalk/index.rst
> 
> Signed-off-by: Rodolfo Zitellini <rwz@xhero.org>

...

> diff --git a/drivers/net/appletalk/tashtalk.c b/drivers/net/appletalk/tashtalk.c

...

> +/* Called by the driver when there's room for more data.
> + * Schedule the transmit.
> + */
> +static void tashtalk_write_wakeup(struct tty_struct *tty)
> +{
> +	struct tashtalk *tt;

I think that tt needs an __rcu annotation. Sparse says:

.../tashtalk.c:290:14: error: incompatible types in comparison expression (different address spaces):
.../tashtalk.c:290:14:    void [noderef] __rcu *
.../tashtalk.c:290:14:    void *

> +
> +	rcu_read_lock();
> +	tt = rcu_dereference(tty->disc_data);
> +	if (tt)
> +		schedule_work(&tt->tx_work);
> +	rcu_read_unlock();
> +}

...

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

actual is set but otherwise unused in this function.
Should it be used as part of checking, with an error code returned on error?
If not, it should probably be removed.

Flagged by W=1 builds.

...

> +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)
> +		return;
> +
> +	spin_lock_bh(&tt->lock);
> +	rcu_assign_pointer(tty->disc_data, NULL);

I think tty->disc_data also needs an __rcu annotation, which will
likely highlight the need for annotations elsewhere. Flagged by Sparse.

> +	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 */
> +}

...