Re: [PATCH net-next v2] mctp: Add MCTP-over-serial transport binding

[Jiri Slaby <jirislaby@kernel.org>]

On 23. 11. 21, 2:59, Jeremy Kerr wrote:
> This change adds a MCTP Serial transport binding, as defined by DMTF
> specificiation DSP0253 - "MCTP Serial Transport Binding". This is
> implemented as a new serial line discipline, and can be attached to
> arbitrary tty devices.
…
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -0,0 +1,510 @@
…
> +static int next_chunk_len(struct mctp_serial *dev)
> +{
> +	int i;
> +
> +	/* either we have no bytes to send ... */
> +	if (dev->txpos == dev->txlen)
> +		return 0;
> +
> +	/* ... or the next byte to send is an escaped byte; requiring a
> +	 * single-byte chunk...

This is not a correct multi-line comment. Or does net/ differ in this?

> +	 */
> +	if (needs_escape(dev->txbuf[dev->txpos]))
> +		return 1;
> +
> +	/* ... or we have one or more bytes up to the next escape - this chunk
> +	 * will be those non-escaped bytes, and does not include the escaped
> +	 * byte.
> +	 */
> +	for (i = 1; i + dev->txpos + 1 < dev->txlen; i++) {
> +		if (needs_escape(dev->txbuf[dev->txpos + i + 1]))
> +			break;
> +	}
> +
> +	return i;
> +}
> +
> +static int write_chunk(struct mctp_serial *dev, unsigned char *buf, int len)
> +{
> +	return dev->tty->ops->write(dev->tty, buf, len);
> +}


> +static netdev_tx_t mctp_serial_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mctp_serial *dev = netdev_priv(ndev);
> +	unsigned long flags;
> +
> +	WARN_ON(dev->txstate != STATE_IDLE);
> +
> +	if (skb->len > MCTP_SERIAL_MTU) {

Shouldn't you implement ndo_change_mtu to forbid larger frames instead?

> +		dev->netdev->stats.tx_dropped++;
> +		goto out;
> +	}
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	netif_stop_queue(dev->netdev);
> +	skb_copy_bits(skb, 0, dev->txbuf, skb->len);
> +	dev->txpos = 0;
> +	dev->txlen = skb->len;
> +	dev->txstate = STATE_START;
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	set_bit(TTY_DO_WRITE_WAKEUP, &dev->tty->flags);
> +	schedule_work(&dev->tx_work);
> +
> +out:
> +	kfree_skb(skb);
> +	return NETDEV_TX_OK;
> +}
…
> +static void mctp_serial_rx(struct mctp_serial *dev)
> +{
> +	struct mctp_skb_cb *cb;
> +	struct sk_buff *skb;
> +
> +	if (dev->rxfcs != dev->rxfcs_rcvd) {
> +		dev->netdev->stats.rx_dropped++;
> +		dev->netdev->stats.rx_crc_errors++;
> +		return;
> +	}
> +
> +	skb = netdev_alloc_skb(dev->netdev, dev->rxlen);

Just thinking… Wouldn't it be easier to have dev->skb instead of 
dev->rxbuf and push data to it directly in all those mctp_serial_push*()?

> +	if (!skb) {
> +		dev->netdev->stats.rx_dropped++;
> +		return;
> +	}
> +
> +	skb->protocol = htons(ETH_P_MCTP);
> +	skb_put_data(skb, dev->rxbuf, dev->rxlen);
> +	skb_reset_network_header(skb);
> +
> +	cb = __mctp_cb(skb);
> +	cb->halen = 0;
> +
> +	netif_rx_ni(skb);
> +	dev->netdev->stats.rx_packets++;
> +	dev->netdev->stats.rx_bytes += dev->rxlen;
> +}


> +static int mctp_serial_open(struct tty_struct *tty)
> +{
> +	struct mctp_serial *dev;
> +	struct net_device *ndev;
> +	char name[32];
> +	int idx, rc;
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (!tty->ops->write)
> +		return -EOPNOTSUPP;
> +
> +	if (tty->disc_data)
> +		return -EEXIST;

This should never happen™.

> +
> +	idx = ida_alloc(&mctp_serial_ida, GFP_KERNEL);
> +	if (idx < 0)
> +		return idx;
> +
> +	snprintf(name, sizeof(name), "mctpserial%d", idx);
> +	ndev = alloc_netdev(sizeof(*dev), name, NET_NAME_ENUM,
> +			    mctp_serial_setup);
> +	if (!ndev) {
> +		rc = -ENOMEM;
> +		goto free_ida;
> +	}
> +
> +	dev = netdev_priv(ndev);
> +	dev->idx = idx;
> +	dev->tty = tty;
> +	dev->netdev = ndev;
> +	dev->txstate = STATE_IDLE;
> +	dev->rxstate = STATE_IDLE;
> +	spin_lock_init(&dev->lock);
> +	INIT_WORK(&dev->tx_work, mctp_serial_tx_work);
> +
> +	rc = register_netdev(ndev);
> +	if (rc)
> +		goto free_netdev;
> +
> +	tty->receive_room = 64 * 1024;
> +	tty->disc_data = dev;
> +
> +	return 0;
> +
> +free_netdev:
> +	free_netdev(ndev);
> +
> +free_ida:
> +	ida_free(&mctp_serial_ida, idx);
> +	return rc;
> +}
> +
> +static void mctp_serial_close(struct tty_struct *tty)
> +{
> +	struct mctp_serial *dev = tty->disc_data;
> +	int idx = dev->idx;
> +
> +	unregister_netdev(dev->netdev);
> +	ida_free(&mctp_serial_ida, idx);

No tx_work flushing/cancelling?

> +}

regards,
-- 
js
suse labs