RE: [PATCH v8 1/2] platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc
From: Vadim Pasternak
Date: Wed Jan 30 2019 - 01:24:38 EST
> -----Original Message-----
> From: Liming Sun <lsun@xxxxxxxxxxxx>
> Sent: Monday, January 28, 2019 7:28 PM
> To: Rob Herring <robh+dt@xxxxxxxxxx>; Mark Rutland
> <mark.rutland@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; David Woods
> <dwoods@xxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren
> Hart <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: Liming Sun <lsun@xxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
> Subject: [PATCH v8 1/2] platform/mellanox: Add TmFifo driver for Mellanox
> BlueField Soc
>
> This commit adds the TmFifo platform driver for Mellanox BlueField Soc. TmFifo
> is a shared FIFO which enables external host machine to exchange data with the
> SoC via USB or PCIe. The driver is based on virtio framework and has console
> and network access enabled.
>
> Reviewed-by: David Woods <dwoods@xxxxxxxxxxxx>
> Signed-off-by: Liming Sun <lsun@xxxxxxxxxxxx>
> ---
> drivers/platform/mellanox/Kconfig | 13 +-
> drivers/platform/mellanox/Makefile | 1 +
> drivers/platform/mellanox/mlxbf-tmfifo-regs.h | 67 ++
> drivers/platform/mellanox/mlxbf-tmfifo.c | 1289
> +++++++++++++++++++++++++
> 4 files changed, 1369 insertions(+), 1 deletion(-) create mode 100644
> drivers/platform/mellanox/mlxbf-tmfifo-regs.h
> create mode 100644 drivers/platform/mellanox/mlxbf-tmfifo.c
>
> diff --git a/drivers/platform/mellanox/Kconfig
> b/drivers/platform/mellanox/Kconfig
> index cd8a908..a565070 100644
> --- a/drivers/platform/mellanox/Kconfig
> +++ b/drivers/platform/mellanox/Kconfig
> @@ -5,7 +5,7 @@
>
> menuconfig MELLANOX_PLATFORM
> bool "Platform support for Mellanox hardware"
> - depends on X86 || ARM || COMPILE_TEST
> + depends on X86 || ARM || ARM64 || COMPILE_TEST
> ---help---
> Say Y here to get to see options for platform support for
> Mellanox systems. This option alone does not add any kernel code.
> @@ -34,4 +34,15 @@ config MLXREG_IO
> to system resets operation, system reset causes monitoring and some
> kinds of mux selection.
>
> +config MLXBF_TMFIFO
> + tristate "Mellanox BlueField SoC TmFifo platform driver"
> + depends on ARM64
Why you make it dependent on ARM64?
Should not it work on any host, x86?
> + default m
User who needs it should select this option.
No need default 'm'.
> + select VIRTIO_CONSOLE
> + select VIRTIO_NET
> + help
> + Say y here to enable TmFifo support. The TmFifo driver provides
> + platform driver support for the TmFifo which supports console
> + and networking based on the virtio framework.
> +
> endif # MELLANOX_PLATFORM
> diff --git a/drivers/platform/mellanox/Makefile
> b/drivers/platform/mellanox/Makefile
> index 57074d9c..f0c061d 100644
> --- a/drivers/platform/mellanox/Makefile
> +++ b/drivers/platform/mellanox/Makefile
> @@ -5,3 +5,4 @@
> #
> obj-$(CONFIG_MLXREG_HOTPLUG) += mlxreg-hotplug.o
> obj-$(CONFIG_MLXREG_IO) += mlxreg-io.o
> +obj-$(CONFIG_MLXBF_TMFIFO) += mlxbf-tmfifo.o
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo-regs.h
> b/drivers/platform/mellanox/mlxbf-tmfifo-regs.h
> new file mode 100644
> index 0000000..90c9c2cf
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo-regs.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
> + */
> +
> +#ifndef __MLXBF_TMFIFO_REGS_H__
> +#define __MLXBF_TMFIFO_REGS_H__
> +
> +#include <linux/types.h>
> +
> +#define MLXBF_TMFIFO_TX_DATA 0x0
> +
> +#define MLXBF_TMFIFO_TX_STS 0x8
> +#define MLXBF_TMFIFO_TX_STS__LENGTH 0x0001 #define
> +MLXBF_TMFIFO_TX_STS__COUNT_SHIFT 0 #define
> +MLXBF_TMFIFO_TX_STS__COUNT_WIDTH 9 #define
> +MLXBF_TMFIFO_TX_STS__COUNT_RESET_VAL 0 #define
> +MLXBF_TMFIFO_TX_STS__COUNT_RMASK 0x1ff #define
> +MLXBF_TMFIFO_TX_STS__COUNT_MASK 0x1ff
> +
> +#define MLXBF_TMFIFO_TX_CTL 0x10
> +#define MLXBF_TMFIFO_TX_CTL__LENGTH 0x0001 #define
> +MLXBF_TMFIFO_TX_CTL__LWM_SHIFT 0 #define
> MLXBF_TMFIFO_TX_CTL__LWM_WIDTH
> +8 #define MLXBF_TMFIFO_TX_CTL__LWM_RESET_VAL 128 #define
> +MLXBF_TMFIFO_TX_CTL__LWM_RMASK 0xff #define
> +MLXBF_TMFIFO_TX_CTL__LWM_MASK 0xff #define
> +MLXBF_TMFIFO_TX_CTL__HWM_SHIFT 8 #define
> MLXBF_TMFIFO_TX_CTL__HWM_WIDTH
> +8 #define MLXBF_TMFIFO_TX_CTL__HWM_RESET_VAL 128 #define
> +MLXBF_TMFIFO_TX_CTL__HWM_RMASK 0xff #define
> +MLXBF_TMFIFO_TX_CTL__HWM_MASK 0xff00 #define
> +MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_SHIFT 32 #define
> +MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_WIDTH 9 #define
> +MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_RESET_VAL 256 #define
> +MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_RMASK 0x1ff #define
> +MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_MASK 0x1ff00000000ULL
> +
> +#define MLXBF_TMFIFO_RX_DATA 0x0
> +
> +#define MLXBF_TMFIFO_RX_STS 0x8
> +#define MLXBF_TMFIFO_RX_STS__LENGTH 0x0001 #define
> +MLXBF_TMFIFO_RX_STS__COUNT_SHIFT 0 #define
> +MLXBF_TMFIFO_RX_STS__COUNT_WIDTH 9 #define
> +MLXBF_TMFIFO_RX_STS__COUNT_RESET_VAL 0 #define
> +MLXBF_TMFIFO_RX_STS__COUNT_RMASK 0x1ff #define
> +MLXBF_TMFIFO_RX_STS__COUNT_MASK 0x1ff
> +
> +#define MLXBF_TMFIFO_RX_CTL 0x10
> +#define MLXBF_TMFIFO_RX_CTL__LENGTH 0x0001 #define
> +MLXBF_TMFIFO_RX_CTL__LWM_SHIFT 0 #define
> MLXBF_TMFIFO_RX_CTL__LWM_WIDTH
> +8 #define MLXBF_TMFIFO_RX_CTL__LWM_RESET_VAL 128 #define
> +MLXBF_TMFIFO_RX_CTL__LWM_RMASK 0xff #define
> +MLXBF_TMFIFO_RX_CTL__LWM_MASK 0xff #define
> +MLXBF_TMFIFO_RX_CTL__HWM_SHIFT 8 #define
> MLXBF_TMFIFO_RX_CTL__HWM_WIDTH
> +8 #define MLXBF_TMFIFO_RX_CTL__HWM_RESET_VAL 128 #define
> +MLXBF_TMFIFO_RX_CTL__HWM_RMASK 0xff #define
> +MLXBF_TMFIFO_RX_CTL__HWM_MASK 0xff00 #define
> +MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_SHIFT 32 #define
> +MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_WIDTH 9 #define
> +MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_RESET_VAL 256 #define
> +MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_RMASK 0x1ff #define
> +MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_MASK 0x1ff00000000ULL
> +
> +#endif /* !defined(__MLXBF_TMFIFO_REGS_H__) */
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> b/drivers/platform/mellanox/mlxbf-tmfifo.c
> new file mode 100644
> index 0000000..c1afe47
> --- /dev/null
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -0,0 +1,1289 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Mellanox BlueField SoC TmFifo driver
> + *
> + * Copyright (C) 2019 Mellanox Technologies */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/cache.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/efi.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/version.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_console.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_net.h>
> +#include <linux/virtio_ring.h>
> +#include <asm/byteorder.h>
Is it must ti include from asm?
Could it be replaced with something like
#include <linux/byteorder/generic.h>
> +
> +#include "mlxbf-tmfifo-regs.h"
> +
> +/* Vring size. */
> +#define MLXBF_TMFIFO_VRING_SIZE 1024
> +
> +/* Console Tx buffer size. */
> +#define MLXBF_TMFIFO_CONS_TX_BUF_SIZE (32 * 1024)
> +
> +/* House-keeping timer interval. */
> +static int mlxbf_tmfifo_timer_interval = HZ / 10;
> +
> +/* Global lock. */
> +static DEFINE_MUTEX(mlxbf_tmfifo_lock);
> +
> +/* Virtual devices sharing the TM FIFO. */
> +#define MLXBF_TMFIFO_VDEV_MAX (VIRTIO_ID_CONSOLE + 1)
> +
> +/* Struct declaration. */
> +struct mlxbf_tmfifo;
> +
> +/* Structure to maintain the ring state. */ struct mlxbf_tmfifo_vring {
> + void *va; /* virtual address */
> + dma_addr_t dma; /* dma address */
> + struct virtqueue *vq; /* virtqueue pointer */
> + struct vring_desc *desc; /* current desc */
> + struct vring_desc *desc_head; /* current desc head */
> + int cur_len; /* processed len in current desc */
> + int rem_len; /* remaining length to be processed */
> + int size; /* vring size */
> + int align; /* vring alignment */
> + int id; /* vring id */
> + int vdev_id; /* TMFIFO_VDEV_xxx */
> + u32 pkt_len; /* packet total length */
> + __virtio16 next_avail; /* next avail desc id */
> + struct mlxbf_tmfifo *fifo; /* pointer back to the tmfifo */
> +};
> +
> +/* Interrupt types. */
> +enum {
> + MLXBF_TM_RX_LWM_IRQ, /* Rx low water mark irq */
> + MLXBF_TM_RX_HWM_IRQ, /* Rx high water mark irq */
> + MLXBF_TM_TX_LWM_IRQ, /* Tx low water mark irq */
> + MLXBF_TM_TX_HWM_IRQ, /* Tx high water mark irq */
> + MLXBF_TM_IRQ_CNT
> +};
> +
> +/* Ring types (Rx & Tx). */
> +enum {
> + MLXBF_TMFIFO_VRING_RX, /* Rx ring */
> + MLXBF_TMFIFO_VRING_TX, /* Tx ring */
> + MLXBF_TMFIFO_VRING_NUM
> +};
> +
> +struct mlxbf_tmfifo_vdev {
> + struct virtio_device vdev; /* virtual device */
> + u8 status;
> + u64 features;
> + union { /* virtio config space */
> + struct virtio_console_config cons;
> + struct virtio_net_config net;
> + } config;
> + struct mlxbf_tmfifo_vring vrings[MLXBF_TMFIFO_VRING_NUM];
> + u8 *tx_buf; /* tx buffer */
> + u32 tx_head; /* tx buffer head */
> + u32 tx_tail; /* tx buffer tail */
> +};
> +
> +struct mlxbf_tmfifo_irq_info {
> + struct mlxbf_tmfifo *fifo; /* tmfifo structure */
> + int irq; /* interrupt number */
> + int index; /* array index */
> +};
> +
> +/* TMFIFO device structure */
> +struct mlxbf_tmfifo {
> + struct mlxbf_tmfifo_vdev *vdev[MLXBF_TMFIFO_VDEV_MAX]; /*
> devices */
> + struct platform_device *pdev; /* platform device */
> + struct mutex lock; /* fifo lock */
> + void __iomem *rx_base; /* mapped register base */
> + void __iomem *tx_base; /* mapped register base */
> + int tx_fifo_size; /* number of entries of the Tx FIFO */
> + int rx_fifo_size; /* number of entries of the Rx FIFO */
> + unsigned long pend_events; /* pending bits for deferred process */
> + struct mlxbf_tmfifo_irq_info irq_info[MLXBF_TM_IRQ_CNT]; /* irq info
> */
> + struct work_struct work; /* work struct for deferred process */
> + struct timer_list timer; /* keepalive timer */
> + struct mlxbf_tmfifo_vring *vring[2]; /* current Tx/Rx ring */
> + bool is_ready; /* ready flag */
> + spinlock_t spin_lock; /* spin lock */
> +};
> +
> +union mlxbf_tmfifo_msg_hdr {
> + struct {
> + u8 type; /* message type */
> + __be16 len; /* payload length */
> + u8 unused[5]; /* reserved, set to 0 */
> + } __packed;
> + u64 data;
> +};
> +
> +/*
> + * Default MAC.
> + * This MAC address will be read from EFI persistent variable if configured.
> + * It can also be reconfigured with standard Linux tools.
> + */
> +static u8 mlxbf_tmfifo_net_default_mac[6] = {
> + 0x00, 0x1A, 0xCA, 0xFF, 0xFF, 0x01};
> +
> +/* MTU setting of the virtio-net interface. */
> +#define MLXBF_TMFIFO_NET_MTU 1500
> +
> +/* Maximum L2 header length. */
> +#define MLXBF_TMFIFO_NET_L2_OVERHEAD 36
> +
> +/* Supported virtio-net features. */
> +#define MLXBF_TMFIFO_NET_FEATURES ((1UL << VIRTIO_NET_F_MTU)
> | \
> + (1UL << VIRTIO_NET_F_STATUS) | \
> + (1UL << VIRTIO_NET_F_MAC))
> +
> +/* Return the consumed Tx buffer space. */ static int
> +mlxbf_tmfifo_vdev_tx_buf_len(struct mlxbf_tmfifo_vdev *vdev) {
> + return ((vdev->tx_tail >= vdev->tx_head) ?
> + (vdev->tx_tail - vdev->tx_head) :
> + (MLXBF_TMFIFO_CONS_TX_BUF_SIZE - vdev->tx_head +
> +vdev->tx_tail)); }
I would suggest to split the above.
> +
> +/* Return the available Tx buffer space. */ static int
> +mlxbf_tmfifo_vdev_tx_buf_avail(struct mlxbf_tmfifo_vdev *vdev) {
> + return (MLXBF_TMFIFO_CONS_TX_BUF_SIZE - 8 -
Thins about some extra define for
"MLXBF_TMFIFO_CONS_TX_BUF_SIZE - 8"
> + mlxbf_tmfifo_vdev_tx_buf_len(vdev));
> +}
> +
> +/* Update Tx buffer pointer after pushing data. */ static void
> +mlxbf_tmfifo_vdev_tx_buf_push(struct mlxbf_tmfifo_vdev *vdev,
> + u32 len)
> +{
> + vdev->tx_tail += len;
> + if (vdev->tx_tail >= MLXBF_TMFIFO_CONS_TX_BUF_SIZE)
> + vdev->tx_tail -= MLXBF_TMFIFO_CONS_TX_BUF_SIZE; }
> +
> +/* Update Tx buffer pointer after popping data. */ static void
> +mlxbf_tmfifo_vdev_tx_buf_pop(struct mlxbf_tmfifo_vdev *vdev,
> + u32 len)
> +{
> + vdev->tx_head += len;
> + if (vdev->tx_head >= MLXBF_TMFIFO_CONS_TX_BUF_SIZE)
> + vdev->tx_head -= MLXBF_TMFIFO_CONS_TX_BUF_SIZE; }
> +
> +/* Allocate vrings for the fifo. */
> +static int mlxbf_tmfifo_alloc_vrings(struct mlxbf_tmfifo *fifo,
> + struct mlxbf_tmfifo_vdev *tm_vdev,
> + int vdev_id)
> +{
> + struct mlxbf_tmfifo_vring *vring;
> + dma_addr_t dma;
> + int i, size;
> + void *va;
> +
> + for (i = 0; i < ARRAY_SIZE(tm_vdev->vrings); i++) {
> + vring = &tm_vdev->vrings[i];
> + vring->fifo = fifo;
> + vring->size = MLXBF_TMFIFO_VRING_SIZE;
> + vring->align = SMP_CACHE_BYTES;
> + vring->id = i;
> + vring->vdev_id = vdev_id;
> +
> + size = PAGE_ALIGN(vring_size(vring->size, vring->align));
> + va = dma_alloc_coherent(tm_vdev->vdev.dev.parent, size,
> &dma,
> + GFP_KERNEL);
> + if (!va) {
> + dev_err(tm_vdev->vdev.dev.parent,
> + "vring allocation failed\n");
> + return -EINVAL;
> + }
> +
> + vring->va = va;
> + vring->dma = dma;
> + }
> +
> + return 0;
> +}
> +
> +/* Free vrings of the fifo device. */
> +static void mlxbf_tmfifo_free_vrings(struct mlxbf_tmfifo *fifo, int
> +vdev_id) {
> + struct mlxbf_tmfifo_vdev *tm_vdev = fifo->vdev[vdev_id];
> + struct mlxbf_tmfifo_vring *vring;
> + int i, size;
> +
> + for (i = 0; i < ARRAY_SIZE(tm_vdev->vrings); i++) {
> + vring = &tm_vdev->vrings[i];
> +
> + size = PAGE_ALIGN(vring_size(vring->size, vring->align));
> + if (vring->va) {
> + dma_free_coherent(tm_vdev->vdev.dev.parent, size,
> + vring->va, vring->dma);
> + vring->va = NULL;
> + if (vring->vq) {
> + vring_del_virtqueue(vring->vq);
> + vring->vq = NULL;
> + }
> + }
> + }
> +}
> +
> +/* Free interrupts of the fifo device. */ static void
> +mlxbf_tmfifo_free_irqs(struct mlxbf_tmfifo *fifo) {
> + int i, irq;
> +
> + for (i = 0; i < MLXBF_TM_IRQ_CNT; i++) {
> + irq = fifo->irq_info[i].irq;
> + if (irq) {
> + fifo->irq_info[i].irq = 0;
> + disable_irq(irq);
> + free_irq(irq, (u8 *)fifo + i);
> + }
> + }
> +}
> +
> +/* Interrupt handler. */
> +static irqreturn_t mlxbf_tmfifo_irq_handler(int irq, void *arg) {
> + struct mlxbf_tmfifo_irq_info *irq_info;
> +
> + irq_info = (struct mlxbf_tmfifo_irq_info *)arg;
> +
> + if (irq_info->index < MLXBF_TM_IRQ_CNT &&
> + !test_and_set_bit(irq_info->index, &irq_info->fifo->pend_events))
> + schedule_work(&irq_info->fifo->work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/* Nothing to do for now. */
> +static void mlxbf_tmfifo_virtio_dev_release(struct device *dev) { }
If there is nothing to do - no reason to have it.
> +
> +/* Get the next packet descriptor from the vring. */ static inline
> +struct vring_desc * mlxbf_tmfifo_virtio_get_next_desc(struct virtqueue
> +*vq) {
> + struct mlxbf_tmfifo_vring *vring;
> + unsigned int idx, head;
> + struct vring *vr;
> +
> + vring = (struct mlxbf_tmfifo_vring *)vq->priv;
> + vr = (struct vring *)virtqueue_get_vring(vq);
> +
> + if (!vr || vring->next_avail == vr->avail->idx)
> + return NULL;
> +
> + idx = vring->next_avail % vr->num;
> + head = vr->avail->ring[idx];
> + BUG_ON(head >= vr->num);
> + vring->next_avail++;
> + return &vr->desc[head];
> +}
> +
> +static inline void mlxbf_tmfifo_virtio_release_desc(
> + struct virtio_device *vdev, struct vring *vr,
> + struct vring_desc *desc, u32 len)
> +{
> + unsigned int idx;
> +
> + idx = vr->used->idx % vr->num;
> + vr->used->ring[idx].id = desc - vr->desc;
> + vr->used->ring[idx].len = cpu_to_virtio32(vdev, len);
> +
> + /* Virtio could poll and check the 'idx' to decide
> + * whether the desc is done or not. Add a memory
> + * barrier here to make sure the update above completes
> + * before updating the idx.
> + */
> + mb();
> + vr->used->idx++;
> +}
> +
> +/* Get the total length of a descriptor chain. */ static inline u32
> +mlxbf_tmfifo_virtio_get_pkt_len(struct virtio_device *vdev,
> + struct vring_desc *desc,
> + struct vring *vr)
> +{
> + u32 len = 0, idx;
> +
> + while (desc) {
> + len += virtio32_to_cpu(vdev, desc->len);
> + if (!(virtio16_to_cpu(vdev, desc->flags) &
> VRING_DESC_F_NEXT))
> + break;
> + idx = virtio16_to_cpu(vdev, desc->next);
> + desc = &vr->desc[idx];
> + }
> +
> + return len;
> +}
> +
> +static void mlxbf_tmfifo_release_pkt(struct virtio_device *vdev,
> + struct mlxbf_tmfifo_vring *vring,
> + struct vring_desc **desc)
> +{
> + struct vring *vr = (struct vring *)virtqueue_get_vring(vring->vq);
> + struct vring_desc *desc_head;
> + uint32_t pkt_len = 0;
> +
> + if (!vr)
> + return;
> +
> + if (desc != NULL && *desc != NULL && vring->desc_head != NULL) {
> + desc_head = vring->desc_head;
> + pkt_len = vring->pkt_len;
> + } else {
> + desc_head = mlxbf_tmfifo_virtio_get_next_desc(vring->vq);
> + if (desc_head != NULL) {
> + pkt_len = mlxbf_tmfifo_virtio_get_pkt_len(
> + vdev, desc_head, vr);
> + }
> + }
> +
> + if (desc_head != NULL)
> + mlxbf_tmfifo_virtio_release_desc(vdev, vr, desc_head,
> pkt_len);
> +
> + if (desc != NULL)
> + *desc = NULL;
> + vring->pkt_len = 0;
> +}
> +
> +/* House-keeping timer. */
> +static void mlxbf_tmfifo_timer(struct timer_list *arg) {
> + struct mlxbf_tmfifo *fifo;
> +
> + fifo = container_of(arg, struct mlxbf_tmfifo, timer);
> +
> + /*
> + * Wake up the work handler to poll the Rx FIFO in case interrupt
> + * missing or any leftover bytes stuck in the FIFO.
> + */
> + test_and_set_bit(MLXBF_TM_RX_HWM_IRQ, &fifo->pend_events);
> +
> + /*
> + * Wake up Tx handler in case virtio has queued too many packets
> + * and are waiting for buffer return.
> + */
> + test_and_set_bit(MLXBF_TM_TX_LWM_IRQ, &fifo->pend_events);
> +
> + schedule_work(&fifo->work);
> +
> + mod_timer(&fifo->timer, jiffies + mlxbf_tmfifo_timer_interval); }
> +
> +/* Buffer the console output. */
> +static void mlxbf_tmfifo_console_output(struct mlxbf_tmfifo_vdev *cons,
> + struct virtqueue *vq)
> +{
> + struct vring *vr = (struct vring *)virtqueue_get_vring(vq);
> + struct vring_desc *head_desc, *desc = NULL;
> + struct virtio_device *vdev = &cons->vdev;
> + u32 len, pkt_len, idx;
> + void *addr;
> +
> + for (;;) {
It's better to modify it as while(on some condition)
> + head_desc = mlxbf_tmfifo_virtio_get_next_desc(vq);
> + if (head_desc == NULL)
> + break;
> +
> + /* Release the packet if no more space. */
> + pkt_len = mlxbf_tmfifo_virtio_get_pkt_len(vdev, head_desc,
> vr);
> + if (pkt_len > mlxbf_tmfifo_vdev_tx_buf_avail(cons)) {
> + mlxbf_tmfifo_virtio_release_desc(vdev, vr, head_desc,
> + pkt_len);
Why do you break line here?
> + break;
> + }
> +
> + desc = head_desc;
> +
> + while (desc != NULL) {
> + addr = phys_to_virt(virtio64_to_cpu(vdev, desc->addr));
> + len = virtio32_to_cpu(vdev, desc->len);
> +
> + if (len <= MLXBF_TMFIFO_CONS_TX_BUF_SIZE -
> + cons->tx_tail) {
Why do you break line here? Also below I see few strange breaks.
> + memcpy(cons->tx_buf + cons->tx_tail, addr,
> len);
> + } else {
> + u32 seg;
> +
> + seg = MLXBF_TMFIFO_CONS_TX_BUF_SIZE -
> + cons->tx_tail;
> + memcpy(cons->tx_buf + cons->tx_tail, addr,
> seg);
> + addr += seg;
> + memcpy(cons->tx_buf, addr, len - seg);
> + }
> + mlxbf_tmfifo_vdev_tx_buf_push(cons, len);
> +
> + if (!(virtio16_to_cpu(vdev, desc->flags) &
> + VRING_DESC_F_NEXT))
> + break;
> + idx = virtio16_to_cpu(vdev, desc->next);
> + desc = &vr->desc[idx];
> + }
> +
> + mlxbf_tmfifo_virtio_release_desc(vdev, vr, head_desc,
> pkt_len);
> + }
> +}
> +
> +/* Rx & Tx processing of a virtual queue. */ static void
> +mlxbf_tmfifo_virtio_rxtx(struct virtqueue *vq, bool is_rx) {
> + int num_avail = 0, hdr_len, tx_reserve;
> + struct mlxbf_tmfifo_vring *vring;
> + struct mlxbf_tmfifo_vdev *cons;
> + struct virtio_device *vdev;
> + struct mlxbf_tmfifo *fifo;
> + struct vring_desc *desc;
> + unsigned long flags;
> + struct vring *vr;
> + u64 sts, data;
> + u32 len, idx;
> + void *addr;
> +
> + if (!vq)
> + return;
> +
> + vring = (struct mlxbf_tmfifo_vring *)vq->priv;
> + fifo = vring->fifo;
> + vr = (struct vring *)virtqueue_get_vring(vq);
> +
> + if (!fifo->vdev[vring->vdev_id])
> + return;
> + vdev = &fifo->vdev[vring->vdev_id]->vdev;
> + cons = fifo->vdev[VIRTIO_ID_CONSOLE];
> +
> + /* Don't continue if another vring is running. */
> + if (fifo->vring[is_rx] != NULL && fifo->vring[is_rx] != vring)
> + return;
> +
> + /* tx_reserve is used to reserved some room in FIFO for console. */
> + if (vring->vdev_id == VIRTIO_ID_NET) {
> + hdr_len = sizeof(struct virtio_net_hdr);
> + tx_reserve = fifo->tx_fifo_size / 16;
Use some define instead of 16/
> + } else {
> + BUG_ON(vring->vdev_id != VIRTIO_ID_CONSOLE);
> + hdr_len = 0;
> + tx_reserve = 1;
> + }
> +
> + desc = vring->desc;
> +
> + while (1) {
I see there are few drivers in platform which use while (1)
But it looks better to use while(some condition)
and instead of break change this condition to false.
> + /* Get available FIFO space. */
> + if (num_avail == 0) {
> + if (is_rx) {
> + /* Get the number of available words in FIFO.
> */
> + sts = readq(fifo->rx_base +
> + MLXBF_TMFIFO_RX_STS);
> + num_avail = FIELD_GET(
> +
> MLXBF_TMFIFO_RX_STS__COUNT_MASK, sts);
num_avail = FIELD_GET(TMFIFO_RX_STS__COUNT_MASK, sts);
> +
> + /* Don't continue if nothing in FIFO. */
> + if (num_avail <= 0)
> + break;
> + } else {
> + /* Get available space in FIFO. */
> + sts = readq(fifo->tx_base +
> + MLXBF_TMFIFO_TX_STS);
> + num_avail = fifo->tx_fifo_size - tx_reserve -
> + FIELD_GET(
> +
> MLXBF_TMFIFO_TX_STS__COUNT_MASK,
> + sts);
Same as above.
> +
> + if (num_avail <= 0)
> + break;
> + }
> + }
> +
> + /* Console output always comes from the Tx buffer. */
> + if (!is_rx && vring->vdev_id == VIRTIO_ID_CONSOLE &&
> + cons != NULL && cons->tx_buf != NULL) {
> + union mlxbf_tmfifo_msg_hdr hdr;
> + int size;
> +
> + size = mlxbf_tmfifo_vdev_tx_buf_len(cons);
> + if (num_avail < 2 || size == 0)
> + return;
> + if (size + sizeof(hdr) > num_avail * sizeof(u64))
> + size = num_avail * sizeof(u64) - sizeof(hdr);
> + /* Write header. */
> + hdr.data = 0;
> + hdr.type = VIRTIO_ID_CONSOLE;
> + hdr.len = htons(size);
> + writeq(cpu_to_le64(hdr.data),
> + fifo->tx_base + MLXBF_TMFIFO_TX_DATA);
> +
> + spin_lock_irqsave(&fifo->spin_lock, flags);
> + while (size > 0) {
> + addr = cons->tx_buf + cons->tx_head;
> +
> + if (cons->tx_head + sizeof(u64) <=
> + MLXBF_TMFIFO_CONS_TX_BUF_SIZE) {
> + memcpy(&data, addr, sizeof(u64));
> + } else {
> + int partial;
> +
> + partial =
> +
> MLXBF_TMFIFO_CONS_TX_BUF_SIZE -
> + cons->tx_head;
> +
> + memcpy(&data, addr, partial);
> + memcpy((u8 *)&data + partial,
> + cons->tx_buf,
> + sizeof(u64) - partial);
> + }
> + writeq(data,
> + fifo->tx_base + MLXBF_TMFIFO_TX_DATA);
> +
> + if (size >= sizeof(u64)) {
> + mlxbf_tmfifo_vdev_tx_buf_pop(
> + cons, sizeof(u64));
> + size -= sizeof(u64);
> + } else {
> + mlxbf_tmfifo_vdev_tx_buf_pop(
> + cons, size);
> + size = 0;
> + }
> + }
> + spin_unlock_irqrestore(&fifo->spin_lock, flags);
> + return;
> + }
> +
> + /* Get the desc of next packet. */
> + if (!desc) {
> + /* Save the head desc of the chain. */
> + vring->desc_head =
> + mlxbf_tmfifo_virtio_get_next_desc(vq);
> + if (!vring->desc_head) {
> + vring->desc = NULL;
> + return;
> + }
> + desc = vring->desc_head;
> + vring->desc = desc;
> +
> + if (is_rx && vring->vdev_id == VIRTIO_ID_NET) {
> + struct virtio_net_hdr *net_hdr;
> +
> + /* Initialize the packet header. */
> + net_hdr = (struct virtio_net_hdr *)
> + phys_to_virt(virtio64_to_cpu(
> + vdev, desc->addr));
> + memset(net_hdr, 0, sizeof(*net_hdr));
> + }
> + }
> +
> + /* Beginning of each packet. */
> + if (vring->pkt_len == 0) {
> + int vdev_id, vring_change = 0;
> + union mlxbf_tmfifo_msg_hdr hdr;
> +
> + num_avail--;
> +
> + /* Read/Write packet length. */
> + if (is_rx) {
> + hdr.data = readq(fifo->rx_base +
> + MLXBF_TMFIFO_RX_DATA);
> + hdr.data = le64_to_cpu(hdr.data);
> +
> + /* Skip the length 0 packet (keepalive). */
> + if (hdr.len == 0)
> + continue;
> +
> + /* Check packet type. */
> + if (hdr.type == VIRTIO_ID_NET) {
> + struct virtio_net_config *config;
> +
> + vdev_id = VIRTIO_ID_NET;
> + hdr_len = sizeof(struct virtio_net_hdr);
> + config =
> + &fifo->vdev[vdev_id]->config.net;
> + if (ntohs(hdr.len) > config->mtu +
> +
> MLXBF_TMFIFO_NET_L2_OVERHEAD)
> + continue;
> + } else if (hdr.type == VIRTIO_ID_CONSOLE) {
> + vdev_id = VIRTIO_ID_CONSOLE;
> + hdr_len = 0;
> + } else {
> + continue;
> + }
> +
> + /*
> + * Check whether the new packet still belongs
> + * to this vring or not. If not, update the
> + * pkt_len of the new vring and return.
> + */
> + if (vdev_id != vring->vdev_id) {
> + struct mlxbf_tmfifo_vdev *dev2 =
> + fifo->vdev[vdev_id];
> +
> + if (!dev2)
> + break;
> + vring->desc = desc;
> + vring =
> + &dev2-
> >vrings[MLXBF_TMFIFO_VRING_RX];
> + vring_change = 1;
> + }
> + vring->pkt_len = ntohs(hdr.len) + hdr_len;
> + } else {
> + vring->pkt_len =
> + mlxbf_tmfifo_virtio_get_pkt_len(
> + vdev, desc, vr);
> +
> + hdr.data = 0;
> + hdr.type = (vring->vdev_id == VIRTIO_ID_NET) ?
> + VIRTIO_ID_NET :
> + VIRTIO_ID_CONSOLE;
> + hdr.len = htons(vring->pkt_len - hdr_len);
> + writeq(cpu_to_le64(hdr.data),
> + fifo->tx_base + MLXBF_TMFIFO_TX_DATA);
> + }
> +
> + vring->cur_len = hdr_len;
> + vring->rem_len = vring->pkt_len;
> + fifo->vring[is_rx] = vring;
> +
> + if (vring_change)
> + return;
> + continue;
> + }
> +
> + /* Check available space in this desc. */
> + len = virtio32_to_cpu(vdev, desc->len);
> + if (len > vring->rem_len)
> + len = vring->rem_len;
> +
> + /* Check if the current desc is already done. */
> + if (vring->cur_len == len)
> + goto check_done;
> +
> + addr = phys_to_virt(virtio64_to_cpu(vdev, desc->addr));
> +
> + /* Read a word from FIFO for Rx. */
> + if (is_rx) {
> + data = readq(fifo->rx_base +
> MLXBF_TMFIFO_RX_DATA);
> + data = le64_to_cpu(data);
> + }
> +
> + if (vring->cur_len + sizeof(u64) <= len) {
> + /* The whole word. */
> + if (is_rx) {
> + memcpy(addr + vring->cur_len, &data,
> + sizeof(u64));
> + } else {
> + memcpy(&data, addr + vring->cur_len,
> + sizeof(u64));
> + }
Why not just.
Also few places like this one below.
if (is_rx)
memcpy(addr + vring->cur_len, &data, sizeof(u64));
else
memcpy(&data, addr + vring->cur_len, sizeof(u64));
> + vring->cur_len += sizeof(u64);
> + } else {
> + /* Leftover bytes. */
> + BUG_ON(vring->cur_len > len);
> + if (is_rx) {
> + memcpy(addr + vring->cur_len, &data,
> + len - vring->cur_len);
> + } else {
> + memcpy(&data, addr + vring->cur_len,
> + len - vring->cur_len);
> + }
> + vring->cur_len = len;
> + }
> +
> + /* Write the word into FIFO for Tx. */
> + if (!is_rx) {
> + writeq(cpu_to_le64(data),
> + fifo->tx_base + MLXBF_TMFIFO_TX_DATA);
> + }
> +
> + num_avail--;
> +
> +check_done:
> + /* Check whether this desc is full or completed. */
> + if (vring->cur_len == len) {
> + vring->cur_len = 0;
> + vring->rem_len -= len;
> +
> + /* Get the next desc on the chain. */
> + if (vring->rem_len > 0 &&
> + (virtio16_to_cpu(vdev, desc->flags) &
> + VRING_DESC_F_NEXT)) {
> + idx = virtio16_to_cpu(vdev, desc->next);
> + desc = &vr->desc[idx];
> + continue;
> + }
> +
> + /* Done and release the desc. */
> + mlxbf_tmfifo_release_pkt(vdev, vring, &desc);
> + fifo->vring[is_rx] = NULL;
> +
> + /* Notify upper layer that packet is done. */
> + spin_lock_irqsave(&fifo->spin_lock, flags);
> + vring_interrupt(0, vq);
> + spin_unlock_irqrestore(&fifo->spin_lock, flags);
> + continue;
> + }
> + }
> +
> + /* Save the current desc. */
> + vring->desc = desc;
> +}
I suggest to split mlxbf_tmfifo_virtio_rxtx() into few small routines.
> +
> +/* The notify function is called when new buffers are posted. */ static
> +bool mlxbf_tmfifo_virtio_notify(struct virtqueue *vq) {
> + struct mlxbf_tmfifo_vring *vring;
> + struct mlxbf_tmfifo *fifo;
> + unsigned long flags;
> +
> + vring = (struct mlxbf_tmfifo_vring *)vq->priv;
> + fifo = vring->fifo;
> +
> + /*
> + * Virtio maintains vrings in pairs, even number ring for Rx
> + * and odd number ring for Tx.
> + */
> + if (!(vring->id & 1)) {
> + /* Set the RX HWM bit to start Rx. */
> + if (!test_and_set_bit(MLXBF_TM_RX_HWM_IRQ, &fifo-
> >pend_events))
> + schedule_work(&fifo->work);
> + } else {
> + /*
> + * Console could make blocking call with interrupts disabled.
> + * In such case, the vring needs to be served right away. For
> + * other cases, just set the TX LWM bit to start Tx in the
> + * worker handler.
> + */
> + if (vring->vdev_id == VIRTIO_ID_CONSOLE) {
> + spin_lock_irqsave(&fifo->spin_lock, flags);
> + mlxbf_tmfifo_console_output(
> + fifo->vdev[VIRTIO_ID_CONSOLE], vq);
mlxbf_tmfifo_console_output(fifo->vdev[VIRTIO_ID_CONSOLE], vq);
> + spin_unlock_irqrestore(&fifo->spin_lock, flags);
> + schedule_work(&fifo->work);
> + } else if (!test_and_set_bit(MLXBF_TM_TX_LWM_IRQ,
> + &fifo->pend_events))
> + schedule_work(&fifo->work);
If {
} else if {
}
For consistency.
> + }
> +
> + return true;
> +}
> +
> +/* Work handler for Rx and Tx case. */
> +static void mlxbf_tmfifo_work_handler(struct work_struct *work) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> + struct mlxbf_tmfifo *fifo;
> + int i;
> +
> + fifo = container_of(work, struct mlxbf_tmfifo, work);
> + if (!fifo->is_ready)
> + return;
> +
> + mutex_lock(&fifo->lock);
> +
> + /* Tx (Send data to the TmFifo). */
> + if (test_and_clear_bit(MLXBF_TM_TX_LWM_IRQ, &fifo->pend_events)
> &&
> + fifo->irq_info[MLXBF_TM_TX_LWM_IRQ].irq) {
> + for (i = 0; i < MLXBF_TMFIFO_VDEV_MAX; i++) {
I suggest to define local variable vq.
And have below:
mlxbf_tmfifo_virtio_rxtx(vq, false);
> + tm_vdev = fifo->vdev[i];
> + if (tm_vdev != NULL) {
> + mlxbf_tmfifo_virtio_rxtx(
> + tm_vdev-
> >vrings[MLXBF_TMFIFO_VRING_TX].vq,
> + false);
> + }
> + }
> + }
> +
> + /* Rx (Receive data from the TmFifo). */
> + if (test_and_clear_bit(MLXBF_TM_RX_HWM_IRQ, &fifo->pend_events)
> &&
> + fifo->irq_info[MLXBF_TM_RX_HWM_IRQ].irq) {
> + for (i = 0; i < MLXBF_TMFIFO_VDEV_MAX; i++) {
> + tm_vdev = fifo->vdev[i];
Same as above.
> + if (tm_vdev != NULL) {
> + mlxbf_tmfifo_virtio_rxtx(
> + tm_vdev-
> >vrings[MLXBF_TMFIFO_VRING_RX].vq,
> + true);
> + }
> + }
> + }
> +
> + mutex_unlock(&fifo->lock);
> +}
> +
> +/* Get the array of feature bits for this device. */ static u64
> +mlxbf_tmfifo_virtio_get_features(struct virtio_device *vdev) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> + return tm_vdev->features;
> +}
> +
> +/* Confirm device features to use. */
> +static int mlxbf_tmfifo_virtio_finalize_features(struct virtio_device
> +*vdev) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> + tm_vdev->features = vdev->features;
> +
> + return 0;
> +}
> +
> +/* Free virtqueues found by find_vqs(). */ static void
> +mlxbf_tmfifo_virtio_del_vqs(struct virtio_device *vdev) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> + struct mlxbf_tmfifo_vring *vring;
> + struct virtqueue *vq;
> + int i;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> +
> + for (i = 0; i < ARRAY_SIZE(tm_vdev->vrings); i++) {
> + vring = &tm_vdev->vrings[i];
> +
> + /* Release the pending packet. */
> + if (vring->desc != NULL) {
> + mlxbf_tmfifo_release_pkt(&tm_vdev->vdev, vring,
> + &vring->desc);
> + }
> +
> + vq = vring->vq;
> + if (vq) {
> + vring->vq = NULL;
> + vring_del_virtqueue(vq);
> + }
> + }
> +}
> +
> +/* Create and initialize the virtual queues. */ static int
> +mlxbf_tmfifo_virtio_find_vqs(struct virtio_device *vdev,
> + unsigned int nvqs,
> + struct virtqueue *vqs[],
> + vq_callback_t *callbacks[],
> + const char * const names[],
> + const bool *ctx,
> + struct irq_affinity *desc)
> +{
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> + struct mlxbf_tmfifo_vring *vring;
> + int i, ret = -EINVAL, size;
Don't initialize ret with -EINVAL.
> + struct virtqueue *vq;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> + if (nvqs > ARRAY_SIZE(tm_vdev->vrings))
> + return -EINVAL;
> +
> + for (i = 0; i < nvqs; ++i) {
> + if (!names[i])
> + goto error;
> + vring = &tm_vdev->vrings[i];
> +
> + /* zero vring */
> + size = vring_size(vring->size, vring->align);
> + memset(vring->va, 0, size);
> + vq = vring_new_virtqueue(i, vring->size, vring->align, vdev,
> + false, false, vring->va,
> + mlxbf_tmfifo_virtio_notify,
> + callbacks[i], names[i]);
> + if (!vq) {
> + dev_err(&vdev->dev, "vring_new_virtqueue failed\n");
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + vqs[i] = vq;
> + vring->vq = vq;
> + vq->priv = vring;
> + }
> +
> + return 0;
> +
> +error:
> + mlxbf_tmfifo_virtio_del_vqs(vdev);
> + return ret;
> +}
> +
> +/* Read the status byte. */
> +static u8 mlxbf_tmfifo_virtio_get_status(struct virtio_device *vdev) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> +
> + return tm_vdev->status;
> +}
> +
> +/* Write the status byte. */
> +static void mlxbf_tmfifo_virtio_set_status(struct virtio_device *vdev,
> + u8 status)
> +{
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> + tm_vdev->status = status;
> +}
> +
> +/* Reset the device. Not much here for now. */ static void
> +mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> + tm_vdev->status = 0;
> +}
> +
> +/* Read the value of a configuration field. */ static void
> +mlxbf_tmfifo_virtio_get(struct virtio_device *vdev,
> + unsigned int offset,
> + void *buf,
> + unsigned int len)
> +{
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> +
unsigned int pos = offset + len;
if (pos > sizeof(tm_vdev->config) || pos < len)
> + if (offset + len > sizeof(tm_vdev->config) || offset + len < len) {
> + dev_err(vdev->dev.parent, "virtio_get access out of bounds\n");
> + return;
> + }
> +
> + memcpy(buf, (u8 *)&tm_vdev->config + offset, len); }
> +
> +/* Write the value of a configuration field. */ static void
> +mlxbf_tmfifo_virtio_set(struct virtio_device *vdev,
> + unsigned int offset,
> + const void *buf,
> + unsigned int len)
> +{
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + tm_vdev = container_of(vdev, struct mlxbf_tmfifo_vdev, vdev);
> +
> + if (offset + len > sizeof(tm_vdev->config) || offset + len < len) {
Same as above.
> + dev_err(vdev->dev.parent, "virtio_get access out of bounds\n");
> + return;
> + }
> +
> + memcpy((u8 *)&tm_vdev->config + offset, buf, len); }
> +
> +/* Virtio config operations. */
> +static const struct virtio_config_ops mlxbf_tmfifo_virtio_config_ops = {
> + .get_features = mlxbf_tmfifo_virtio_get_features,
> + .finalize_features = mlxbf_tmfifo_virtio_finalize_features,
> + .find_vqs = mlxbf_tmfifo_virtio_find_vqs,
> + .del_vqs = mlxbf_tmfifo_virtio_del_vqs,
> + .reset = mlxbf_tmfifo_virtio_reset,
> + .set_status = mlxbf_tmfifo_virtio_set_status,
> + .get_status = mlxbf_tmfifo_virtio_get_status,
> + .get = mlxbf_tmfifo_virtio_get,
> + .set = mlxbf_tmfifo_virtio_set,
> +};
> +
> +/* Create vdev type in a tmfifo. */
> +int mlxbf_tmfifo_create_vdev(struct mlxbf_tmfifo *fifo, int vdev_id,
> + u64 features, void *config, u32 size) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> + int ret = 0;
> +
> + mutex_lock(&fifo->lock);
> +
> + tm_vdev = fifo->vdev[vdev_id];
> + if (tm_vdev != NULL) {
> + pr_err("vdev %d already exists\n", vdev_id);
> + ret = -EEXIST;
> + goto already_exist;
> + }
> +
> + tm_vdev = kzalloc(sizeof(*tm_vdev), GFP_KERNEL);
> + if (!tm_vdev) {
> + ret = -ENOMEM;
> + goto already_exist;
> + }
> +
> + tm_vdev->vdev.id.device = vdev_id;
> + tm_vdev->vdev.config = &mlxbf_tmfifo_virtio_config_ops;
> + tm_vdev->vdev.dev.parent = &fifo->pdev->dev;
> + tm_vdev->vdev.dev.release = mlxbf_tmfifo_virtio_dev_release;
> + tm_vdev->features = features;
> + if (config)
> + memcpy(&tm_vdev->config, config, size);
> + if (mlxbf_tmfifo_alloc_vrings(fifo, tm_vdev, vdev_id)) {
> + pr_err("Unable to allocate vring\n");
> + ret = -ENOMEM;
> + goto alloc_vring_fail;
> + }
> + if (vdev_id == VIRTIO_ID_CONSOLE) {
> + tm_vdev->tx_buf =
> kmalloc(MLXBF_TMFIFO_CONS_TX_BUF_SIZE,
> + GFP_KERNEL);
> + }
> + fifo->vdev[vdev_id] = tm_vdev;
> +
> + /* Register the virtio device. */
> + ret = register_virtio_device(&tm_vdev->vdev);
> + if (ret) {
> + dev_err(&fifo->pdev->dev, "register_virtio_device() failed\n");
> + goto register_fail;
> + }
> +
> + mutex_unlock(&fifo->lock);
> + return 0;
> +
> +register_fail:
> + mlxbf_tmfifo_free_vrings(fifo, vdev_id);
> + fifo->vdev[vdev_id] = NULL;
> +alloc_vring_fail:
> + kfree(tm_vdev);
> +already_exist:
> + mutex_unlock(&fifo->lock);
> + return ret;
> +}
> +
> +/* Delete vdev type from a tmfifo. */
> +int mlxbf_tmfifo_delete_vdev(struct mlxbf_tmfifo *fifo, int vdev_id) {
> + struct mlxbf_tmfifo_vdev *tm_vdev;
> +
> + mutex_lock(&fifo->lock);
> +
> + /* Unregister vdev. */
> + tm_vdev = fifo->vdev[vdev_id];
> + if (tm_vdev) {
> + unregister_virtio_device(&tm_vdev->vdev);
> + mlxbf_tmfifo_free_vrings(fifo, vdev_id);
> + kfree(tm_vdev->tx_buf);
> + kfree(tm_vdev);
> + fifo->vdev[vdev_id] = NULL;
> + }
> +
> + mutex_unlock(&fifo->lock);
> +
> + return 0;
> +}
> +
> +/* Device remove function. */
> +static int mlxbf_tmfifo_remove(struct platform_device *pdev) {
Locate it after probe.
If you'll use all devm_, like Andy noted:
devm_ioremap
devm_ioremap_resource
devm_kzalloc
devm_request_mem_region
you can drop all kfree, release_mem_region, iounmap
And make the below as a separate routine, something like
mlxbf_tmfifo_cleanup(), if you still need it.
> + struct mlxbf_tmfifo *fifo = platform_get_drvdata(pdev);
> + struct resource *rx_res, *tx_res;
> + int i;
> +
> + if (fifo) {
> + mutex_lock(&mlxbf_tmfifo_lock);
> +
> + fifo->is_ready = false;
> +
> + /* Stop the timer. */
> + del_timer_sync(&fifo->timer);
> +
> + /* Release interrupts. */
> + mlxbf_tmfifo_free_irqs(fifo);
> +
> + /* Cancel the pending work. */
> + cancel_work_sync(&fifo->work);
> +
> + for (i = 0; i < MLXBF_TMFIFO_VDEV_MAX; i++)
> + mlxbf_tmfifo_delete_vdev(fifo, i);
> +
> + /* Release IO resources. */
> + if (fifo->rx_base)
> + iounmap(fifo->rx_base);
> + if (fifo->tx_base)
> + iounmap(fifo->tx_base);
> +
> + platform_set_drvdata(pdev, NULL);
> + kfree(fifo);
> +
> + mutex_unlock(&mlxbf_tmfifo_lock);
> + }
> +
> + rx_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (rx_res)
> + release_mem_region(rx_res->start, resource_size(rx_res));
> + tx_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (tx_res)
> + release_mem_region(tx_res->start, resource_size(tx_res));
> +
> + return 0;
> +}
> +
> +/* Read the configured network MAC address from efi variable. */ static
> +void mlxbf_tmfifo_get_cfg_mac(u8 *mac) {
> + efi_char16_t name[] = {
> + 'R', 's', 'h', 'i', 'm', 'M', 'a', 'c', 'A', 'd', 'd', 'r', 0 };
Could it be moved out and set like:
static const efi_char16_t mlxbf_tmfifo_efi_name[] = "...";
Could you check if the are some examples in kernel, please?
> + efi_guid_t guid = EFI_GLOBAL_VARIABLE_GUID;
> + efi_status_t status;
> + unsigned long size;
> + u8 buf[6];
> +
> + size = sizeof(buf);
> + status = efi.get_variable(name, &guid, NULL, &size, buf);
> + if (status == EFI_SUCCESS && size == sizeof(buf))
> + memcpy(mac, buf, sizeof(buf));
> +}
> +
> +/* Probe the TMFIFO. */
> +static int mlxbf_tmfifo_probe(struct platform_device *pdev) {
> + struct virtio_net_config net_config;
> + struct resource *rx_res, *tx_res;
> + struct mlxbf_tmfifo *fifo;
> + int i, ret;
> + u64 ctl;
> +
> + /* Get the resource of the Rx & Tx FIFO. */
> + rx_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + tx_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!rx_res || !tx_res) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + if (request_mem_region(rx_res->start,
> + resource_size(rx_res), "bf-tmfifo") == NULL) {
> + ret = -EBUSY;
> + goto early_err;
> + }
> +
> + if (request_mem_region(tx_res->start,
> + resource_size(tx_res), "bf-tmfifo") == NULL) {
> + release_mem_region(rx_res->start, resource_size(rx_res));
> + ret = -EBUSY;
> + goto early_err;
> + }
> +
> + ret = -ENOMEM;
> + fifo = kzalloc(sizeof(struct mlxbf_tmfifo), GFP_KERNEL);
> + if (!fifo)
> + goto err;
> +
> + fifo->pdev = pdev;
> + platform_set_drvdata(pdev, fifo);
> +
> + spin_lock_init(&fifo->spin_lock);
> + INIT_WORK(&fifo->work, mlxbf_tmfifo_work_handler);
> +
> + timer_setup(&fifo->timer, mlxbf_tmfifo_timer, 0);
> + fifo->timer.function = mlxbf_tmfifo_timer;
> +
> + for (i = 0; i < MLXBF_TM_IRQ_CNT; i++) {
> + fifo->irq_info[i].index = i;
> + fifo->irq_info[i].fifo = fifo;
> + fifo->irq_info[i].irq = platform_get_irq(pdev, i);
> + ret = request_irq(fifo->irq_info[i].irq,
> + mlxbf_tmfifo_irq_handler, 0,
> + "tmfifo", &fifo->irq_info[i]);
> + if (ret) {
> + pr_err("Unable to request irq\n");
> + fifo->irq_info[i].irq = 0;
> + goto err;
> + }
> + }
> +
> + fifo->rx_base = ioremap(rx_res->start, resource_size(rx_res));
> + if (!fifo->rx_base)
> + goto err;
> +
> + fifo->tx_base = ioremap(tx_res->start, resource_size(tx_res));
> + if (!fifo->tx_base)
> + goto err;
> +
> + /* Get Tx FIFO size and set the low/high watermark. */
> + ctl = readq(fifo->tx_base + MLXBF_TMFIFO_TX_CTL);
> + fifo->tx_fifo_size =
> + FIELD_GET(MLXBF_TMFIFO_TX_CTL__MAX_ENTRIES_MASK,
> ctl);
> + ctl = (ctl & ~MLXBF_TMFIFO_TX_CTL__LWM_MASK) |
> + FIELD_PREP(MLXBF_TMFIFO_TX_CTL__LWM_MASK,
> + fifo->tx_fifo_size / 2);
> + ctl = (ctl & ~MLXBF_TMFIFO_TX_CTL__HWM_MASK) |
> + FIELD_PREP(MLXBF_TMFIFO_TX_CTL__HWM_MASK,
> + fifo->tx_fifo_size - 1);
> + writeq(ctl, fifo->tx_base + MLXBF_TMFIFO_TX_CTL);
> +
> + /* Get Rx FIFO size and set the low/high watermark. */
> + ctl = readq(fifo->rx_base + MLXBF_TMFIFO_RX_CTL);
> + fifo->rx_fifo_size =
> + FIELD_GET(MLXBF_TMFIFO_RX_CTL__MAX_ENTRIES_MASK,
> ctl);
> + ctl = (ctl & ~MLXBF_TMFIFO_RX_CTL__LWM_MASK) |
> + FIELD_PREP(MLXBF_TMFIFO_RX_CTL__LWM_MASK, 0);
> + ctl = (ctl & ~MLXBF_TMFIFO_RX_CTL__HWM_MASK) |
> + FIELD_PREP(MLXBF_TMFIFO_RX_CTL__HWM_MASK, 1);
> + writeq(ctl, fifo->rx_base + MLXBF_TMFIFO_RX_CTL);
> +
> + mutex_init(&fifo->lock);
> +
> + /* Create the console vdev. */
> + ret = mlxbf_tmfifo_create_vdev(fifo, VIRTIO_ID_CONSOLE, 0, NULL, 0);
> + if (ret)
> + goto err;
> +
> + /* Create the network vdev. */
> + memset(&net_config, 0, sizeof(net_config));
> + net_config.mtu = MLXBF_TMFIFO_NET_MTU;
> + net_config.status = VIRTIO_NET_S_LINK_UP;
> + memcpy(net_config.mac, mlxbf_tmfifo_net_default_mac, 6);
> + mlxbf_tmfifo_get_cfg_mac(net_config.mac);
> + ret = mlxbf_tmfifo_create_vdev(fifo, VIRTIO_ID_NET,
> + MLXBF_TMFIFO_NET_FEATURES, &net_config,
> sizeof(net_config));
> + if (ret)
> + goto err;
> +
> + mod_timer(&fifo->timer, jiffies + mlxbf_tmfifo_timer_interval);
> +
> + fifo->is_ready = true;
> +
> + return 0;
> +
> +err:
> + mlxbf_tmfifo_remove(pdev);
> +early_err:
> + dev_err(&pdev->dev, "Probe Failed\n");
> + return ret;
> +}
> +
> +static const struct of_device_id mlxbf_tmfifo_match[] = {
> + { .compatible = "mellanox,bf-tmfifo" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mlxbf_tmfifo_match);
> +
> +static const struct acpi_device_id mlxbf_tmfifo_acpi_match[] = {
> + { "MLNXBF01", 0 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, mlxbf_tmfifo_acpi_match);
> +
> +static struct platform_driver mlxbf_tmfifo_driver = {
> + .probe = mlxbf_tmfifo_probe,
> + .remove = mlxbf_tmfifo_remove,
> + .driver = {
> + .name = "bf-tmfifo",
> + .of_match_table = mlxbf_tmfifo_match,
> + .acpi_match_table = ACPI_PTR(mlxbf_tmfifo_acpi_match),
> + },
> +};
> +
> +module_platform_driver(mlxbf_tmfifo_driver);
> +
> +MODULE_DESCRIPTION("Mellanox BlueField SoC TmFifo Driver");
> +MODULE_LICENSE("GPL"); MODULE_AUTHOR("Mellanox Technologies");
> --
> 1.8.3.1