Usart Network driver

Question

I wrote a network driver and I would like a review of my code. The code generally works, albeit with some performance issues. The purpose of my code is to have a Network driver that uses the USART and then the pin is there to enable the enable on the hardware so it starts to send and when is is not sending it needs to be dissabled.

Header file

#include <linux/if_ether.h>
#include <linux/if_vlan.h>
#include <linux/types.h>
#include <linux/serdev.h>
#include <linux/gpio/consumer.h>

#define STMUART_DRV_VERSION "0.1.0"
#define STMUART_DRV_NAME "stmNetUart"
#define STMUART_TX_TIMEOUT (1 * HZ)

/* Frame is currently being received */
#define STMFRM_GATHER 0

/*  No header byte while expecting it */
#define STMFRM_NOHEAD (STMFRM_ERR_BASE - 1)

/* Min/Max Ethernet MTU: 46/1500 */
#define STMFRM_MIN_MTU (ETH_ZLEN - ETH_HLEN)
#define STMFRM_MAX_MTU ETH_DATA_LEN

/* Min/Max frame lengths */
#define STMFRM_MIN_LEN (STMFRM_MIN_MTU + ETH_HLEN)
#define STMFRM_MAX_LEN (STMFRM_MAX_MTU + VLAN_ETH_HLEN)

/* QCA7K header len */
#define STMFRM_HEADER_LEN 8

/* QCA7K Framing. */
#define STMFRM_ERR_BASE -1000

enum stmfrm_state {
    /*  Waiting first 0xAA of header */
    STMFRM_WAIT_AA1 = 0x8000,

    /*  Waiting second 0xAA of header */
    STMFRM_WAIT_AA2 = STMFRM_WAIT_AA1 - 1,

    /*  Waiting third 0xAA of header */ 
    STMFRM_WAIT_AA3 = STMFRM_WAIT_AA2 - 1,

    /*  Waiting fourth 0xAA of header */
    STMFRM_WAIT_AA4 = STMFRM_WAIT_AA3 - 1,
    
    /*  Waiting fourth 0xAA of header */
    STMFRM_WAIT_AA5 = STMFRM_WAIT_AA4 - 1,
    
    /*  Waiting fourth 0xAA of header */
    STMFRM_WAIT_AA6 = STMFRM_WAIT_AA5 - 1,
    
    /*  Waiting fourth 0xAA of header */
    STMFRM_WAIT_AA7 = STMFRM_WAIT_AA6 - 1,
    
    /*  Waiting fourth 0xAA of header */
    STMFRM_WAIT_AB8 = STMFRM_WAIT_AA7 - 1,
};

/*   Structure to maintain the frame decoding during reception. */

struct stmfrm_handle {
    /*  Current decoding state */
    enum stmfrm_state state;
    /* Initial state depends on connection type */
    enum stmfrm_state init;

    /* Offset in buffer*/
    u16 offset;

    /* Frame length as kept by this module */
    u16 len;
};

u16 stmfrm_create_header(u8 *buf);

static inline void qcafrm_fsm_init_uart(struct stmfrm_handle *handle)
{
    handle->init = STMFRM_WAIT_AA1;
    handle->state = handle->init;
}


/*   Gather received bytes and try to extract a full Ethernet frame
 *   by following a simple state machine.
 *
 * Return:   QCAFRM_GATHER       No Ethernet frame fully received yet.
 *           QCAFRM_NOHEAD       Header expected but not found.
 */

s32 stmfrm_fsm_decode(struct stmfrm_handle *handle, u8 *buf, u16 buf_len, u8 recv_byte);

struct stmuart {
    struct net_device *net_dev;
    struct gpio_desc *rts_gpio;
    spinlock_t lock;            /* transmit lock */
    struct work_struct tx_work;     /* Flushes transmit buffer   */

    struct serdev_device *serdev;
    struct stmfrm_handle frm_handle;
    struct sk_buff *rx_skb;

    unsigned char *tx_head;         /* pointer to next XMIT byte */
    int tx_left;                /* bytes left in XMIT queue  */
    unsigned char *tx_buffer;
};

Implementation file

#include <linux/device.h>
#include <linux/errno.h>
#include <linux/etherdevice.h>
#include <linux/if_arp.h>
#include <linux/if_ether.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/netdevice.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_net.h>
#include <linux/sched.h>
#include <linux/serdev.h>
#include <linux/skbuff.h>
#include <linux/types.h>
#include <linux/serial.h>
#include <linux/tty.h>
#include <linux/tty_driver.h>
#include <linux/poll.h>

#include "serdevNetwork.h"

u16
stmfrm_create_header(u8 *buf)
{
    if (!buf)
        return 0;

    buf[0] = 0xAA;
    buf[1] = 0xAA;
    buf[2] = 0xAA;
    buf[3] = 0xAA;
    buf[4] = 0xAA;
    buf[5] = 0xAA;
    buf[6] = 0xAA;
    buf[7] = 0xAB;

    return STMFRM_HEADER_LEN;
}

s32
stmfrm_fsm_decode(struct stmfrm_handle *handle, u8 *buf, u16 buf_len, u8 recv_byte)
{
    s32 ret = STMFRM_GATHER;

    switch (handle->state) {
    /* 4 bytes header pattern */
    case STMFRM_WAIT_AA1:
    case STMFRM_WAIT_AA2:
    case STMFRM_WAIT_AA3:
    case STMFRM_WAIT_AA4:
    case STMFRM_WAIT_AA5:
    case STMFRM_WAIT_AA6:
    case STMFRM_WAIT_AA7:
        if (recv_byte != 0xAA) {
            ret = STMFRM_NOHEAD;
            handle->state = handle->init;
        } else {
            handle->state--;
        }
        break;
    case STMFRM_WAIT_AB8:
        if (recv_byte != 0xAB) {
            ret = STMFRM_NOHEAD;
            handle->state = handle->init;
        } else {
            handle->offset = 0;
            handle->state--;
        }
        break;
    default:
        /* Receiving Ethernet frame itself. */
        buf[handle->offset] = recv_byte;
        handle->offset++;
        break;
    }

    return ret;
}

static int
stm_tty_receive(struct serdev_device *serdev, const unsigned char *data,
        size_t count)
{
    struct stmuart *stm = serdev_device_get_drvdata(serdev);
    struct net_device *netdev = stm->net_dev;
    struct net_device_stats *n_stats = &netdev->stats;
    struct stmfrm_handle *frame_handle = &stm->frm_handle;
    size_t i, c;

    if (!stm->rx_skb) {
        stm->rx_skb = netdev_alloc_skb_ip_align(netdev,
                            netdev->mtu +
                            VLAN_ETH_HLEN);
        if (!stm->rx_skb) {
            n_stats->rx_errors++;
            n_stats->rx_dropped++;
            return 0;
        }
    }
    
    if(count > (netdev->mtu + VLAN_ETH_HLEN)){
        c = (netdev->mtu + VLAN_ETH_HLEN);
    }else{
        c = count;
    }

    for (i = 0; i < c; i++) {
        s32 retcode;

        retcode = stmfrm_fsm_decode(frame_handle,
                        stm->rx_skb->data,
                        skb_tailroom(stm->rx_skb),
                        data[i]);

        switch (retcode) {
        case STMFRM_GATHER:
        case STMFRM_NOHEAD:
            break;
        }
    }
    
    frame_handle->state = frame_handle->init;
    n_stats->rx_packets++;
    n_stats->rx_bytes += frame_handle->offset;
    skb_put(stm->rx_skb, frame_handle->offset);
    stm->rx_skb->protocol = eth_type_trans(
            stm->rx_skb, stm->rx_skb->dev);
    stm->rx_skb->ip_summed = CHECKSUM_NONE;
    netif_rx_ni(stm->rx_skb);
    stm->rx_skb = netdev_alloc_skb_ip_align(netdev,
                        netdev->mtu +
                        VLAN_ETH_HLEN);
    if (!stm->rx_skb) {
        netdev_dbg(netdev, "recv: out of RX resources\n");
        n_stats->rx_errors++;
        return i;
    }

    return i;
}

/* Write out any remaining transmit buffer. Scheduled when tty is writable */
static void stmuart_transmit(struct work_struct *work)
{
    struct stmuart *stm = container_of(work, struct stmuart, tx_work);
    struct net_device_stats *n_stats = &stm->net_dev->stats;
    int written;

    spin_lock_bh(&stm->lock);

    /* First make sure we're connected. */
    if (!netif_running(stm->net_dev)) {
        spin_unlock_bh(&stm->lock);
        return;
    }

    if (stm->tx_left <= 0)  {
        /* Now serial buffer is almost free & we can start
         * transmission of another packet
         */
        n_stats->tx_packets++;
        spin_unlock_bh(&stm->lock);
        netif_wake_queue(stm->net_dev);
        return;
    }
    
    written = serdev_device_write_buf(stm->serdev, stm->tx_head,
                      stm->tx_left);
    
    if (written > 0) {
        stm->tx_left -= written;
        stm->tx_head += written;
    }
    
    spin_unlock_bh(&stm->lock);
}

/* Called by the driver when there's room for more data.
 * Schedule the transmit.
 */
static void stm_tty_wakeup(struct serdev_device *serdev)
{
    struct stmuart *stm = serdev_device_get_drvdata(serdev);

    schedule_work(&stm->tx_work);
}

static const struct serdev_device_ops stm_serdev_ops = {
    .receive_buf = stm_tty_receive,
    .write_wakeup = stm_tty_wakeup,
};

static int stmuart_netdev_open(struct net_device *dev)
{
    struct stmuart *stm = netdev_priv(dev);
    netif_start_queue(stm->net_dev);
 
    return 0;
}

static int stmuart_netdev_close(struct net_device *dev)
{
    struct stmuart *stm = netdev_priv(dev);

    netif_stop_queue(dev);
    flush_work(&stm->tx_work);

    spin_lock_bh(&stm->lock);
    stm->tx_left = 0;
    spin_unlock_bh(&stm->lock);
    return 0;
}

static netdev_tx_t
stmuart_netdev_xmit(struct sk_buff *skb, struct net_device *dev)
{
    struct net_device_stats *n_stats = &dev->stats;
    struct stmuart *stm = netdev_priv(dev);
    u8 pad_len = 0;
    int written;
    u8 *pos;
    
    spin_lock(&stm->lock);
    gpiod_set_value(stm->rts_gpio, 1);
    //serdev_device_wait_for_cts(stm->serdev, true, 10);

    WARN_ON(stm->tx_left);

    if (!netif_running(dev))  {
        spin_unlock(&stm->lock);
        netdev_warn(stm->net_dev, "xmit: iface is down\n");
        goto out;
    }

    pos = stm->tx_buffer;

    if (skb->len < STMFRM_MIN_LEN)
        pad_len = STMFRM_MIN_LEN - skb->len;

    pos += stmfrm_create_header(pos);

    memcpy(pos, skb->data, skb->len);
    pos += skb->len;

    if (pad_len) {
        memset(pos, 0, pad_len);
        pos += pad_len;
    }

    netif_stop_queue(stm->net_dev);

    written = serdev_device_write_buf(stm->serdev, stm->tx_buffer,
                      pos - stm->tx_buffer);
    
    if (written > 0) {
        stm->tx_left = (pos - stm->tx_buffer) - written;
        stm->tx_head = stm->tx_buffer + written;
        n_stats->tx_bytes += written;
    }
    

out:
    spin_unlock(&stm->lock);
    gpiod_set_value(stm->rts_gpio, 0);
    //serdev_device_wait_for_cts(stm->serdev, false, 10);
    netif_trans_update(dev);
    dev_kfree_skb_any(skb);
    return NETDEV_TX_OK;
}

static void stmuart_netdev_tx_timeout(struct net_device *dev, unsigned int txqueue)
{
    struct stmuart *stm = netdev_priv(dev);

    netdev_info(stm->net_dev, "Transmit timeout at %ld, latency %ld\n",
            jiffies, dev_trans_start(dev));
    dev->stats.tx_errors++;
    dev->stats.tx_dropped++;
}

static int stmuart_netdev_init(struct net_device *dev)
{
    struct stmuart *stm = netdev_priv(dev);
    size_t len;

    /* Finish setting up the device info. */
    dev->mtu = STMFRM_MAX_MTU;
    dev->type = ARPHRD_ETHER;

    len = STMFRM_HEADER_LEN + STMFRM_MAX_LEN;// + STMFRM_FOOTER_LEN;
    stm->tx_buffer = devm_kmalloc(&stm->serdev->dev, len, GFP_KERNEL);
    if (!stm->tx_buffer)
        return -ENOMEM;

    stm->rx_skb = netdev_alloc_skb_ip_align(stm->net_dev,
                        stm->net_dev->mtu +
                        VLAN_ETH_HLEN);
    if (!stm->rx_skb)
        return -ENOBUFS;

    return 0;
}

static void stmuart_netdev_uninit(struct net_device *dev)
{
    struct stmuart *stm = netdev_priv(dev);

    dev_kfree_skb(stm->rx_skb);
}

static const struct net_device_ops stmuart_netdev_ops = {
    .ndo_init = stmuart_netdev_init,
    .ndo_uninit = stmuart_netdev_uninit,
    .ndo_open = stmuart_netdev_open,
    .ndo_stop = stmuart_netdev_close,
    .ndo_start_xmit = stmuart_netdev_xmit,
    .ndo_set_mac_address = eth_mac_addr,
    .ndo_tx_timeout = stmuart_netdev_tx_timeout,
    .ndo_validate_addr = eth_validate_addr,
};

static void stmuart_netdev_setup(struct net_device *dev)
{
    dev->netdev_ops = &stmuart_netdev_ops;
    dev->watchdog_timeo = STMUART_TX_TIMEOUT;
    dev->priv_flags &= ~IFF_TX_SKB_SHARING;
    dev->tx_queue_len = 100;

    /* MTU range: 46 - 1500 */
    dev->min_mtu = STMFRM_MIN_MTU;
    dev->max_mtu = STMFRM_MAX_MTU;
}

static const struct of_device_id stm32_match[] = {
    {
        .compatible = "st,stm32_usart_net",
    },
    {}
};
MODULE_DEVICE_TABLE(of, stm32_match);

static int stm_uart_probe(struct serdev_device *serdev)
{
    struct net_device *stmuart_dev = alloc_etherdev(sizeof(struct stmuart));
    struct stmuart *stm;
    const char *mac;
    //u32 speed = 10000000u;
    u32 speed = 3000000u;
    int ret;
    
    if (!stmuart_dev)
        return -ENOMEM;

    stmuart_netdev_setup(stmuart_dev);
    SET_NETDEV_DEV(stmuart_dev, &serdev->dev);

    stm = netdev_priv(stmuart_dev);
    if (!stm) {
        pr_err("qca_uart: Fail to retrieve private structure\n");
        ret = -ENOMEM;
        goto free;
    } 
    stm->net_dev = stmuart_dev;
    stm->serdev = serdev;
    qcafrm_fsm_init_uart(&stm->frm_handle);

    spin_lock_init(&stm->lock);
    INIT_WORK(&stm->tx_work, stmuart_transmit);

    mac = of_get_mac_address(serdev->dev.of_node);

    if (!IS_ERR(mac))
        ether_addr_copy(stm->net_dev->dev_addr, mac);

    //if (!is_valid_ether_addr(stm->net_dev->dev_addr)) {
        eth_hw_addr_random(stm->net_dev);
        dev_info(&serdev->dev, "Using random MAC address: %pM\n",
             stm->net_dev->dev_addr);
    //}

    netif_carrier_on(stm->net_dev);
    serdev_device_set_drvdata(serdev, stm);
    serdev_device_set_client_ops(serdev, &stm_serdev_ops);
    
    ret = serdev_device_open(serdev);
    if (ret) {
        dev_err(&serdev->dev, "Unable to open device %s\n",
            stmuart_dev->name);
        goto free;
    }

    speed = serdev_device_set_baudrate(serdev, speed);
    dev_info(&serdev->dev, "Using baudrate: %u\n", speed);
   
    serdev_device_set_flow_control(serdev, false);
    
    stm->rts_gpio = devm_gpiod_get(&serdev->dev, "rts", GPIOD_OUT_HIGH);
    gpiod_set_value(stm->rts_gpio, 0);
    
    if(IS_ERR(stm->rts_gpio)){
        dev_err(&serdev->dev, "Unable to open rts_gpio %s\n",
            stmuart_dev->name);
    }

    ret = register_netdev(stmuart_dev);
    if (ret) {
        dev_err(&serdev->dev, "Unable to register net device %s\n",
            stmuart_dev->name);
        serdev_device_close(serdev);
        cancel_work_sync(&stm->tx_work);
        goto free;
    }
    
    return ret;

free:
    free_netdev(stmuart_dev);
    return ret;
}

static void stm_uart_remove(struct serdev_device *serdev)
{
    struct stmuart *stm = serdev_device_get_drvdata(serdev);

    unregister_netdev(stm->net_dev);

    /* Flush any pending characters in the driver. */
    serdev_device_close(serdev);
    cancel_work_sync(&stm->tx_work);

    free_netdev(stm->net_dev);
}

static struct serdev_device_driver stm_uart_driver = {
    .probe = stm_uart_probe,
    .remove = stm_uart_remove,
    .driver = {
        .name = STMUART_DRV_NAME,
        .of_match_table = of_match_ptr(stm32_match),
    },
};

module_serdev_device_driver(stm_uart_driver);

MODULE_DESCRIPTION("USART Network driver");
MODULE_AUTHOR("xxx");
MODULE_LICENSE("GPL");
MODULE_VERSION(STMUART_DRV_VERSION);

I hope someone could tell me what I could do better. My secend thing is my pin get not always toggled a package gets send.

I add it here i used a lof of stuff from the qca_uart driver https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/qca_uart.c

Answer 1

Weird enum values

Why does stmfrm_state start at 0x8000, and then goes down? Also, if the enum names all start with SMFRM_WAIT_AA and only have a different number at the end, this tells me you shouldn't be using an enum here to begin with, and instead you should use an unsigned int counter.

Indeed it looks like it's just counting how many bytes of the header pattern it has received so far.

Related to this, why the arbitrarily chosen value of -1000 for STMFRM_ERR_BASE?

Authorship of code

Could it be you are copy&pasting code from the QCA7000 driver? That would explain the weird enum values. If so, be aware that you should include the copyright notice of the original.

Make the header a const array

Instead of having a function that writes individual bytes to a buffer and returning a constant length value, I would create a const array that holds the header:

static const u8 stmfrm_header[8] = {0xAA, 0xAA, ..., 0xAA, 0xAB};

Then instead of writing pos += strfrm_create_header(pos), write:

memcpy(pos, stmfrm_header, sizeof stmfrm_header);
pos += sizeof stmfrm_header;

Also, if you pre-allocated the transmit buffer, you can initialize its header in stmuart_netdev_init() instead of doing this every time stmuart_netdev_xmit() is called.

Length of GPIO pin being high

In stmuart_netdev_xmit(), you start transmitting a packet, but if it doesn't fit inside the serial port's transmit buffer, you let stmuart_transmit() send the remainder. However, the GPIO pin is only held high for the duration of stmuart_netdev_xmit(), which might actually be arbitrarily short. Either the GPIO pin should be pulsed to signal the start of a new packet, in which case there is a minimum time that it should be held high that should be observed, or it should be held high for the whole packet, in which case you have to wait for the whole packet to be sent out.

Also, once the GPIO pin goes low there is likely also a minimum time it should be held low.

Does this code really depend on an STM32?

Everything has "stm" in the name, and there's even a mention of stm32 in the code. However, it's just sending things over a serial port and toggling a GPIO pin, that doesn't seem to me like it is something only an STM32 chip could do. Won't the QCA7000 driver already work on an STM32? If not, maybe try to make it more generic and not depend explicitly on any one vendor's hardware.

Accessing the RTS pin

It looks like you ask the serial device subsystem for the GPIO pin associated with the RTS line, and then use gpiod_*() functions to toggle this pin. But you should be able to use serdev_device_set_rts() to toggle that pin without having to involve the GPIO subsystem yourself.