This driver supports Socket CANfd functionality for NCT6694 MFD
device based on USB interface.
Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
---
MAINTAINERS | 1 +
drivers/net/can/Kconfig | 10 +
drivers/net/can/Makefile | 1 +
drivers/net/can/nct6694_canfd.c | 920 ++++++++++++++++++++++++++++++++
4 files changed, 932 insertions(+)
create mode 100644 drivers/net/can/nct6694_canfd.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a190f2b08fa3..eb5d46825e71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16548,6 +16548,7 @@ S: Supported
F: drivers/gpio/gpio-nct6694.c
F: drivers/i2c/busses/i2c-nct6694.c
F: drivers/mfd/nct6694.c
+F: drivers/net/can/nct6694_canfd.c
F: include/linux/mfd/nct6694.h
NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index cf989bea9aa3..130e98ec28a5 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -200,6 +200,16 @@ config CAN_SUN4I
To compile this driver as a module, choose M here: the module will
be called sun4i_can.
+config CAN_NCT6694
+ tristate "Nuvoton NCT6694 Socket CANfd support"
+ depends on MFD_NCT6694
+ help
+ If you say yes to this option, support will be included for Nuvoton
+ NCT6694, a USB device to socket CANfd controller.
+
+ This driver can also be built as a module. If so, the module will
+ be called nct6694_canfd.
+
config CAN_TI_HECC
depends on ARM
tristate "TI High End CAN Controller"
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index a71db2cfe990..4a6b5b9d6c2b 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o
obj-$(CONFIG_CAN_MSCAN) += mscan/
obj-$(CONFIG_CAN_M_CAN) += m_can/
+obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o
obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
obj-$(CONFIG_CAN_SJA1000) += sja1000/
obj-$(CONFIG_CAN_SUN4I) += sun4i_can.o
diff --git a/drivers/net/can/nct6694_canfd.c b/drivers/net/can/nct6694_canfd.c
new file mode 100644
index 000000000000..54f20f0681e2
--- /dev/null
+++ b/drivers/net/can/nct6694_canfd.c
@@ -0,0 +1,920 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NCT6694 Socket CANfd driver based on USB interface.
+ *
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#include <linux/can/dev.h>
+#include <linux/ethtool.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nct6694.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+
+#define DRVNAME "nct6694-can"
+
+/* Host interface */
+#define NCT6694_CAN_MOD 0x05
+
+/* Message Channel*/
+/* Command 00h */
+#define NCT6694_CAN_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000)
+#define NCT6694_CAN_CTRL1_MON BIT(0)
+#define NCT6694_CAN_CTRL1_NISO BIT(1)
+#define NCT6694_CAN_CTRL1_LBCK BIT(2)
+
+/* Command 01h */
+#define NCT6694_CAN_CMD1_OFFSET 0x0001
+
+/* Command 02h */
+#define NCT6694_CAN_CMD2_OFFSET(idx, mask) \
+ ({ typeof(mask) mask_ = (mask); \
+ idx ? ((0x80 | (mask_ & 0xFF)) << 8 | 0x02) : \
+ ((0x00 | (mask_ & 0xFF)) << 8 | 0x02); })
+
+#define NCT6694_CAN_EVENT_ERR BIT(0)
+#define NCT6694_CAN_EVENT_STATUS BIT(1)
+#define NCT6694_CAN_EVENT_TX_EVT BIT(2)
+#define NCT6694_CAN_EVENT_RX_EVT BIT(3)
+#define NCT6694_CAN_EVENT_REC BIT(4)
+#define NCT6694_CAN_EVENT_TEC BIT(5)
+#define NCT6694_CAN_EVT_TX_FIFO_EMPTY BIT(7) /* Read-clear */
+#define NCT6694_CAN_EVT_RX_DATA_LOST BIT(5) /* Read-clear */
+#define NCT6694_CAN_EVT_RX_HALF_FULL BIT(6) /* Read-clear */
+#define NCT6694_CAN_EVT_RX_DATA_IN BIT(7)
+
+/* Command 10h */
+#define NCT6694_CAN_CMD10_OFFSET(buf_cnt) \
+ (((buf_cnt) & 0xFF) << 8 | 0x10)
+#define NCT6694_CAN_TAG_CAN0 0xC0
+#define NCT6694_CAN_TAG_CAN1 0xC1
+#define NCT6694_CAN_FLAG_EFF BIT(0)
+#define NCT6694_CAN_FLAG_RTR BIT(1)
+#define NCT6694_CAN_FLAG_FD BIT(2)
+#define NCT6694_CAN_FLAG_BRS BIT(3)
+#define NCT6694_CAN_FLAG_ERR BIT(4)
+
+/* Command 11h */
+#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
+ ({ typeof(buf_cnt) buf_cnt_ = (buf_cnt); \
+ idx ? ((0x80 | (buf_cnt_ & 0xFF)) << 8 | 0x11) : \
+ ((0x00 | (buf_cnt_ & 0xFF)) << 8 | 0x11); })
+
+#define NCT6694_CAN_RX_QUOTA 64
+
+enum nct6694_event_err {
+ NCT6694_CAN_EVT_NO_ERROR,
+ NCT6694_CAN_EVT_CRC_ERROR,
+ NCT6694_CAN_EVT_STUFF_ERROR,
+ NCT6694_CAN_EVT_ACK_ERROR,
+ NCT6694_CAN_EVT_FORM_ERROR,
+ NCT6694_CAN_EVT_BIT_ERROR,
+ NCT6694_CAN_EVT_TIMEOUT_ERROR,
+ NCT6694_CAN_EVT_UNKNOWN_ERROR,
+};
+
+enum nct6694_event_status {
+ NCT6694_CAN_EVT_ERROR_ACTIVE,
+ NCT6694_CAN_EVT_ERROR_PASSIVE,
+ NCT6694_CAN_EVT_BUS_OFF,
+ NCT6694_CAN_EVT_WARNING,
+};
+
+struct __packed nct6694_can_setting {
+ __le32 nbr;
+ __le32 dbr;
+ u8 active;
+ u8 reserved[3];
+ __le16 ctrl1;
+ __le16 ctrl2;
+ __le32 nbtp;
+ __le32 dbtp;
+};
+
+struct __packed nct6694_can_information {
+ u8 tx_fifo_cnt;
+ u8 rx_fifo_cnt;
+ __le16 reserved;
+ __le32 can_clk;
+};
+
+struct __packed nct6694_can_event {
+ u8 err1;
+ u8 status1;
+ u8 tx_evt1;
+ u8 rx_evt1;
+ u8 rec1;
+ u8 tec1;
+ u8 reserved1[2];
+ u8 err2;
+ u8 status2;
+ u8 tx_evt2;
+ u8 rx_evt2;
+ u8 rec2;
+ u8 tec2;
+ u8 reserved2[2];
+};
+
+struct __packed nct6694_can_xmit {
+ u8 tag;
+ u8 flag;
+ u8 reserved;
+ u8 dlc;
+ __le32 id;
+ u8 data[64];
+ u8 msg_buf[72];
+};
+
+struct nct6694_can_priv {
+ struct can_priv can; /* must be the first member */
+ struct net_device *ndev;
+ struct nct6694 *nct6694;
+ struct mutex lock;
+ struct sk_buff *tx_skb;
+ struct workqueue_struct *wq;
+ struct work_struct tx_work;
+ unsigned char *tx_buf;
+ unsigned char *rx_buf;
+ unsigned char can_idx;
+ bool tx_busy;
+};
+
+static const struct can_bittiming_const nct6694_can_bittiming_nominal_const = {
+ .name = DRVNAME,
+ .tseg1_min = 2,
+ .tseg1_max = 256,
+ .tseg2_min = 2,
+ .tseg2_max = 128,
+ .sjw_max = 128,
+ .brp_min = 1,
+ .brp_max = 511,
+ .brp_inc = 1,
+};
+
+static const struct can_bittiming_const nct6694_can_bittiming_data_const = {
+ .name = DRVNAME,
+ .tseg1_min = 1,
+ .tseg1_max = 32,
+ .tseg2_min = 1,
+ .tseg2_max = 16,
+ .sjw_max = 16,
+ .brp_min = 1,
+ .brp_max = 31,
+ .brp_inc = 1,
+};
+
+static void nct6694_can_clean(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+
+ if (priv->tx_skb || priv->tx_busy)
+ ndev->stats.tx_errors++;
+ dev_kfree_skb(priv->tx_skb);
+ if (priv->tx_busy)
+ can_free_echo_skb(priv->ndev, 0, NULL);
+ priv->tx_skb = NULL;
+ priv->tx_busy = false;
+}
+
+static int nct6694_can_get_berr_counter(const struct net_device *ndev,
+ struct can_berr_counter *bec)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
+ u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC;
+ int ret;
+
+ guard(mutex)(&priv->lock);
+
+ ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD2_OFFSET(priv->can_idx, mask),
+ sizeof(struct nct6694_can_event),
+ evt);
+ if (ret < 0)
+ return ret;
+
+ bec->rxerr = priv->can_idx ? evt->rec2 : evt->rec1;
+ bec->txerr = priv->can_idx ? evt->tec2 : evt->tec1;
+
+ return 0;
+}
+
+static int nct6694_can_handle_lost_msg(struct net_device *ndev)
+{
+ struct net_device_stats *stats = &ndev->stats;
+ struct sk_buff *skb;
+ struct can_frame *frame;
+
+ netdev_err(ndev, "RX FIFO overflow, message(s) lost.\n");
+
+ stats->rx_errors++;
+ stats->rx_over_errors++;
+
+ skb = alloc_can_err_skb(ndev, &frame);
+ if (unlikely(!skb))
+ return 0;
+
+ frame->can_id |= CAN_ERR_CRTL;
+ frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+
+ netif_receive_skb(skb);
+
+ return 1;
+}
+
+static void nct6694_can_read_fifo(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
+ struct net_device_stats *stats = &ndev->stats;
+ struct canfd_frame *cf;
+ struct sk_buff *skb;
+ int can_idx = priv->can_idx;
+ u32 id;
+ int ret;
+ u8 fd_format = 0;
+
+ guard(mutex)(&priv->lock);
+
+ ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
+ sizeof(struct nct6694_can_xmit), xmit);
+ if (ret < 0)
+ return;
+
+ /* Check type of frame and create skb */
+ fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
+ if (fd_format)
+ skb = alloc_canfd_skb(ndev, &cf);
+ else
+ skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
+
+ if (!skb) {
+ stats->rx_dropped++;
+ return;
+ }
+
+ cf->len = xmit->dlc;
+
+ /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
+ id = le32_to_cpu(xmit->id);
+ if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
+ /*
+ * In case the Extended ID frame is received, the standard
+ * and extended part of the ID are swapped in the register,
+ * so swap them back to obtain the correct ID.
+ */
+ id |= CAN_EFF_FLAG;
+ }
+
+ cf->can_id = id;
+
+ /* Set ESI flag */
+ if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
+ cf->flags |= CANFD_ESI;
+ netdev_dbg(ndev, "ESI Error\n");
+ }
+
+ /* Set RTR and BRS */
+ if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
+ cf->can_id |= CAN_RTR_FLAG;
+ } else {
+ if (xmit->flag & NCT6694_CAN_FLAG_BRS)
+ cf->flags |= CANFD_BRS;
+
+ memcpy(cf->data, xmit->data, cf->len);
+
+ stats->rx_bytes += cf->len;
+ }
+
+ stats->rx_packets++;
+
+ netif_receive_skb(skb);
+}
+
+static int nct6694_can_do_rx_poll(struct net_device *ndev, int quota)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
+ int can_idx = priv->can_idx;
+ u32 pkts = 0;
+ u8 mask_rx = NCT6694_CAN_EVENT_RX_EVT;
+ u8 rx_evt;
+
+ for (;;) {
+ scoped_guard(mutex, &priv->lock) {
+ nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD2_OFFSET(can_idx, mask_rx),
+ sizeof(struct nct6694_can_event), evt);
+
+ rx_evt = can_idx ? evt->rx_evt2 : evt->rx_evt1;
+ }
+
+ if (rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST)
+ nct6694_can_handle_lost_msg(ndev);
+
+ /* No data */
+ if ((rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) == 0)
+ break;
+
+ if (quota <= 0)
+ break;
+
+ nct6694_can_read_fifo(ndev);
+ quota--;
+ pkts++;
+ }
+
+ return pkts;
+}
+
+static int nct6694_can_handle_lec_err(struct net_device *ndev, u8 bus_err)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &ndev->stats;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+
+ if (bus_err == NCT6694_CAN_EVT_NO_ERROR)
+ return 0;
+
+ priv->can.can_stats.bus_error++;
+ stats->rx_errors++;
+
+ /* Propagate the error condition to the CAN stack. */
+ skb = alloc_can_err_skb(ndev, &cf);
+
+ if (unlikely(!skb))
+ return 0;
+
+ /* Read the error counter register and check for new errors. */
+ cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+ switch (bus_err) {
+ case NCT6694_CAN_EVT_CRC_ERROR:
+ cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
+ break;
+
+ case NCT6694_CAN_EVT_STUFF_ERROR:
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ break;
+
+ case NCT6694_CAN_EVT_ACK_ERROR:
+ cf->data[3] = CAN_ERR_PROT_LOC_ACK;
+ break;
+
+ case NCT6694_CAN_EVT_FORM_ERROR:
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ break;
+
+ case NCT6694_CAN_EVT_BIT_ERROR:
+ cf->data[2] |= CAN_ERR_PROT_BIT |
+ CAN_ERR_PROT_BIT0 |
+ CAN_ERR_PROT_BIT1;
+ break;
+
+ case NCT6694_CAN_EVT_TIMEOUT_ERROR:
+ cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+ break;
+
+ case NCT6694_CAN_EVT_UNKNOWN_ERROR:
+ cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+ /*
+ * It means 'unspecified'(the value is '0').
+ * But it is not sure if it's ok to send an error package
+ * without specific error bit.
+ */
+ break;
+
+ default:
+ break;
+ }
+
+ /* Reset the error counter, ack the IRQ and re-enable the counter. */
+ stats->rx_packets++;
+ stats->rx_bytes += cf->can_dlc;
+ netif_receive_skb(skb);
+
+ return 1;
+}
+
+static int nct6694_can_handle_state_change(struct net_device *ndev,
+ enum can_state new_state)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &ndev->stats;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ struct can_berr_counter bec;
+
+ switch (new_state) {
+ case CAN_STATE_ERROR_ACTIVE:
+ priv->can.can_stats.error_warning++;
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ break;
+ case CAN_STATE_ERROR_WARNING:
+ priv->can.can_stats.error_warning++;
+ priv->can.state = CAN_STATE_ERROR_WARNING;
+ break;
+ case CAN_STATE_ERROR_PASSIVE:
+ priv->can.can_stats.error_passive++;
+ priv->can.state = CAN_STATE_ERROR_PASSIVE;
+ break;
+ case CAN_STATE_BUS_OFF:
+ priv->can.state = CAN_STATE_BUS_OFF;
+ priv->can.can_stats.bus_off++;
+ can_bus_off(ndev);
+ break;
+ default:
+ break;
+ }
+
+ /* propagate the error condition to the CAN stack */
+ skb = alloc_can_err_skb(ndev, &cf);
+ if (unlikely(!skb))
+ return 0;
+
+ nct6694_can_get_berr_counter(ndev, &bec);
+
+ switch (new_state) {
+ case CAN_STATE_ERROR_WARNING:
+ /* error warning state */
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
+ CAN_ERR_CRTL_RX_WARNING;
+ cf->data[6] = bec.txerr;
+ cf->data[7] = bec.rxerr;
+ break;
+ case CAN_STATE_ERROR_PASSIVE:
+ /* error passive state */
+ cf->can_id |= CAN_ERR_CRTL;
+ cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
+ if (bec.txerr > 127)
+ cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
+ cf->data[6] = bec.txerr;
+ cf->data[7] = bec.rxerr;
+ break;
+ case CAN_STATE_BUS_OFF:
+ /* bus-off state */
+ cf->can_id |= CAN_ERR_BUSOFF;
+ break;
+ default:
+ break;
+ }
+
+ stats->rx_packets++;
+ stats->rx_bytes += cf->can_dlc;
+ netif_receive_skb(skb);
+
+ return 1;
+}
+
+static int nct6694_can_handle_state_errors(struct net_device *ndev,
+ unsigned char can_status)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ int work_done = 0;
+
+ if (can_status == NCT6694_CAN_EVT_ERROR_ACTIVE &&
+ priv->can.state != CAN_STATE_ERROR_ACTIVE) {
+ netdev_dbg(ndev, "Error, entered active state\n");
+ work_done += nct6694_can_handle_state_change(ndev,
+ CAN_STATE_ERROR_ACTIVE);
+ }
+
+ if (can_status == NCT6694_CAN_EVT_WARNING &&
+ priv->can.state != CAN_STATE_ERROR_WARNING) {
+ netdev_dbg(ndev, "Error, entered warning state\n");
+ work_done += nct6694_can_handle_state_change(ndev,
+ CAN_STATE_ERROR_WARNING);
+ }
+
+ if (can_status == NCT6694_CAN_EVT_ERROR_PASSIVE &&
+ priv->can.state != CAN_STATE_ERROR_PASSIVE) {
+ netdev_dbg(ndev, "Error, entered passive state\n");
+ work_done += nct6694_can_handle_state_change(ndev,
+ CAN_STATE_ERROR_PASSIVE);
+ }
+
+ if (can_status == NCT6694_CAN_EVT_BUS_OFF &&
+ priv->can.state != CAN_STATE_BUS_OFF) {
+ netdev_dbg(ndev, "Error, entered bus-off state\n");
+ work_done += nct6694_can_handle_state_change(ndev,
+ CAN_STATE_BUS_OFF);
+ }
+
+ return work_done;
+}
+
+static int nct6694_can_poll(struct net_device *ndev, int quota)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
+ int can_idx = priv->can_idx;
+ int work_done = 0, ret;
+ u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
+ u8 bus_err, can_status;
+
+ scoped_guard(mutex, &priv->lock) {
+ ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
+ sizeof(struct nct6694_can_event), evt);
+ if (ret < 0)
+ return IRQ_NONE;
+
+ if (can_idx) {
+ bus_err = evt->err2;
+ can_status = evt->status2;
+ } else {
+ bus_err = evt->err1;
+ can_status = evt->status1;
+ }
+ }
+
+ /* Handle bus state changes */
+ work_done += nct6694_can_handle_state_errors(ndev, can_status);
+
+ /* Handle lec errors on the bus */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
+ work_done += nct6694_can_handle_lec_err(ndev, bus_err);
+
+ /* Handle RX events */
+ work_done += nct6694_can_do_rx_poll(ndev, quota - work_done);
+ return work_done;
+}
+
+static void nct6694_can_tx_irq(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &ndev->stats;
+
+ guard(mutex)(&priv->lock);
+ stats->tx_bytes += can_get_echo_skb(ndev, 0, NULL);
+ stats->tx_packets++;
+ priv->tx_busy = false;
+ netif_wake_queue(ndev);
+}
+
+static irqreturn_t nct6694_can_irq(int irq, void *data)
+{
+ struct net_device *ndev = data;
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
+ int can_idx = priv->can_idx;
+ int ret;
+ u8 mask_sts = NCT6694_CAN_EVENT_TX_EVT;
+ u8 tx_evt;
+
+ scoped_guard(mutex, &priv->lock) {
+ ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD2_OFFSET(can_idx, mask_sts),
+ sizeof(struct nct6694_can_event), evt);
+ if (ret < 0)
+ return IRQ_NONE;
+
+ tx_evt = can_idx ? evt->tx_evt2 : evt->tx_evt1;
+ }
+
+ if (tx_evt) {
+ nct6694_can_tx_irq(ndev);
+ } else {
+ ret = nct6694_can_poll(ndev, NCT6694_CAN_RX_QUOTA);
+ if (!ret)
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int nct6694_can_start(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_setting *setting = (struct nct6694_can_setting *)priv->tx_buf;
+ const struct can_bittiming *n_bt = &priv->can.bittiming;
+ const struct can_bittiming *d_bt = &priv->can.data_bittiming;
+ int ret;
+
+ guard(mutex)(&priv->lock);
+
+ memset(priv->tx_buf, 0, sizeof(struct nct6694_can_setting));
+ setting->nbr = cpu_to_le32(n_bt->bitrate);
+ setting->dbr = cpu_to_le32(d_bt->bitrate);
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+ setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_MON);
+
+ if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+ priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
+ setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_NISO);
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
+ setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_LBCK);
+
+ ret = nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD0_OFFSET(priv->can_idx),
+ sizeof(struct nct6694_can_setting), setting);
+ if (ret < 0)
+ return ret;
+
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+ return 0;
+}
+
+static int nct6694_can_stop(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+
+ netif_stop_queue(ndev);
+ free_irq(ndev->irq, ndev);
+ destroy_workqueue(priv->wq);
+ priv->wq = NULL;
+ nct6694_can_clean(ndev);
+ priv->can.state = CAN_STATE_STOPPED;
+ close_candev(ndev);
+
+ return 0;
+}
+
+static int nct6694_can_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+ switch (mode) {
+ case CAN_MODE_START:
+ nct6694_can_clean(ndev);
+ nct6694_can_start(ndev);
+ netif_wake_queue(ndev);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int nct6694_can_open(struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ int ret;
+
+ ret = open_candev(ndev);
+ if (ret)
+ return ret;
+
+ ret = request_threaded_irq(ndev->irq, NULL,
+ nct6694_can_irq, IRQF_ONESHOT,
+ "nct6694_can", ndev);
+ if (ret) {
+ netdev_err(ndev, "Failed to request IRQ\n");
+ goto close_candev;
+ }
+
+ priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
+ WQ_FREEZABLE | WQ_MEM_RECLAIM,
+ ndev->name);
+ if (!priv->wq) {
+ ret = -ENOMEM;
+ goto free_irq;
+ }
+
+ priv->tx_skb = NULL;
+ priv->tx_busy = false;
+
+ ret = nct6694_can_start(ndev);
+ if (ret)
+ goto destroy_wq;
+
+ netif_start_queue(ndev);
+
+ return 0;
+
+destroy_wq:
+ destroy_workqueue(priv->wq);
+free_irq:
+ free_irq(ndev->irq, ndev);
+close_candev:
+ close_candev(ndev);
+ return ret;
+}
+
+static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
+ struct net_device *ndev)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+
+ if (priv->tx_skb || priv->tx_busy) {
+ netdev_err(ndev, "hard_xmit called while tx busy\n");
+ return NETDEV_TX_BUSY;
+ }
+
+ if (can_dev_dropped_skb(ndev, skb))
+ return NETDEV_TX_OK;
+
+ netif_stop_queue(ndev);
+ priv->tx_skb = skb;
+ queue_work(priv->wq, &priv->tx_work);
+
+ return NETDEV_TX_OK;
+}
+
+static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
+{
+ struct nct6694_can_priv *priv = netdev_priv(ndev);
+ struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
+ u32 txid = 0;
+
+ memset(xmit, 0, sizeof(struct nct6694_can_xmit));
+
+ if (priv->can_idx == 0)
+ xmit->tag = NCT6694_CAN_TAG_CAN0;
+ else
+ xmit->tag = NCT6694_CAN_TAG_CAN1;
+
+ if (cf->can_id & CAN_EFF_FLAG) {
+ txid = cf->can_id & CAN_EFF_MASK;
+ /*
+ * In case the Extended ID frame is transmitted, the
+ * standard and extended part of the ID are swapped
+ * in the register, so swap them back to send the
+ * correct ID.
+ */
+ xmit->flag |= NCT6694_CAN_FLAG_EFF;
+ } else {
+ txid = cf->can_id & CAN_SFF_MASK;
+ }
+
+ xmit->id = cpu_to_le32(txid);
+ xmit->dlc = cf->len;
+
+ if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+ can_is_canfd_skb(priv->tx_skb)) {
+ xmit->flag |= NCT6694_CAN_FLAG_FD;
+ if (cf->flags & CANFD_BRS)
+ xmit->flag |= NCT6694_CAN_FLAG_BRS;
+ }
+
+ if (cf->can_id & CAN_RTR_FLAG)
+ xmit->flag |= NCT6694_CAN_FLAG_RTR;
+
+ memcpy(xmit->data, cf->data, cf->len);
+
+ nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD10_OFFSET(1),
+ sizeof(struct nct6694_can_xmit),
+ xmit);
+}
+
+static void nct6694_can_tx_work(struct work_struct *work)
+{
+ struct nct6694_can_priv *priv = container_of(work,
+ struct nct6694_can_priv,
+ tx_work);
+ struct net_device *ndev = priv->ndev;
+ struct canfd_frame *cf;
+
+ guard(mutex)(&priv->lock);
+
+ if (priv->tx_skb) {
+ if (priv->can.state == CAN_STATE_BUS_OFF) {
+ nct6694_can_clean(ndev);
+ } else {
+ cf = (struct canfd_frame *)priv->tx_skb->data;
+ nct6694_can_tx(ndev, cf);
+ priv->tx_busy = true;
+ can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
+ priv->tx_skb = NULL;
+ }
+ }
+}
+
+static const struct net_device_ops nct6694_can_netdev_ops = {
+ .ndo_open = nct6694_can_open,
+ .ndo_stop = nct6694_can_stop,
+ .ndo_start_xmit = nct6694_can_start_xmit,
+ .ndo_change_mtu = can_change_mtu,
+};
+
+static const struct ethtool_ops nct6694_can_ethtool_ops = {
+ .get_ts_info = ethtool_op_get_ts_info,
+};
+
+static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
+{
+ struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
+ int ret;
+
+ ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+ NCT6694_CAN_CMD1_OFFSET,
+ sizeof(struct nct6694_can_information),
+ info);
+ if (ret)
+ return ret;
+
+ return le32_to_cpu(info->can_clk);
+}
+
+static int nct6694_can_probe(struct platform_device *pdev)
+{
+ const struct mfd_cell *cell = mfd_get_cell(pdev);
+ struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
+ struct nct6694_can_priv *priv;
+ struct net_device *ndev;
+ int ret, irq, can_clk;
+
+ irq = irq_create_mapping(nct6694->domain,
+ NCT6694_IRQ_CAN1 + cell->id);
+ if (!irq)
+ return -EINVAL;
+
+ ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
+ if (!ndev)
+ return -ENOMEM;
+
+ ndev->irq = irq;
+ ndev->flags |= IFF_ECHO;
+ ndev->netdev_ops = &nct6694_can_netdev_ops;
+ ndev->ethtool_ops = &nct6694_can_ethtool_ops;
+
+ priv = netdev_priv(ndev);
+ priv->nct6694 = nct6694;
+ priv->ndev = ndev;
+
+ priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
+ sizeof(unsigned char), GFP_KERNEL);
+ if (!priv->tx_buf) {
+ ret = -ENOMEM;
+ goto free_candev;
+ }
+
+ priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
+ sizeof(unsigned char), GFP_KERNEL);
+ if (!priv->rx_buf) {
+ ret = -ENOMEM;
+ goto free_candev;
+ }
+
+ can_clk = nct6694_can_get_clock(priv);
+ if (can_clk < 0) {
+ ret = -EIO;
+ goto free_candev;
+ }
+
+ mutex_init(&priv->lock);
+ INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
+
+ priv->can_idx = cell->id;
+ priv->can.state = CAN_STATE_STOPPED;
+ priv->can.clock.freq = can_clk;
+ priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
+ priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
+ priv->can.do_set_mode = nct6694_can_set_mode;
+ priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
+
+ priv->can.ctrlmode = CAN_CTRLMODE_FD;
+
+ priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+ CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_FD |
+ CAN_CTRLMODE_FD_NON_ISO |
+ CAN_CTRLMODE_BERR_REPORTING;
+
+ platform_set_drvdata(pdev, priv);
+ SET_NETDEV_DEV(priv->ndev, &pdev->dev);
+
+ ret = register_candev(priv->ndev);
+ if (ret)
+ goto free_candev;
+
+ return 0;
+
+free_candev:
+ free_candev(ndev);
+ return dev_err_probe(&pdev->dev, ret, "Probe failed\n");
+}
+
+static void nct6694_can_remove(struct platform_device *pdev)
+{
+ struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
+
+ cancel_work_sync(&priv->tx_work);
+ unregister_candev(priv->ndev);
+ free_candev(priv->ndev);
+}
+
+static struct platform_driver nct6694_can_driver = {
+ .driver = {
+ .name = DRVNAME,
+ },
+ .probe = nct6694_can_probe,
+ .remove = nct6694_can_remove,
+};
+
+module_platform_driver(nct6694_can_driver);
+
+MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
+MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nct6694-can");
--
2.34.1On 10.12.2024 18:45:21, Ming Yu wrote:
> This driver supports Socket CANfd functionality for NCT6694 MFD
> device based on USB interface.
Please use the rx-offload helper, otherwise the CAN frames might be
revived out of order.
>
> Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
> ---
> MAINTAINERS | 1 +
> drivers/net/can/Kconfig | 10 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/nct6694_canfd.c | 920 ++++++++++++++++++++++++++++++++
> 4 files changed, 932 insertions(+)
> create mode 100644 drivers/net/can/nct6694_canfd.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a190f2b08fa3..eb5d46825e71 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16548,6 +16548,7 @@ S: Supported
> F: drivers/gpio/gpio-nct6694.c
> F: drivers/i2c/busses/i2c-nct6694.c
> F: drivers/mfd/nct6694.c
> +F: drivers/net/can/nct6694_canfd.c
> F: include/linux/mfd/nct6694.h
>
> NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index cf989bea9aa3..130e98ec28a5 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -200,6 +200,16 @@ config CAN_SUN4I
> To compile this driver as a module, choose M here: the module will
> be called sun4i_can.
>
> +config CAN_NCT6694
> + tristate "Nuvoton NCT6694 Socket CANfd support"
> + depends on MFD_NCT6694
> + help
> + If you say yes to this option, support will be included for Nuvoton
> + NCT6694, a USB device to socket CANfd controller.
> +
> + This driver can also be built as a module. If so, the module will
> + be called nct6694_canfd.
> +
> config CAN_TI_HECC
> depends on ARM
> tristate "TI High End CAN Controller"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index a71db2cfe990..4a6b5b9d6c2b 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
> obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> obj-$(CONFIG_CAN_M_CAN) += m_can/
> +obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o
> obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
> obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_SUN4I) += sun4i_can.o
> diff --git a/drivers/net/can/nct6694_canfd.c b/drivers/net/can/nct6694_canfd.c
> new file mode 100644
> index 000000000000..54f20f0681e2
> --- /dev/null
> +++ b/drivers/net/can/nct6694_canfd.c
> @@ -0,0 +1,920 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NCT6694 Socket CANfd driver based on USB interface.
> + *
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/can/dev.h>
> +#include <linux/ethtool.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/nct6694.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/platform_device.h>
> +
> +#define DRVNAME "nct6694-can"
> +
> +/* Host interface */
> +#define NCT6694_CAN_MOD 0x05
> +
> +/* Message Channel*/
> +/* Command 00h */
> +#define NCT6694_CAN_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000)
> +#define NCT6694_CAN_CTRL1_MON BIT(0)
> +#define NCT6694_CAN_CTRL1_NISO BIT(1)
> +#define NCT6694_CAN_CTRL1_LBCK BIT(2)
> +
> +/* Command 01h */
> +#define NCT6694_CAN_CMD1_OFFSET 0x0001
> +
> +/* Command 02h */
> +#define NCT6694_CAN_CMD2_OFFSET(idx, mask) \
> + ({ typeof(mask) mask_ = (mask); \
> + idx ? ((0x80 | (mask_ & 0xFF)) << 8 | 0x02) : \
> + ((0x00 | (mask_ & 0xFF)) << 8 | 0x02); })
> +
> +#define NCT6694_CAN_EVENT_ERR BIT(0)
> +#define NCT6694_CAN_EVENT_STATUS BIT(1)
> +#define NCT6694_CAN_EVENT_TX_EVT BIT(2)
> +#define NCT6694_CAN_EVENT_RX_EVT BIT(3)
> +#define NCT6694_CAN_EVENT_REC BIT(4)
> +#define NCT6694_CAN_EVENT_TEC BIT(5)
> +#define NCT6694_CAN_EVT_TX_FIFO_EMPTY BIT(7) /* Read-clear */
> +#define NCT6694_CAN_EVT_RX_DATA_LOST BIT(5) /* Read-clear */
> +#define NCT6694_CAN_EVT_RX_HALF_FULL BIT(6) /* Read-clear */
> +#define NCT6694_CAN_EVT_RX_DATA_IN BIT(7)
> +
> +/* Command 10h */
> +#define NCT6694_CAN_CMD10_OFFSET(buf_cnt) \
> + (((buf_cnt) & 0xFF) << 8 | 0x10)
> +#define NCT6694_CAN_TAG_CAN0 0xC0
> +#define NCT6694_CAN_TAG_CAN1 0xC1
> +#define NCT6694_CAN_FLAG_EFF BIT(0)
> +#define NCT6694_CAN_FLAG_RTR BIT(1)
> +#define NCT6694_CAN_FLAG_FD BIT(2)
> +#define NCT6694_CAN_FLAG_BRS BIT(3)
> +#define NCT6694_CAN_FLAG_ERR BIT(4)
> +
> +/* Command 11h */
> +#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
> + ({ typeof(buf_cnt) buf_cnt_ = (buf_cnt); \
> + idx ? ((0x80 | (buf_cnt_ & 0xFF)) << 8 | 0x11) : \
> + ((0x00 | (buf_cnt_ & 0xFF)) << 8 | 0x11); })
> +
> +#define NCT6694_CAN_RX_QUOTA 64
> +
> +enum nct6694_event_err {
> + NCT6694_CAN_EVT_NO_ERROR,
^^^ add _ERR_
> + NCT6694_CAN_EVT_CRC_ERROR,
> + NCT6694_CAN_EVT_STUFF_ERROR,
> + NCT6694_CAN_EVT_ACK_ERROR,
> + NCT6694_CAN_EVT_FORM_ERROR,
> + NCT6694_CAN_EVT_BIT_ERROR,
> + NCT6694_CAN_EVT_TIMEOUT_ERROR,
> + NCT6694_CAN_EVT_UNKNOWN_ERROR,
> +};
> +
> +enum nct6694_event_status {
> + NCT6694_CAN_EVT_ERROR_ACTIVE,
^^^ add _STATUS_
> + NCT6694_CAN_EVT_ERROR_PASSIVE,
> + NCT6694_CAN_EVT_BUS_OFF,
> + NCT6694_CAN_EVT_WARNING,
> +};
> +
> +struct __packed nct6694_can_setting {
> + __le32 nbr;
> + __le32 dbr;
> + u8 active;
> + u8 reserved[3];
> + __le16 ctrl1;
> + __le16 ctrl2;
> + __le32 nbtp;
> + __le32 dbtp;
> +};
> +
> +struct __packed nct6694_can_information {
> + u8 tx_fifo_cnt;
> + u8 rx_fifo_cnt;
> + __le16 reserved;
u8 reserved[2];
> + __le32 can_clk;
> +};
> +
> +struct __packed nct6694_can_event {
> + u8 err1;
> + u8 status1;
> + u8 tx_evt1;
> + u8 rx_evt1;
> + u8 rec1;
> + u8 tec1;
> + u8 reserved1[2];
> + u8 err2;
> + u8 status2;
> + u8 tx_evt2;
> + u8 rx_evt2;
> + u8 rec2;
> + u8 tec2;
> + u8 reserved2[2];
> +};
Create an extra struct that only describes a channel
struct __packed nct6694_can_event_channel {
u8 err;
u8 status;
u8 tx_evt;
u8 rx_evt;
u8 rec;
u8 tec;
u8 reserved[2];
}
and put an array of 2 into struct __packed nct6694_can_event.
> +
> +struct __packed nct6694_can_xmit {
> + u8 tag;
> + u8 flag;
> + u8 reserved;
> + u8 dlc;
> + __le32 id;
> + u8 data[64];
> + u8 msg_buf[72];
Why is the message so long? What's in the msg_buf?
> +};
> +
> +struct nct6694_can_priv {
> + struct can_priv can; /* must be the first member */
> + struct net_device *ndev;
> + struct nct6694 *nct6694;
> + struct mutex lock;
What does lock protect?
> + struct sk_buff *tx_skb;
> + struct workqueue_struct *wq;
> + struct work_struct tx_work;
> + unsigned char *tx_buf;
void *
> + unsigned char *rx_buf;
void *
> + unsigned char can_idx;
> + bool tx_busy;
IMHO it makes no sense to have tx_skb and tx_busy
> +};
> +
> +static const struct can_bittiming_const nct6694_can_bittiming_nominal_const = {
> + .name = DRVNAME,
> + .tseg1_min = 2,
> + .tseg1_max = 256,
> + .tseg2_min = 2,
> + .tseg2_max = 128,
> + .sjw_max = 128,
> + .brp_min = 1,
> + .brp_max = 511,
> + .brp_inc = 1,
> +};
> +
> +static const struct can_bittiming_const nct6694_can_bittiming_data_const = {
> + .name = DRVNAME,
> + .tseg1_min = 1,
> + .tseg1_max = 32,
> + .tseg2_min = 1,
> + .tseg2_max = 16,
> + .sjw_max = 16,
> + .brp_min = 1,
> + .brp_max = 31,
> + .brp_inc = 1,
> +};
> +
> +static void nct6694_can_clean(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> +
> + if (priv->tx_skb || priv->tx_busy)
> + ndev->stats.tx_errors++;
> + dev_kfree_skb(priv->tx_skb);
> + if (priv->tx_busy)
> + can_free_echo_skb(priv->ndev, 0, NULL);
> + priv->tx_skb = NULL;
> + priv->tx_busy = false;
> +}
> +
> +static int nct6694_can_get_berr_counter(const struct net_device *ndev,
> + struct can_berr_counter *bec)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC;
> + int ret;
> +
> + guard(mutex)(&priv->lock);
> +
> + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD2_OFFSET(priv->can_idx, mask),
> + sizeof(struct nct6694_can_event),
> + evt);
> + if (ret < 0)
> + return ret;
> +
> + bec->rxerr = priv->can_idx ? evt->rec2 : evt->rec1;
> + bec->txerr = priv->can_idx ? evt->tec2 : evt->tec1;
> +
> + return 0;
> +}
> +
> +static int nct6694_can_handle_lost_msg(struct net_device *ndev)
> +{
> + struct net_device_stats *stats = &ndev->stats;
> + struct sk_buff *skb;
> + struct can_frame *frame;
> +
> + netdev_err(ndev, "RX FIFO overflow, message(s) lost.\n");
> +
> + stats->rx_errors++;
> + stats->rx_over_errors++;
> +
> + skb = alloc_can_err_skb(ndev, &frame);
> + if (unlikely(!skb))
> + return 0;
> +
> + frame->can_id |= CAN_ERR_CRTL;
> + frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +
> + netif_receive_skb(skb);
> +
> + return 1;
> +}
> +
> +static void nct6694_can_read_fifo(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
> + struct net_device_stats *stats = &ndev->stats;
> + struct canfd_frame *cf;
> + struct sk_buff *skb;
> + int can_idx = priv->can_idx;
> + u32 id;
> + int ret;
> + u8 fd_format = 0;
bool - no need to init
> +
> + guard(mutex)(&priv->lock);
> +
> + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
> + sizeof(struct nct6694_can_xmit), xmit);
> + if (ret < 0)
> + return;
> +
> + /* Check type of frame and create skb */
> + fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
> + if (fd_format)
> + skb = alloc_canfd_skb(ndev, &cf);
> + else
> + skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
> +
> + if (!skb) {
> + stats->rx_dropped++;
> + return;
> + }
> +
> + cf->len = xmit->dlc;
what does xmit->dlc contain? The DLC or the length?
> +
> + /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
> + id = le32_to_cpu(xmit->id);
> + if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
> + /*
> + * In case the Extended ID frame is received, the standard
> + * and extended part of the ID are swapped in the register,
> + * so swap them back to obtain the correct ID.
> + */
You comment doesn't match the code.
> + id |= CAN_EFF_FLAG;
> + }
> +
> + cf->can_id = id;
> +
> + /* Set ESI flag */
> + if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
> + cf->flags |= CANFD_ESI;
> + netdev_dbg(ndev, "ESI Error\n");
> + }
> +
> + /* Set RTR and BRS */
> + if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
> + cf->can_id |= CAN_RTR_FLAG;
> + } else {
> + if (xmit->flag & NCT6694_CAN_FLAG_BRS)
> + cf->flags |= CANFD_BRS;
> +
> + memcpy(cf->data, xmit->data, cf->len);
> +
> + stats->rx_bytes += cf->len;
> + }
> +
> + stats->rx_packets++;
> +
> + netif_receive_skb(skb);
> +}
> +
> +static int nct6694_can_do_rx_poll(struct net_device *ndev, int quota)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> + int can_idx = priv->can_idx;
> + u32 pkts = 0;
> + u8 mask_rx = NCT6694_CAN_EVENT_RX_EVT;
> + u8 rx_evt;
> +
> + for (;;) {
> + scoped_guard(mutex, &priv->lock) {
> + nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD2_OFFSET(can_idx, mask_rx),
> + sizeof(struct nct6694_can_event), evt);
> +
> + rx_evt = can_idx ? evt->rx_evt2 : evt->rx_evt1;
> + }
> +
> + if (rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST)
> + nct6694_can_handle_lost_msg(ndev);
> +
> + /* No data */
> + if ((rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) == 0)
> + break;
> +
> + if (quota <= 0)
> + break;
> +
> + nct6694_can_read_fifo(ndev);
> + quota--;
> + pkts++;
> + }
> +
> + return pkts;
> +}
> +
> +static int nct6694_can_handle_lec_err(struct net_device *ndev, u8 bus_err)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> +
> + if (bus_err == NCT6694_CAN_EVT_NO_ERROR)
> + return 0;
> +
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + /* Propagate the error condition to the CAN stack. */
> + skb = alloc_can_err_skb(ndev, &cf);
> +
> + if (unlikely(!skb))
> + return 0;
> +
> + /* Read the error counter register and check for new errors. */
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + switch (bus_err) {
> + case NCT6694_CAN_EVT_CRC_ERROR:
> + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> + break;
> +
> + case NCT6694_CAN_EVT_STUFF_ERROR:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> +
> + case NCT6694_CAN_EVT_ACK_ERROR:
> + cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> + break;
> +
> + case NCT6694_CAN_EVT_FORM_ERROR:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> +
> + case NCT6694_CAN_EVT_BIT_ERROR:
> + cf->data[2] |= CAN_ERR_PROT_BIT |
> + CAN_ERR_PROT_BIT0 |
> + CAN_ERR_PROT_BIT1;
> + break;
> +
> + case NCT6694_CAN_EVT_TIMEOUT_ERROR:
> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> + break;
> +
> + case NCT6694_CAN_EVT_UNKNOWN_ERROR:
> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> + /*
> + * It means 'unspecified'(the value is '0').
> + * But it is not sure if it's ok to send an error package
> + * without specific error bit.
> + */
> + break;
> +
> + default:
> + break;
> + }
> +
> + /* Reset the error counter, ack the IRQ and re-enable the counter. */
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + netif_receive_skb(skb);
> +
> + return 1;
> +}
> +
> +static int nct6694_can_handle_state_change(struct net_device *ndev,
> + enum can_state new_state)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + struct can_berr_counter bec;
> +
> + switch (new_state) {
> + case CAN_STATE_ERROR_ACTIVE:
> + priv->can.can_stats.error_warning++;
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> + break;
> + case CAN_STATE_ERROR_WARNING:
> + priv->can.can_stats.error_warning++;
> + priv->can.state = CAN_STATE_ERROR_WARNING;
> + break;
> + case CAN_STATE_ERROR_PASSIVE:
> + priv->can.can_stats.error_passive++;
> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> + break;
> + case CAN_STATE_BUS_OFF:
> + priv->can.state = CAN_STATE_BUS_OFF;
> + priv->can.can_stats.bus_off++;
> + can_bus_off(ndev);
> + break;
> + default:
> + break;
> + }
> +
> + /* propagate the error condition to the CAN stack */
> + skb = alloc_can_err_skb(ndev, &cf);
> + if (unlikely(!skb))
> + return 0;
> +
> + nct6694_can_get_berr_counter(ndev, &bec);
> +
> + switch (new_state) {
> + case CAN_STATE_ERROR_WARNING:
> + /* error warning state */
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
> + CAN_ERR_CRTL_RX_WARNING;
> + cf->data[6] = bec.txerr;
> + cf->data[7] = bec.rxerr;
> + break;
> + case CAN_STATE_ERROR_PASSIVE:
> + /* error passive state */
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> + if (bec.txerr > 127)
> + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> + cf->data[6] = bec.txerr;
> + cf->data[7] = bec.rxerr;
> + break;
> + case CAN_STATE_BUS_OFF:
> + /* bus-off state */
> + cf->can_id |= CAN_ERR_BUSOFF;
> + break;
> + default:
> + break;
> + }
> +
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + netif_receive_skb(skb);
> +
> + return 1;
> +}
> +
> +static int nct6694_can_handle_state_errors(struct net_device *ndev,
> + unsigned char can_status)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + int work_done = 0;
> +
> + if (can_status == NCT6694_CAN_EVT_ERROR_ACTIVE &&
> + priv->can.state != CAN_STATE_ERROR_ACTIVE) {
> + netdev_dbg(ndev, "Error, entered active state\n");
> + work_done += nct6694_can_handle_state_change(ndev,
> + CAN_STATE_ERROR_ACTIVE);
> + }
> +
> + if (can_status == NCT6694_CAN_EVT_WARNING &&
> + priv->can.state != CAN_STATE_ERROR_WARNING) {
> + netdev_dbg(ndev, "Error, entered warning state\n");
> + work_done += nct6694_can_handle_state_change(ndev,
> + CAN_STATE_ERROR_WARNING);
> + }
> +
> + if (can_status == NCT6694_CAN_EVT_ERROR_PASSIVE &&
> + priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> + netdev_dbg(ndev, "Error, entered passive state\n");
> + work_done += nct6694_can_handle_state_change(ndev,
> + CAN_STATE_ERROR_PASSIVE);
> + }
> +
> + if (can_status == NCT6694_CAN_EVT_BUS_OFF &&
> + priv->can.state != CAN_STATE_BUS_OFF) {
> + netdev_dbg(ndev, "Error, entered bus-off state\n");
> + work_done += nct6694_can_handle_state_change(ndev,
> + CAN_STATE_BUS_OFF);
> + }
> +
> + return work_done;
> +}
> +
> +static int nct6694_can_poll(struct net_device *ndev, int quota)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> + int can_idx = priv->can_idx;
> + int work_done = 0, ret;
> + u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
> + u8 bus_err, can_status;
> +
> + scoped_guard(mutex, &priv->lock) {
> + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
> + sizeof(struct nct6694_can_event), evt);
> + if (ret < 0)
> + return IRQ_NONE;
propagate the error
> +
> + if (can_idx) {
> + bus_err = evt->err2;
> + can_status = evt->status2;
> + } else {
> + bus_err = evt->err1;
> + can_status = evt->status1;
> + }
> + }
> +
> + /* Handle bus state changes */
> + work_done += nct6694_can_handle_state_errors(ndev, can_status);
> +
> + /* Handle lec errors on the bus */
> + if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> + work_done += nct6694_can_handle_lec_err(ndev, bus_err);
> +
> + /* Handle RX events */
> + work_done += nct6694_can_do_rx_poll(ndev, quota - work_done);
> + return work_done;
> +}
> +
> +static void nct6694_can_tx_irq(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> +
> + guard(mutex)(&priv->lock);
> + stats->tx_bytes += can_get_echo_skb(ndev, 0, NULL);
> + stats->tx_packets++;
> + priv->tx_busy = false;
> + netif_wake_queue(ndev);
> +}
> +
> +static irqreturn_t nct6694_can_irq(int irq, void *data)
> +{
> + struct net_device *ndev = data;
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> + int can_idx = priv->can_idx;
> + int ret;
> + u8 mask_sts = NCT6694_CAN_EVENT_TX_EVT;
> + u8 tx_evt;
> +
> + scoped_guard(mutex, &priv->lock) {
> + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD2_OFFSET(can_idx, mask_sts),
> + sizeof(struct nct6694_can_event), evt);
> + if (ret < 0)
> + return IRQ_NONE;
> +
> + tx_evt = can_idx ? evt->tx_evt2 : evt->tx_evt1;
> + }
> +
> + if (tx_evt) {
> + nct6694_can_tx_irq(ndev);
> + } else {
> + ret = nct6694_can_poll(ndev, NCT6694_CAN_RX_QUOTA);
> + if (!ret)
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int nct6694_can_start(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_setting *setting = (struct nct6694_can_setting *)priv->tx_buf;
> + const struct can_bittiming *n_bt = &priv->can.bittiming;
> + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> + int ret;
> +
> + guard(mutex)(&priv->lock);
> +
> + memset(priv->tx_buf, 0, sizeof(struct nct6694_can_setting));
> + setting->nbr = cpu_to_le32(n_bt->bitrate);
> + setting->dbr = cpu_to_le32(d_bt->bitrate);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_MON);
> +
> + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> + priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_NISO);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_LBCK);
> +
> + ret = nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD0_OFFSET(priv->can_idx),
> + sizeof(struct nct6694_can_setting), setting);
> + if (ret < 0)
> + return ret;
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + return 0;
> +}
> +
> +static int nct6694_can_stop(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> +
> + netif_stop_queue(ndev);
> + free_irq(ndev->irq, ndev);
> + destroy_workqueue(priv->wq);
> + priv->wq = NULL;
> + nct6694_can_clean(ndev);
> + priv->can.state = CAN_STATE_STOPPED;
> + close_candev(ndev);
> +
> + return 0;
> +}
> +
> +static int nct6694_can_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> + switch (mode) {
> + case CAN_MODE_START:
> + nct6694_can_clean(ndev);
> + nct6694_can_start(ndev);
> + netif_wake_queue(ndev);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int nct6694_can_open(struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = open_candev(ndev);
> + if (ret)
> + return ret;
> +
> + ret = request_threaded_irq(ndev->irq, NULL,
> + nct6694_can_irq, IRQF_ONESHOT,
> + "nct6694_can", ndev);
> + if (ret) {
> + netdev_err(ndev, "Failed to request IRQ\n");
> + goto close_candev;
> + }
> +
> + priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
> + WQ_FREEZABLE | WQ_MEM_RECLAIM,
> + ndev->name);
> + if (!priv->wq) {
> + ret = -ENOMEM;
> + goto free_irq;
> + }
> +
> + priv->tx_skb = NULL;
> + priv->tx_busy = false;
> +
> + ret = nct6694_can_start(ndev);
> + if (ret)
> + goto destroy_wq;
> +
> + netif_start_queue(ndev);
> +
> + return 0;
> +
> +destroy_wq:
> + destroy_workqueue(priv->wq);
> +free_irq:
> + free_irq(ndev->irq, ndev);
> +close_candev:
> + close_candev(ndev);
> + return ret;
> +}
> +
> +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> + struct net_device *ndev)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> +
> + if (priv->tx_skb || priv->tx_busy) {
> + netdev_err(ndev, "hard_xmit called while tx busy\n");
> + return NETDEV_TX_BUSY;
> + }
> +
> + if (can_dev_dropped_skb(ndev, skb))
> + return NETDEV_TX_OK;
please drop first
> +
> + netif_stop_queue(ndev);
> + priv->tx_skb = skb;
> + queue_work(priv->wq, &priv->tx_work);
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
> +{
> + struct nct6694_can_priv *priv = netdev_priv(ndev);
> + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
> + u32 txid = 0;
> +
> + memset(xmit, 0, sizeof(struct nct6694_can_xmit));
> +
> + if (priv->can_idx == 0)
> + xmit->tag = NCT6694_CAN_TAG_CAN0<;
> + else
> + xmit->tag = NCT6694_CAN_TAG_CAN1;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + txid = cf->can_id & CAN_EFF_MASK;
> + /*
> + * In case the Extended ID frame is transmitted, the
> + * standard and extended part of the ID are swapped
> + * in the register, so swap them back to send the
> + * correct ID.
You comment doesn't match the code.
> + */
> + xmit->flag |= NCT6694_CAN_FLAG_EFF;
> + } else {
> + txid = cf->can_id & CAN_SFF_MASK;
> + }
> +
> + xmit->id = cpu_to_le32(txid);
> + xmit->dlc = cf->len;
> +
> + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
No need to check ctrlmode
> + can_is_canfd_skb(priv->tx_skb)) {
> + xmit->flag |= NCT6694_CAN_FLAG_FD;
> + if (cf->flags & CANFD_BRS)
> + xmit->flag |= NCT6694_CAN_FLAG_BRS;
> + }
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + xmit->flag |= NCT6694_CAN_FLAG_RTR;
you can move this into the !can_is_canfd_skb branch of the if
> +
> + memcpy(xmit->data, cf->data, cf->len);
> +
> + nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD10_OFFSET(1),
> + sizeof(struct nct6694_can_xmit),
> + xmit);
> +}
> +
> +static void nct6694_can_tx_work(struct work_struct *work)
> +{
> + struct nct6694_can_priv *priv = container_of(work,
> + struct nct6694_can_priv,
> + tx_work);
> + struct net_device *ndev = priv->ndev;
> + struct canfd_frame *cf;
> +
> + guard(mutex)(&priv->lock);
> +
> + if (priv->tx_skb) {
> + if (priv->can.state == CAN_STATE_BUS_OFF) {
> + nct6694_can_clean(ndev);
> + } else {
> + cf = (struct canfd_frame *)priv->tx_skb->data;
> + nct6694_can_tx(ndev, cf);
> + priv->tx_busy = true;
> + can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
> + priv->tx_skb = NULL;
> + }
> + }
> +}
> +
> +static const struct net_device_ops nct6694_can_netdev_ops = {
> + .ndo_open = nct6694_can_open,
> + .ndo_stop = nct6694_can_stop,
> + .ndo_start_xmit = nct6694_can_start_xmit,
> + .ndo_change_mtu = can_change_mtu,
> +};
> +
> +static const struct ethtool_ops nct6694_can_ethtool_ops = {
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
> +
> +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> +{
> + struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
> + int ret;
> +
> + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> + NCT6694_CAN_CMD1_OFFSET,
> + sizeof(struct nct6694_can_information),
> + info);
> + if (ret)
> + return ret;
> +
> + return le32_to_cpu(info->can_clk);
> +}
> +
> +static int nct6694_can_probe(struct platform_device *pdev)
> +{
> + const struct mfd_cell *cell = mfd_get_cell(pdev);
> + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> + struct nct6694_can_priv *priv;
> + struct net_device *ndev;
> + int ret, irq, can_clk;
> +
> + irq = irq_create_mapping(nct6694->domain,
> + NCT6694_IRQ_CAN1 + cell->id);
> + if (!irq)
> + return -EINVAL;
propagate error value
> +
> + ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> + if (!ndev)
> + return -ENOMEM;
> +
> + ndev->irq = irq;
> + ndev->flags |= IFF_ECHO;
> + ndev->netdev_ops = &nct6694_can_netdev_ops;
> + ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> +
> + priv = netdev_priv(ndev);
> + priv->nct6694 = nct6694;
> + priv->ndev = ndev;
> +
> + priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> + sizeof(unsigned char), GFP_KERNEL);
devm_kzalloc()
> + if (!priv->tx_buf) {
> + ret = -ENOMEM;
> + goto free_candev;
> + }
> +
> + priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> + sizeof(unsigned char), GFP_KERNEL);
devm_kzalloc()
> + if (!priv->rx_buf) {
> + ret = -ENOMEM;
> + goto free_candev;
> + }
> +
> + can_clk = nct6694_can_get_clock(priv);
> + if (can_clk < 0) {
> + ret = -EIO;
propagate the error value, don't overwrite it
move the dev_err_probe() here.
> + goto free_candev;
> + }
> +
> + mutex_init(&priv->lock);
> + INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> +
> + priv->can_idx = cell->id;
> + priv->can.state = CAN_STATE_STOPPED;
> + priv->can.clock.freq = can_clk;
> + priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> + priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> + priv->can.do_set_mode = nct6694_can_set_mode;
> + priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> +
> + priv->can.ctrlmode = CAN_CTRLMODE_FD;
> +
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> + CAN_CTRLMODE_LISTENONLY |
> + CAN_CTRLMODE_FD |
> + CAN_CTRLMODE_FD_NON_ISO |
> + CAN_CTRLMODE_BERR_REPORTING;
> +
> + platform_set_drvdata(pdev, priv);
> + SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> +
> + ret = register_candev(priv->ndev);
> + if (ret)
> + goto free_candev;
> +
> + return 0;
> +
> +free_candev:
> + free_candev(ndev);
> + return dev_err_probe(&pdev->dev, ret, "Probe failed\n");
Move the dev_err_probe() with an appropriate error message to where the
error occurs. If malloc fails, the kernel already prints for you, so
here it's only nct6694_can_get_clock() only.
> +}
> +
> +static void nct6694_can_remove(struct platform_device *pdev)
> +{
> + struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> +
> + cancel_work_sync(&priv->tx_work);
> + unregister_candev(priv->ndev);
> + free_candev(priv->ndev);
> +}
> +
> +static struct platform_driver nct6694_can_driver = {
> + .driver = {
> + .name = DRVNAME,
> + },
> + .probe = nct6694_can_probe,
> + .remove = nct6694_can_remove,
> +};
> +
> +module_platform_driver(nct6694_can_driver);
> +
> +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:nct6694-can");
> --
> 2.34.1
>
>
>
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Dear Marc,
Thank you for your comments,
Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年12月11日 週三 下午5:59寫道：
>
> On 10.12.2024 18:45:21, Ming Yu wrote:
> > This driver supports Socket CANfd functionality for NCT6694 MFD
> > device based on USB interface.
>
> Please use the rx-offload helper, otherwise the CAN frames might be
> revived out of order.
>
Understood. I'll make the modifications in the next patch.
> >
...
> > +
> > +struct __packed nct6694_can_event {
> > + u8 err1;
> > + u8 status1;
> > + u8 tx_evt1;
> > + u8 rx_evt1;
> > + u8 rec1;
> > + u8 tec1;
> > + u8 reserved1[2];
> > + u8 err2;
> > + u8 status2;
> > + u8 tx_evt2;
> > + u8 rx_evt2;
> > + u8 rec2;
> > + u8 tec2;
> > + u8 reserved2[2];
> > +};
>
> Create an extra struct that only describes a channel
>
> struct __packed nct6694_can_event_channel {
> u8 err;
> u8 status;
> u8 tx_evt;
> u8 rx_evt;
> u8 rec;
> u8 tec;
> u8 reserved[2];
> }
>
> and put an array of 2 into struct __packed nct6694_can_event.
>
Fix it in v4.
> > +
> > +struct __packed nct6694_can_xmit {
> > + u8 tag;
> > + u8 flag;
> > + u8 reserved;
> > + u8 dlc;
> > + __le32 id;
> > + u8 data[64];
> > + u8 msg_buf[72];
>
> Why is the message so long? What's in the msg_buf?
>
It's deprecated, I'll drop it in v4.
> > +};
> > +
> > +struct nct6694_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct net_device *ndev;
> > + struct nct6694 *nct6694;
> > + struct mutex lock;
>
> What does lock protect?
>
The lock is used to protect tx_buf and rx_buf for each CAN device.
> > + struct sk_buff *tx_skb;
> > + struct workqueue_struct *wq;
> > + struct work_struct tx_work;
> > + unsigned char *tx_buf;
> void *
> > + unsigned char *rx_buf;
> void *
> > + unsigned char can_idx;
> > + bool tx_busy;
>
> IMHO it makes no sense to have tx_skb and tx_busy
>
Okay! I will revisit these to evaluate whether they are still necessary.
> > +};
> > +
...
> > +static void nct6694_can_read_fifo(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct canfd_frame *cf;
> > + struct sk_buff *skb;
> > + int can_idx = priv->can_idx;
> > + u32 id;
> > + int ret;
> > + u8 fd_format = 0;
> bool - no need to init
Fix it in v4.
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
> > + sizeof(struct nct6694_can_xmit), xmit);
> > + if (ret < 0)
> > + return;
> > +
> > + /* Check type of frame and create skb */
> > + fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
> > + if (fd_format)
> > + skb = alloc_canfd_skb(ndev, &cf);
> > + else
> > + skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
> > +
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
> > + }
> > +
> > + cf->len = xmit->dlc;
>
> what does xmit->dlc contain? The DLC or the length?
>
It reflects the actual data length.
> > +
> > + /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
> > + id = le32_to_cpu(xmit->id);
> > + if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
> > + /*
> > + * In case the Extended ID frame is received, the standard
> > + * and extended part of the ID are swapped in the register,
> > + * so swap them back to obtain the correct ID.
> > + */
>
> You comment doesn't match the code.
>
Fix it in v4.
> > + id |= CAN_EFF_FLAG;
> > + }
> > +
> > + cf->can_id = id;
> > +
> > + /* Set ESI flag */
> > + if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
> > + cf->flags |= CANFD_ESI;
> > + netdev_dbg(ndev, "ESI Error\n");
> > + }
> > +
> > + /* Set RTR and BRS */
> > + if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + if (xmit->flag & NCT6694_CAN_FLAG_BRS)
> > + cf->flags |= CANFD_BRS;
> > +
> > + memcpy(cf->data, xmit->data, cf->len);
> > +
> > + stats->rx_bytes += cf->len;
> > + }
> > +
> > + stats->rx_packets++;
> > +
> > + netif_receive_skb(skb);
> > +}
...
> > +static int nct6694_can_poll(struct net_device *ndev, int quota)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + int can_idx = priv->can_idx;
> > + int work_done = 0, ret;
> > + u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
> > + u8 bus_err, can_status;
> > +
> > + scoped_guard(mutex, &priv->lock) {
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
> > + sizeof(struct nct6694_can_event), evt);
> > + if (ret < 0)
> > + return IRQ_NONE;
>
> propagate the error
>
Fix it in v4.
> > +
> > + if (can_idx) {
> > + bus_err = evt->err2;
> > + can_status = evt->status2;
> > + } else {
> > + bus_err = evt->err1;
> > + can_status = evt->status1;
> > + }
> > + }
> > +
...
> > +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> > + struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (priv->tx_skb || priv->tx_busy) {
> > + netdev_err(ndev, "hard_xmit called while tx busy\n");
> > + return NETDEV_TX_BUSY;
> > + }
> > +
> > + if (can_dev_dropped_skb(ndev, skb))
> > + return NETDEV_TX_OK;
>
> please drop first
>
Fix it in v4.
> > +
> > + netif_stop_queue(ndev);
> > + priv->tx_skb = skb;
> > + queue_work(priv->wq, &priv->tx_work);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
> > + u32 txid = 0;
> > +
> > + memset(xmit, 0, sizeof(struct nct6694_can_xmit));
> > +
> > + if (priv->can_idx == 0)
> > + xmit->tag = NCT6694_CAN_TAG_CAN0<;
> > + else
> > + xmit->tag = NCT6694_CAN_TAG_CAN1;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + txid = cf->can_id & CAN_EFF_MASK;
> > + /*
> > + * In case the Extended ID frame is transmitted, the
> > + * standard and extended part of the ID are swapped
> > + * in the register, so swap them back to send the
> > + * correct ID.
>
> You comment doesn't match the code.
>
Fix it in v4.
> > + */
> > + xmit->flag |= NCT6694_CAN_FLAG_EFF;
> > + } else {
> > + txid = cf->can_id & CAN_SFF_MASK;
> > + }
> > +
> > + xmit->id = cpu_to_le32(txid);
> > + xmit->dlc = cf->len;
> > +
> > + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
>
> No need to check ctrlmode
>
Fix it in v4.
> > + can_is_canfd_skb(priv->tx_skb)) {
> > + xmit->flag |= NCT6694_CAN_FLAG_FD;
> > + if (cf->flags & CANFD_BRS)
> > + xmit->flag |= NCT6694_CAN_FLAG_BRS;
> > + }
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + xmit->flag |= NCT6694_CAN_FLAG_RTR;
>
> you can move this into the !can_is_canfd_skb branch of the if
>
Fix it in v4.
> > +
> > + memcpy(xmit->data, cf->data, cf->len);
> > +
> > + nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD10_OFFSET(1),
> > + sizeof(struct nct6694_can_xmit),
> > + xmit);
> > +}
> > +
> > +static void nct6694_can_tx_work(struct work_struct *work)
> > +{
> > + struct nct6694_can_priv *priv = container_of(work,
> > + struct nct6694_can_priv,
> > + tx_work);
> > + struct net_device *ndev = priv->ndev;
> > + struct canfd_frame *cf;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + if (priv->tx_skb) {
> > + if (priv->can.state == CAN_STATE_BUS_OFF) {
> > + nct6694_can_clean(ndev);
> > + } else {
> > + cf = (struct canfd_frame *)priv->tx_skb->data;
> > + nct6694_can_tx(ndev, cf);
> > + priv->tx_busy = true;
> > + can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
> > + priv->tx_skb = NULL;
> > + }
> > + }
> > +}
> > +
> > +static const struct net_device_ops nct6694_can_netdev_ops = {
> > + .ndo_open = nct6694_can_open,
> > + .ndo_stop = nct6694_can_stop,
> > + .ndo_start_xmit = nct6694_can_start_xmit,
> > + .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static const struct ethtool_ops nct6694_can_ethtool_ops = {
> > + .get_ts_info = ethtool_op_get_ts_info,
> > +};
> > +
> > +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> > +{
> > + struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
> > + int ret;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD1_OFFSET,
> > + sizeof(struct nct6694_can_information),
> > + info);
> > + if (ret)
> > + return ret;
> > +
> > + return le32_to_cpu(info->can_clk);
> > +}
> > +
> > +static int nct6694_can_probe(struct platform_device *pdev)
> > +{
> > + const struct mfd_cell *cell = mfd_get_cell(pdev);
> > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > + struct nct6694_can_priv *priv;
> > + struct net_device *ndev;
> > + int ret, irq, can_clk;
> > +
> > + irq = irq_create_mapping(nct6694->domain,
> > + NCT6694_IRQ_CAN1 + cell->id);
> > + if (!irq)
> > + return -EINVAL;
>
> propagate error value
>
Fix it in v4.
> > +
> > + ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> > + if (!ndev)
> > + return -ENOMEM;
> > +
> > + ndev->irq = irq;
> > + ndev->flags |= IFF_ECHO;
> > + ndev->netdev_ops = &nct6694_can_netdev_ops;
> > + ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->nct6694 = nct6694;
> > + priv->ndev = ndev;
> > +
> > + priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
>
> devm_kzalloc()
> > + if (!priv->tx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
> devm_kzalloc()
>
Fix it in v4.
> > + if (!priv->rx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + can_clk = nct6694_can_get_clock(priv);
> > + if (can_clk < 0) {
> > + ret = -EIO;
>
> propagate the error value, don't overwrite it
>
> move the dev_err_probe() here.
>
Fix it in v4.
> > + goto free_candev;
> > + }
> > +
> > + mutex_init(&priv->lock);
> > + INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> > +
> > + priv->can_idx = cell->id;
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = can_clk;
> > + priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> > + priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> > + priv->can.do_set_mode = nct6694_can_set_mode;
> > + priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> > +
> > + priv->can.ctrlmode = CAN_CTRLMODE_FD;
> > +
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_FD |
> > + CAN_CTRLMODE_FD_NON_ISO |
> > + CAN_CTRLMODE_BERR_REPORTING;
> > +
> > + platform_set_drvdata(pdev, priv);
> > + SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> > +
> > + ret = register_candev(priv->ndev);
> > + if (ret)
> > + goto free_candev;
> > +
> > + return 0;
> > +
> > +free_candev:
> > + free_candev(ndev);
> > + return dev_err_probe(&pdev->dev, ret, "Probe failed\n");
>
> Move the dev_err_probe() with an appropriate error message to where the
> error occurs. If malloc fails, the kernel already prints for you, so
> here it's only nct6694_can_get_clock() only.
>
Understood. I'll make the modifications in the next patch.
> > +}
> > +
> > +static void nct6694_can_remove(struct platform_device *pdev)
> > +{
> > + struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> > +
> > + cancel_work_sync(&priv->tx_work);
> > + unregister_candev(priv->ndev);
> > + free_candev(priv->ndev);
> > +}
> > +
> > +static struct platform_driver nct6694_can_driver = {
> > + .driver = {
> > + .name = DRVNAME,
> > + },
> > + .probe = nct6694_can_probe,
> > + .remove = nct6694_can_remove,
> > +};
> > +
> > +module_platform_driver(nct6694_can_driver);
> > +
> > +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:nct6694-can");
> > --
> > 2.34.1
> >
> >
> >
>
> regards,
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung Nürnberg | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Best regards,
Ming
Hi Marc,
> > > +struct nct6694_can_priv {
> > > + struct can_priv can; /* must be the first member */
> > > + struct net_device *ndev;
> > > + struct nct6694 *nct6694;
> > > + struct mutex lock;
> >
> > What does lock protect?
> >
>
> The lock is used to protect tx_buf and rx_buf for each CAN device.
>
> > > + struct sk_buff *tx_skb;
> > > + struct workqueue_struct *wq;
> > > + struct work_struct tx_work;
> > > + unsigned char *tx_buf;
> > void *
> > > + unsigned char *rx_buf;
> > void *
> > > + unsigned char can_idx;
> > > + bool tx_busy;
> >
> > IMHO it makes no sense to have tx_skb and tx_busy
> >
>
> Okay! I will revisit these to evaluate whether they are still necessary.
>
> > > +};
> > > +
I think there needs to be a tx_skb to record the skb passed by
start_xmit(), otherwise it can't handle the can_frame in tx_work. If
this is not necessary, could you please explain?
In addition, the tx flow is based on the implementation in
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/net/can/spi/mcp251x.c
Thanks,
Ming
On Wed. 11 Dec. 2024 at 18:59, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 10.12.2024 18:45:21, Ming Yu wrote:
> > This driver supports Socket CANfd functionality for NCT6694 MFD
> > device based on USB interface.
>
> Please use the rx-offload helper, otherwise the CAN frames might be
> revived out of order.
>
> >
> > Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/net/can/Kconfig | 10 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/nct6694_canfd.c | 920 ++++++++++++++++++++++++++++++++
> > 4 files changed, 932 insertions(+)
> > create mode 100644 drivers/net/can/nct6694_canfd.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a190f2b08fa3..eb5d46825e71 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16548,6 +16548,7 @@ S: Supported
> > F: drivers/gpio/gpio-nct6694.c
> > F: drivers/i2c/busses/i2c-nct6694.c
> > F: drivers/mfd/nct6694.c
> > +F: drivers/net/can/nct6694_canfd.c
> > F: include/linux/mfd/nct6694.h
> >
> > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index cf989bea9aa3..130e98ec28a5 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -200,6 +200,16 @@ config CAN_SUN4I
> > To compile this driver as a module, choose M here: the module will
> > be called sun4i_can.
> >
> > +config CAN_NCT6694
> > + tristate "Nuvoton NCT6694 Socket CANfd support"
> > + depends on MFD_NCT6694
> > + help
> > + If you say yes to this option, support will be included for Nuvoton
> > + NCT6694, a USB device to socket CANfd controller.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called nct6694_canfd.
> > +
> > config CAN_TI_HECC
> > depends on ARM
> > tristate "TI High End CAN Controller"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> > index a71db2cfe990..4a6b5b9d6c2b 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
> > obj-$(CONFIG_CAN_KVASER_PCIEFD) += kvaser_pciefd.o
> > obj-$(CONFIG_CAN_MSCAN) += mscan/
> > obj-$(CONFIG_CAN_M_CAN) += m_can/
> > +obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o
> > obj-$(CONFIG_CAN_PEAK_PCIEFD) += peak_canfd/
> > obj-$(CONFIG_CAN_SJA1000) += sja1000/
> > obj-$(CONFIG_CAN_SUN4I) += sun4i_can.o
> > diff --git a/drivers/net/can/nct6694_canfd.c b/drivers/net/can/nct6694_canfd.c
> > new file mode 100644
> > index 000000000000..54f20f0681e2
> > --- /dev/null
> > +++ b/drivers/net/can/nct6694_canfd.c
> > @@ -0,0 +1,920 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NCT6694 Socket CANfd driver based on USB interface.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#include <linux/can/dev.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRVNAME "nct6694-can"
> > +
> > +/* Host interface */
> > +#define NCT6694_CAN_MOD 0x05
> > +
> > +/* Message Channel*/
> > +/* Command 00h */
Instead of this comment, explain what the command 00h does.
Also better to give a memorable name instead of CMD0. For example:
CMD_TX, CMD_RX…
If possible, make it match the structure names.
> > +#define NCT6694_CAN_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000)
> > +#define NCT6694_CAN_CTRL1_MON BIT(0)
> > +#define NCT6694_CAN_CTRL1_NISO BIT(1)
> > +#define NCT6694_CAN_CTRL1_LBCK BIT(2)
> > +
> > +/* Command 01h */
> > +#define NCT6694_CAN_CMD1_OFFSET 0x0001
> > +
> > +/* Command 02h */
> > +#define NCT6694_CAN_CMD2_OFFSET(idx, mask) \
> > + ({ typeof(mask) mask_ = (mask); \
> > + idx ? ((0x80 | (mask_ & 0xFF)) << 8 | 0x02) : \
> > + ((0x00 | (mask_ & 0xFF)) << 8 | 0x02); })
> > +
> > +#define NCT6694_CAN_EVENT_ERR BIT(0)
> > +#define NCT6694_CAN_EVENT_STATUS BIT(1)
> > +#define NCT6694_CAN_EVENT_TX_EVT BIT(2)
> > +#define NCT6694_CAN_EVENT_RX_EVT BIT(3)
> > +#define NCT6694_CAN_EVENT_REC BIT(4)
> > +#define NCT6694_CAN_EVENT_TEC BIT(5)
> > +#define NCT6694_CAN_EVT_TX_FIFO_EMPTY BIT(7) /* Read-clear */
> > +#define NCT6694_CAN_EVT_RX_DATA_LOST BIT(5) /* Read-clear */
> > +#define NCT6694_CAN_EVT_RX_HALF_FULL BIT(6) /* Read-clear */
> > +#define NCT6694_CAN_EVT_RX_DATA_IN BIT(7)
> > +
> > +/* Command 10h */
> > +#define NCT6694_CAN_CMD10_OFFSET(buf_cnt) \
> > + (((buf_cnt) & 0xFF) << 8 | 0x10)
> > +#define NCT6694_CAN_TAG_CAN0 0xC0
> > +#define NCT6694_CAN_TAG_CAN1 0xC1
> > +#define NCT6694_CAN_FLAG_EFF BIT(0)
> > +#define NCT6694_CAN_FLAG_RTR BIT(1)
> > +#define NCT6694_CAN_FLAG_FD BIT(2)
> > +#define NCT6694_CAN_FLAG_BRS BIT(3)
> > +#define NCT6694_CAN_FLAG_ERR BIT(4)
> > +
> > +/* Command 11h */
> > +#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
> > + ({ typeof(buf_cnt) buf_cnt_ = (buf_cnt); \
> > + idx ? ((0x80 | (buf_cnt_ & 0xFF)) << 8 | 0x11) : \
> > + ((0x00 | (buf_cnt_ & 0xFF)) << 8 | 0x11); })
Simplify this. Do something like:
#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
(idx ? 0x80 : 0x00) | \
(buf_cnt & 0xFF)) << 8 | 0x11) \
(apply this also to NCT6694_CAN_CMD2_OFFSET())
> > +#define NCT6694_CAN_RX_QUOTA 64
> > +
> > +enum nct6694_event_err {
> > + NCT6694_CAN_EVT_NO_ERROR,
> ^^^ add _ERR_
> > + NCT6694_CAN_EVT_CRC_ERROR,
> > + NCT6694_CAN_EVT_STUFF_ERROR,
> > + NCT6694_CAN_EVT_ACK_ERROR,
> > + NCT6694_CAN_EVT_FORM_ERROR,
> > + NCT6694_CAN_EVT_BIT_ERROR,
> > + NCT6694_CAN_EVT_TIMEOUT_ERROR,
> > + NCT6694_CAN_EVT_UNKNOWN_ERROR,
> > +};
> > +
> > +enum nct6694_event_status {
> > + NCT6694_CAN_EVT_ERROR_ACTIVE,
> ^^^ add _STATUS_
> > + NCT6694_CAN_EVT_ERROR_PASSIVE,
> > + NCT6694_CAN_EVT_BUS_OFF,
> > + NCT6694_CAN_EVT_WARNING,
> > +};
> > +
> > +struct __packed nct6694_can_setting {
> > + __le32 nbr;
> > + __le32 dbr;
> > + u8 active;
> > + u8 reserved[3];
> > + __le16 ctrl1;
> > + __le16 ctrl2;
> > + __le32 nbtp;
> > + __le32 dbtp;
> > +};
> > +
> > +struct __packed nct6694_can_information {
> > + u8 tx_fifo_cnt;
> > + u8 rx_fifo_cnt;
> > + __le16 reserved;
> u8 reserved[2];
> > + __le32 can_clk;
> > +};
> > +
> > +struct __packed nct6694_can_event {
> > + u8 err1;
> > + u8 status1;
> > + u8 tx_evt1;
> > + u8 rx_evt1;
> > + u8 rec1;
> > + u8 tec1;
> > + u8 reserved1[2];
> > + u8 err2;
> > + u8 status2;
> > + u8 tx_evt2;
> > + u8 rx_evt2;
> > + u8 rec2;
> > + u8 tec2;
> > + u8 reserved2[2];
> > +};
>
> Create an extra struct that only describes a channel
>
> struct __packed nct6694_can_event_channel {
> u8 err;
> u8 status;
> u8 tx_evt;
> u8 rx_evt;
> u8 rec;
> u8 tec;
> u8 reserved[2];
> }
>
> and put an array of 2 into struct __packed nct6694_can_event.
>
> > +
> > +struct __packed nct6694_can_xmit {
Is this struct for both TX and RX? If so, name it something like
struct nct6694_can_frame
The term xmit is only used for transmission, not for reception.
> > + u8 tag;
> > + u8 flag;
> > + u8 reserved;
> > + u8 dlc;
> > + __le32 id;
> > + u8 data[64];
> > + u8 msg_buf[72];
>
> Why is the message so long? What's in the msg_buf?
>
> > +};
> > +
> > +struct nct6694_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct net_device *ndev;
> > + struct nct6694 *nct6694;
> > + struct mutex lock;
>
> What does lock protect?
+1
mutexes are good if you want to keep the lock for a long time.
For short period, spinlock are more performant:
spinlock_t lock;
> > + struct sk_buff *tx_skb;
> > + struct workqueue_struct *wq;
> > + struct work_struct tx_work;
> > + unsigned char *tx_buf;
> void *
> > + unsigned char *rx_buf;
> void *
Rather than void*, tx_buf and rx_buf can be replaced by an union:
union nct6694_can_rx {
struct nct6694_can_event event;
struct nct6694_can_xmit xmit;
struct nct6694_can_information info;
};
(same for nct6694_can_tx)
Then in struct nct6694_can_priv, you will just have:
union nct6694_can_tx tx;
union nct6694_can_rx rx;
With this,
NCT6694_MAX_PACKET_SZ
can most likely be replaced by sizeof(union nct6694_can_rx) or
sizeof(union nct6694_can_tx).
> > + unsigned char can_idx;
> > + bool tx_busy;
>
> IMHO it makes no sense to have tx_skb and tx_busy
>
> > +};
> > +
> > +static const struct can_bittiming_const nct6694_can_bittiming_nominal_const = {
> > + .name = DRVNAME,
> > + .tseg1_min = 2,
> > + .tseg1_max = 256,
> > + .tseg2_min = 2,
> > + .tseg2_max = 128,
> > + .sjw_max = 128,
> > + .brp_min = 1,
> > + .brp_max = 511,
> > + .brp_inc = 1,
> > +};
> > +
> > +static const struct can_bittiming_const nct6694_can_bittiming_data_const = {
> > + .name = DRVNAME,
> > + .tseg1_min = 1,
> > + .tseg1_max = 32,
> > + .tseg2_min = 1,
> > + .tseg2_max = 16,
> > + .sjw_max = 16,
> > + .brp_min = 1,
> > + .brp_max = 31,
> > + .brp_inc = 1,
> > +};
> > +
> > +static void nct6694_can_clean(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (priv->tx_skb || priv->tx_busy)
> > + ndev->stats.tx_errors++;
> > + dev_kfree_skb(priv->tx_skb);
> > + if (priv->tx_busy)
> > + can_free_echo_skb(priv->ndev, 0, NULL);
> > + priv->tx_skb = NULL;
> > + priv->tx_busy = false;
> > +}
> > +
> > +static int nct6694_can_get_berr_counter(const struct net_device *ndev,
> > + struct can_berr_counter *bec)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC;
> > + int ret;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(priv->can_idx, mask),
> > + sizeof(struct nct6694_can_event),
> > + evt);
> > + if (ret < 0)
> > + return ret;
> > +
> > + bec->rxerr = priv->can_idx ? evt->rec2 : evt->rec1;
> > + bec->txerr = priv->can_idx ? evt->tec2 : evt->tec1;
> > +
> > + return 0;
> > +}
> > +
> > +static int nct6694_can_handle_lost_msg(struct net_device *ndev)
> > +{
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct sk_buff *skb;
> > + struct can_frame *frame;
> > +
> > + netdev_err(ndev, "RX FIFO overflow, message(s) lost.\n");
> > +
> > + stats->rx_errors++;
> > + stats->rx_over_errors++;
> > +
> > + skb = alloc_can_err_skb(ndev, &frame);
> > + if (unlikely(!skb))
> > + return 0;
> > +
> > + frame->can_id |= CAN_ERR_CRTL;
> > + frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +
> > + netif_receive_skb(skb);
> > +
> > + return 1;
> > +}
> > +
> > +static void nct6694_can_read_fifo(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct canfd_frame *cf;
> > + struct sk_buff *skb;
> > + int can_idx = priv->can_idx;
> > + u32 id;
> > + int ret;
> > + u8 fd_format = 0;
> bool - no need to init
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
> > + sizeof(struct nct6694_can_xmit), xmit);
> > + if (ret < 0)
> > + return;
> > +
> > + /* Check type of frame and create skb */
> > + fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
> > + if (fd_format)
> > + skb = alloc_canfd_skb(ndev, &cf);
> > + else
> > + skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
> > +
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
> > + }
> > +
> > + cf->len = xmit->dlc;
>
> what does xmit->dlc contain? The DLC or the length?
+1
Also, do not trust the device data. Even if SPI attacks are less
common, make sure to sanitize this length.
cf->len = canfd_sanitize_len(xmit->dlc);
Or
cf->len = canfd_sanitize_len(xmit->dlc);
if xmit->dlc is in fact a DLC.
> > +
> > + /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
> > + id = le32_to_cpu(xmit->id);
> > + if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
> > + /*
> > + * In case the Extended ID frame is received, the standard
> > + * and extended part of the ID are swapped in the register,
> > + * so swap them back to obtain the correct ID.
> > + */
>
> You comment doesn't match the code.
>
> > + id |= CAN_EFF_FLAG;
> > + }
> > +
> > + cf->can_id = id;
> > +
> > + /* Set ESI flag */
> > + if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
> > + cf->flags |= CANFD_ESI;
> > + netdev_dbg(ndev, "ESI Error\n");
> > + }
> > +
> > + /* Set RTR and BRS */
> > + if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + if (xmit->flag & NCT6694_CAN_FLAG_BRS)
> > + cf->flags |= CANFD_BRS;
> > +
> > + memcpy(cf->data, xmit->data, cf->len);
> > +
> > + stats->rx_bytes += cf->len;
> > + }
> > +
> > + stats->rx_packets++;
> > +
> > + netif_receive_skb(skb);
> > +}
> > +
> > +static int nct6694_can_do_rx_poll(struct net_device *ndev, int quota)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + int can_idx = priv->can_idx;
> > + u32 pkts = 0;
> > + u8 mask_rx = NCT6694_CAN_EVENT_RX_EVT;
> > + u8 rx_evt;
> > +
> > + for (;;) {
> > + scoped_guard(mutex, &priv->lock) {
> > + nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(can_idx, mask_rx),
> > + sizeof(struct nct6694_can_event), evt);
> > +
> > + rx_evt = can_idx ? evt->rx_evt2 : evt->rx_evt1;
> > + }
> > +
> > + if (rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST)
> > + nct6694_can_handle_lost_msg(ndev);
> > +
> > + /* No data */
> > + if ((rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) == 0)
> > + break;
> > +
> > + if (quota <= 0)
> > + break;
> > +
> > + nct6694_can_read_fifo(ndev);
> > + quota--;
> > + pkts++;
> > + }
> > +
> > + return pkts;
> > +}
> > +
> > +static int nct6694_can_handle_lec_err(struct net_device *ndev, u8 bus_err)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > +
> > + if (bus_err == NCT6694_CAN_EVT_NO_ERROR)
> > + return 0;
> > +
> > + priv->can.can_stats.bus_error++;
> > + stats->rx_errors++;
> > +
> > + /* Propagate the error condition to the CAN stack. */
> > + skb = alloc_can_err_skb(ndev, &cf);
> > +
> > + if (unlikely(!skb))
> > + return 0;
Do not exit if the memory allocation fails. Instead, do the error
handling to increase the statistics.
Look at what other CAN drivers are doing.
> > + /* Read the error counter register and check for new errors. */
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > + switch (bus_err) {
> > + case NCT6694_CAN_EVT_CRC_ERROR:
> > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_STUFF_ERROR:
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_ACK_ERROR:
> > + cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_FORM_ERROR:
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_BIT_ERROR:
> > + cf->data[2] |= CAN_ERR_PROT_BIT |
> > + CAN_ERR_PROT_BIT0 |
> > + CAN_ERR_PROT_BIT1;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_TIMEOUT_ERROR:
> > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > + break;
> > +
> > + case NCT6694_CAN_EVT_UNKNOWN_ERROR:
> > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > + /*
> > + * It means 'unspecified'(the value is '0').
> > + * But it is not sure if it's ok to send an error package
> > + * without specific error bit.
> > + */
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + /* Reset the error counter, ack the IRQ and re-enable the counter. */
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
The CAN error frames are not regular packets. Do not increase the RX
stats. Instead, increase the error stats.
> > + netif_receive_skb(skb);
> > +
> > + return 1;
> > +}
> > +
> > +static int nct6694_can_handle_state_change(struct net_device *ndev,
> > + enum can_state new_state)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct can_berr_counter bec;
> > +
> > + switch (new_state) {
> > + case CAN_STATE_ERROR_ACTIVE:
> > + priv->can.can_stats.error_warning++;
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + break;
> > + case CAN_STATE_ERROR_WARNING:
> > + priv->can.can_stats.error_warning++;
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + break;
> > + case CAN_STATE_ERROR_PASSIVE:
> > + priv->can.can_stats.error_passive++;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + break;
> > + case CAN_STATE_BUS_OFF:
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + priv->can.can_stats.bus_off++;
> > + can_bus_off(ndev);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + /* propagate the error condition to the CAN stack */
> > + skb = alloc_can_err_skb(ndev, &cf);
> > + if (unlikely(!skb))
> > + return 0;
Same as above: handle the statistics even if the allocation fails.
> > + nct6694_can_get_berr_counter(ndev, &bec);
> > +
> > + switch (new_state) {
> > + case CAN_STATE_ERROR_WARNING:
> > + /* error warning state */
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
> > + CAN_ERR_CRTL_RX_WARNING;
Prefer an if/else here instead of the ternary operator. It is more readable.
> > + cf->data[6] = bec.txerr;
> > + cf->data[7] = bec.rxerr;
> > + break;
> > + case CAN_STATE_ERROR_PASSIVE:
> > + /* error passive state */
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > + if (bec.txerr > 127)
> > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > + cf->data[6] = bec.txerr;
> > + cf->data[7] = bec.rxerr;
> > + break;
> > + case CAN_STATE_BUS_OFF:
> > + /* bus-off state */
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
Do not increase the RX stats when you receive a CAN event.
> > + netif_receive_skb(skb);
> > +
> > + return 1;
> > +}
> > +
> > +static int nct6694_can_handle_state_errors(struct net_device *ndev,
> > + unsigned char can_status)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + int work_done = 0;
> > +
> > + if (can_status == NCT6694_CAN_EVT_ERROR_ACTIVE &&
> > + priv->can.state != CAN_STATE_ERROR_ACTIVE) {
> > + netdev_dbg(ndev, "Error, entered active state\n");
> > + work_done += nct6694_can_handle_state_change(ndev,
> > + CAN_STATE_ERROR_ACTIVE);
> > + }
> > +
> > + if (can_status == NCT6694_CAN_EVT_WARNING &&
> > + priv->can.state != CAN_STATE_ERROR_WARNING) {
> > + netdev_dbg(ndev, "Error, entered warning state\n");
> > + work_done += nct6694_can_handle_state_change(ndev,
> > + CAN_STATE_ERROR_WARNING);
> > + }
> > +
> > + if (can_status == NCT6694_CAN_EVT_ERROR_PASSIVE &&
> > + priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> > + netdev_dbg(ndev, "Error, entered passive state\n");
> > + work_done += nct6694_can_handle_state_change(ndev,
> > + CAN_STATE_ERROR_PASSIVE);
> > + }
> > +
> > + if (can_status == NCT6694_CAN_EVT_BUS_OFF &&
> > + priv->can.state != CAN_STATE_BUS_OFF) {
> > + netdev_dbg(ndev, "Error, entered bus-off state\n");
> > + work_done += nct6694_can_handle_state_change(ndev,
> > + CAN_STATE_BUS_OFF);
> > + }
> > +
> > + return work_done;
> > +}
> > +
> > +static int nct6694_can_poll(struct net_device *ndev, int quota)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + int can_idx = priv->can_idx;
> > + int work_done = 0, ret;
> > + u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
> > + u8 bus_err, can_status;
> > +
> > + scoped_guard(mutex, &priv->lock) {
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
> > + sizeof(struct nct6694_can_event), evt);
> > + if (ret < 0)
> > + return IRQ_NONE;
>
> propagate the error
>
> > +
> > + if (can_idx) {
> > + bus_err = evt->err2;
> > + can_status = evt->status2;
> > + } else {
> > + bus_err = evt->err1;
> > + can_status = evt->status1;
> > + }
> > + }
> > +
> > + /* Handle bus state changes */
> > + work_done += nct6694_can_handle_state_errors(ndev, can_status);
> > +
> > + /* Handle lec errors on the bus */
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> > + work_done += nct6694_can_handle_lec_err(ndev, bus_err);
> > +
> > + /* Handle RX events */
> > + work_done += nct6694_can_do_rx_poll(ndev, quota - work_done);
> > + return work_done;
> > +}
> > +
> > +static void nct6694_can_tx_irq(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > +
> > + guard(mutex)(&priv->lock);
> > + stats->tx_bytes += can_get_echo_skb(ndev, 0, NULL);
> > + stats->tx_packets++;
> > + priv->tx_busy = false;
> > + netif_wake_queue(ndev);
> > +}
> > +
> > +static irqreturn_t nct6694_can_irq(int irq, void *data)
> > +{
> > + struct net_device *ndev = data;
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + int can_idx = priv->can_idx;
> > + int ret;
> > + u8 mask_sts = NCT6694_CAN_EVENT_TX_EVT;
> > + u8 tx_evt;
> > +
> > + scoped_guard(mutex, &priv->lock) {
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(can_idx, mask_sts),
> > + sizeof(struct nct6694_can_event), evt);
> > + if (ret < 0)
> > + return IRQ_NONE;
> > +
> > + tx_evt = can_idx ? evt->tx_evt2 : evt->tx_evt1;
> > + }
> > +
> > + if (tx_evt) {
> > + nct6694_can_tx_irq(ndev);
> > + } else {
> > + ret = nct6694_can_poll(ndev, NCT6694_CAN_RX_QUOTA);
> > + if (!ret)
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int nct6694_can_start(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_setting *setting = (struct nct6694_can_setting *)priv->tx_buf;
> > + const struct can_bittiming *n_bt = &priv->can.bittiming;
> > + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > + int ret;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + memset(priv->tx_buf, 0, sizeof(struct nct6694_can_setting));
When you memset(), use this pattern:
memset(setting, 0, sizeof(*setting));
Apply this throughout your driver.
> > + setting->nbr = cpu_to_le32(n_bt->bitrate);
> > + setting->dbr = cpu_to_le32(d_bt->bitrate);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_MON);
> > +
> > + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > + priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_NISO);
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_CTRL1_LBCK);
> > +
> > + ret = nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD0_OFFSET(priv->can_idx),
> > + sizeof(struct nct6694_can_setting), setting);
> > + if (ret < 0)
> > + return ret;
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + return 0;
> > +}
> > +
> > +static int nct6694_can_stop(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + netif_stop_queue(ndev);
> > + free_irq(ndev->irq, ndev);
> > + destroy_workqueue(priv->wq);
> > + priv->wq = NULL;
> > + nct6694_can_clean(ndev);
> > + priv->can.state = CAN_STATE_STOPPED;
> > + close_candev(ndev);
> > +
> > + return 0;
> > +}
> > +
> > +static int nct6694_can_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + nct6694_can_clean(ndev);
> > + nct6694_can_start(ndev);
> > + netif_wake_queue(ndev);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct6694_can_open(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
> > + ret = open_candev(ndev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = request_threaded_irq(ndev->irq, NULL,
> > + nct6694_can_irq, IRQF_ONESHOT,
> > + "nct6694_can", ndev);
> > + if (ret) {
> > + netdev_err(ndev, "Failed to request IRQ\n");
> > + goto close_candev;
> > + }
> > +
> > + priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
> > + WQ_FREEZABLE | WQ_MEM_RECLAIM,
> > + ndev->name);
> > + if (!priv->wq) {
> > + ret = -ENOMEM;
> > + goto free_irq;
> > + }
> > +
> > + priv->tx_skb = NULL;
> > + priv->tx_busy = false;
> > +
> > + ret = nct6694_can_start(ndev);
> > + if (ret)
> > + goto destroy_wq;
> > +
> > + netif_start_queue(ndev);
> > +
> > + return 0;
> > +
> > +destroy_wq:
> > + destroy_workqueue(priv->wq);
> > +free_irq:
> > + free_irq(ndev->irq, ndev);
> > +close_candev:
> > + close_candev(ndev);
> > + return ret;
> > +}
> > +
> > +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> > + struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (priv->tx_skb || priv->tx_busy) {
> > + netdev_err(ndev, "hard_xmit called while tx busy\n");
> > + return NETDEV_TX_BUSY;
> > + }
> > +
> > + if (can_dev_dropped_skb(ndev, skb))
> > + return NETDEV_TX_OK;
>
> please drop first
>
> > +
> > + netif_stop_queue(ndev);
> > + priv->tx_skb = skb;
> > + queue_work(priv->wq, &priv->tx_work);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
> > + u32 txid = 0;
> > +
> > + memset(xmit, 0, sizeof(struct nct6694_can_xmit));
> > +
> > + if (priv->can_idx == 0)
> > + xmit->tag = NCT6694_CAN_TAG_CAN0<;
> > + else
> > + xmit->tag = NCT6694_CAN_TAG_CAN1;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + txid = cf->can_id & CAN_EFF_MASK;
> > + /*
> > + * In case the Extended ID frame is transmitted, the
> > + * standard and extended part of the ID are swapped
> > + * in the register, so swap them back to send the
> > + * correct ID.
>
> You comment doesn't match the code.
>
> > + */
> > + xmit->flag |= NCT6694_CAN_FLAG_EFF;
> > + } else {
> > + txid = cf->can_id & CAN_SFF_MASK;
> > + }
> > +
> > + xmit->id = cpu_to_le32(txid);
> > + xmit->dlc = cf->len;
> > +
> > + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
>
> No need to check ctrlmode
>
> > + can_is_canfd_skb(priv->tx_skb)) {
> > + xmit->flag |= NCT6694_CAN_FLAG_FD;
> > + if (cf->flags & CANFD_BRS)
> > + xmit->flag |= NCT6694_CAN_FLAG_BRS;
> > + }
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + xmit->flag |= NCT6694_CAN_FLAG_RTR;
>
> you can move this into the !can_is_canfd_skb branch of the if
>
> > +
> > + memcpy(xmit->data, cf->data, cf->len);
> > +
> > + nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD10_OFFSET(1),
> > + sizeof(struct nct6694_can_xmit),
> > + xmit);
> > +}
> > +
> > +static void nct6694_can_tx_work(struct work_struct *work)
> > +{
> > + struct nct6694_can_priv *priv = container_of(work,
> > + struct nct6694_can_priv,
> > + tx_work);
> > + struct net_device *ndev = priv->ndev;
> > + struct canfd_frame *cf;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + if (priv->tx_skb) {
> > + if (priv->can.state == CAN_STATE_BUS_OFF) {
> > + nct6694_can_clean(ndev);
> > + } else {
> > + cf = (struct canfd_frame *)priv->tx_skb->data;
> > + nct6694_can_tx(ndev, cf);
> > + priv->tx_busy = true;
> > + can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
> > + priv->tx_skb = NULL;
> > + }
> > + }
> > +}
> > +
> > +static const struct net_device_ops nct6694_can_netdev_ops = {
> > + .ndo_open = nct6694_can_open,
> > + .ndo_stop = nct6694_can_stop,
> > + .ndo_start_xmit = nct6694_can_start_xmit,
> > + .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static const struct ethtool_ops nct6694_can_ethtool_ops = {
> > + .get_ts_info = ethtool_op_get_ts_info,
> > +};
> > +
> > +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> > +{
> > + struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
> > + int ret;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD1_OFFSET,
> > + sizeof(struct nct6694_can_information),
> > + info);
> > + if (ret)
> > + return ret;
> > +
> > + return le32_to_cpu(info->can_clk);
> > +}
> > +
> > +static int nct6694_can_probe(struct platform_device *pdev)
> > +{
> > + const struct mfd_cell *cell = mfd_get_cell(pdev);
> > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > + struct nct6694_can_priv *priv;
> > + struct net_device *ndev;
> > + int ret, irq, can_clk;
> > +
> > + irq = irq_create_mapping(nct6694->domain,
> > + NCT6694_IRQ_CAN1 + cell->id);
> > + if (!irq)
> > + return -EINVAL;
>
> propagate error value
>
> > +
> > + ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> > + if (!ndev)
> > + return -ENOMEM;
> > +
> > + ndev->irq = irq;
> > + ndev->flags |= IFF_ECHO;
> > + ndev->netdev_ops = &nct6694_can_netdev_ops;
> > + ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->nct6694 = nct6694;
> > + priv->ndev = ndev;
> > +
> > + priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
>
> devm_kzalloc()
> > + if (!priv->tx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
> devm_kzalloc()
>
> > + if (!priv->rx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + can_clk = nct6694_can_get_clock(priv);
> > + if (can_clk < 0) {
> > + ret = -EIO;
>
> propagate the error value, don't overwrite it
>
> move the dev_err_probe() here.
>
> > + goto free_candev;
> > + }
> > +
> > + mutex_init(&priv->lock);
> > + INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> > +
> > + priv->can_idx = cell->id;
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = can_clk;
> > + priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> > + priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> > + priv->can.do_set_mode = nct6694_can_set_mode;
> > + priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> > +
> > + priv->can.ctrlmode = CAN_CTRLMODE_FD;
> > +
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_FD |
> > + CAN_CTRLMODE_FD_NON_ISO |
> > + CAN_CTRLMODE_BERR_REPORTING;
> > +
> > + platform_set_drvdata(pdev, priv);
> > + SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> > +
> > + ret = register_candev(priv->ndev);
> > + if (ret)
> > + goto free_candev;
> > +
> > + return 0;
> > +
> > +free_candev:
> > + free_candev(ndev);
> > + return dev_err_probe(&pdev->dev, ret, "Probe failed\n");
>
> Move the dev_err_probe() with an appropriate error message to where the
> error occurs. If malloc fails, the kernel already prints for you, so
> here it's only nct6694_can_get_clock() only.
>
> > +}
> > +
> > +static void nct6694_can_remove(struct platform_device *pdev)
> > +{
> > + struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> > +
> > + cancel_work_sync(&priv->tx_work);
> > + unregister_candev(priv->ndev);
> > + free_candev(priv->ndev);
> > +}
> > +
> > +static struct platform_driver nct6694_can_driver = {
> > + .driver = {
> > + .name = DRVNAME,
> > + },
> > + .probe = nct6694_can_probe,
> > + .remove = nct6694_can_remove,
> > +};
> > +
> > +module_platform_driver(nct6694_can_driver);
> > +
> > +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:nct6694-can");
Yours sincerely,
Vincent Mailhol
Dear Vincent,
Thank you for your comments,
Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月11日 週三 下午11:25寫道：
>
...
> > > +/* Host interface */
> > > +#define NCT6694_CAN_MOD 0x05
> > > +
> > > +/* Message Channel*/
> > > +/* Command 00h */
>
> Instead of this comment, explain what the command 00h does.
>
> Also better to give a memorable name instead of CMD0. For example:
> CMD_TX, CMD_RX…
>
> If possible, make it match the structure names.
>
Okay, I'll make the modifications in the next patch.
> > > +#define NCT6694_CAN_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000)
> > > +#define NCT6694_CAN_CTRL1_MON BIT(0)
> > > +#define NCT6694_CAN_CTRL1_NISO BIT(1)
> > > +#define NCT6694_CAN_CTRL1_LBCK BIT(2)
> > > +
> > > +/* Command 01h */
> > > +#define NCT6694_CAN_CMD1_OFFSET 0x0001
> > > +
> > > +/* Command 02h */
> > > +#define NCT6694_CAN_CMD2_OFFSET(idx, mask) \
> > > + ({ typeof(mask) mask_ = (mask); \
> > > + idx ? ((0x80 | (mask_ & 0xFF)) << 8 | 0x02) : \
> > > + ((0x00 | (mask_ & 0xFF)) << 8 | 0x02); })
> > > +
> > > +#define NCT6694_CAN_EVENT_ERR BIT(0)
> > > +#define NCT6694_CAN_EVENT_STATUS BIT(1)
> > > +#define NCT6694_CAN_EVENT_TX_EVT BIT(2)
> > > +#define NCT6694_CAN_EVENT_RX_EVT BIT(3)
> > > +#define NCT6694_CAN_EVENT_REC BIT(4)
> > > +#define NCT6694_CAN_EVENT_TEC BIT(5)
> > > +#define NCT6694_CAN_EVT_TX_FIFO_EMPTY BIT(7) /* Read-clear */
> > > +#define NCT6694_CAN_EVT_RX_DATA_LOST BIT(5) /* Read-clear */
> > > +#define NCT6694_CAN_EVT_RX_HALF_FULL BIT(6) /* Read-clear */
> > > +#define NCT6694_CAN_EVT_RX_DATA_IN BIT(7)
> > > +
> > > +/* Command 10h */
> > > +#define NCT6694_CAN_CMD10_OFFSET(buf_cnt) \
> > > + (((buf_cnt) & 0xFF) << 8 | 0x10)
> > > +#define NCT6694_CAN_TAG_CAN0 0xC0
> > > +#define NCT6694_CAN_TAG_CAN1 0xC1
> > > +#define NCT6694_CAN_FLAG_EFF BIT(0)
> > > +#define NCT6694_CAN_FLAG_RTR BIT(1)
> > > +#define NCT6694_CAN_FLAG_FD BIT(2)
> > > +#define NCT6694_CAN_FLAG_BRS BIT(3)
> > > +#define NCT6694_CAN_FLAG_ERR BIT(4)
> > > +
> > > +/* Command 11h */
> > > +#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
> > > + ({ typeof(buf_cnt) buf_cnt_ = (buf_cnt); \
> > > + idx ? ((0x80 | (buf_cnt_ & 0xFF)) << 8 | 0x11) : \
> > > + ((0x00 | (buf_cnt_ & 0xFF)) << 8 | 0x11); })
>
> Simplify this. Do something like:
>
> #define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
> (idx ? 0x80 : 0x00) | \
> (buf_cnt & 0xFF)) << 8 | 0x11) \
>
> (apply this also to NCT6694_CAN_CMD2_OFFSET())
>
Understood! I will fix these in v4.
> > > +#define NCT6694_CAN_RX_QUOTA 64
> > > +
> > > +enum nct6694_event_err {
> > > + NCT6694_CAN_EVT_NO_ERROR,
> > ^^^ add _ERR_
> > > + NCT6694_CAN_EVT_CRC_ERROR,
> > > + NCT6694_CAN_EVT_STUFF_ERROR,
> > > + NCT6694_CAN_EVT_ACK_ERROR,
> > > + NCT6694_CAN_EVT_FORM_ERROR,
> > > + NCT6694_CAN_EVT_BIT_ERROR,
> > > + NCT6694_CAN_EVT_TIMEOUT_ERROR,
> > > + NCT6694_CAN_EVT_UNKNOWN_ERROR,
> > > +};
> > > +
> > > +enum nct6694_event_status {
> > > + NCT6694_CAN_EVT_ERROR_ACTIVE,
> > ^^^ add _STATUS_
> > > + NCT6694_CAN_EVT_ERROR_PASSIVE,
> > > + NCT6694_CAN_EVT_BUS_OFF,
> > > + NCT6694_CAN_EVT_WARNING,
> > > +};
> > > +
> > > +struct __packed nct6694_can_setting {
> > > + __le32 nbr;
> > > + __le32 dbr;
> > > + u8 active;
> > > + u8 reserved[3];
> > > + __le16 ctrl1;
> > > + __le16 ctrl2;
> > > + __le32 nbtp;
> > > + __le32 dbtp;
> > > +};
> > > +
> > > +struct __packed nct6694_can_information {
> > > + u8 tx_fifo_cnt;
> > > + u8 rx_fifo_cnt;
> > > + __le16 reserved;
> > u8 reserved[2];
> > > + __le32 can_clk;
> > > +};
> > > +
> > > +struct __packed nct6694_can_event {
> > > + u8 err1;
> > > + u8 status1;
> > > + u8 tx_evt1;
> > > + u8 rx_evt1;
> > > + u8 rec1;
> > > + u8 tec1;
> > > + u8 reserved1[2];
> > > + u8 err2;
> > > + u8 status2;
> > > + u8 tx_evt2;
> > > + u8 rx_evt2;
> > > + u8 rec2;
> > > + u8 tec2;
> > > + u8 reserved2[2];
> > > +};
> >
> > Create an extra struct that only describes a channel
> >
> > struct __packed nct6694_can_event_channel {
> > u8 err;
> > u8 status;
> > u8 tx_evt;
> > u8 rx_evt;
> > u8 rec;
> > u8 tec;
> > u8 reserved[2];
> > }
> >
> > and put an array of 2 into struct __packed nct6694_can_event.
> >
> > > +
> > > +struct __packed nct6694_can_xmit {
>
> Is this struct for both TX and RX? If so, name it something like
>
> struct nct6694_can_frame
>
> The term xmit is only used for transmission, not for reception.
>
Okay, I'll make the modifications in the next patch.
> > > + u8 tag;
> > > + u8 flag;
> > > + u8 reserved;
> > > + u8 dlc;
> > > + __le32 id;
> > > + u8 data[64];
> > > + u8 msg_buf[72];
> >
> > Why is the message so long? What's in the msg_buf?
> >
> > > +};
> > > +
> > > +struct nct6694_can_priv {
> > > + struct can_priv can; /* must be the first member */
> > > + struct net_device *ndev;
> > > + struct nct6694 *nct6694;
> > > + struct mutex lock;
> >
> > What does lock protect?
>
> +1
>
> mutexes are good if you want to keep the lock for a long time.
>
> For short period, spinlock are more performant:
>
> spinlock_t lock;
>
The lock ensures that data accessed by nct6694_read_msg() and
nct6694_write_msg() is not overwritten.
Since nct6694_read_msg() and nct6694_write_msg() use usb_bulk_msg(),
which may cause the process to sleep, spinlock is not used.
> > > + struct sk_buff *tx_skb;
> > > + struct workqueue_struct *wq;
> > > + struct work_struct tx_work;
> > > + unsigned char *tx_buf;
> > void *
> > > + unsigned char *rx_buf;
> > void *
>
> Rather than void*, tx_buf and rx_buf can be replaced by an union:
>
> union nct6694_can_rx {
> struct nct6694_can_event event;
> struct nct6694_can_xmit xmit;
> struct nct6694_can_information info;
> };
>
> (same for nct6694_can_tx)
>
> Then in struct nct6694_can_priv, you will just have:
>
> union nct6694_can_tx tx;
> union nct6694_can_rx rx;
>
> With this,
>
> NCT6694_MAX_PACKET_SZ
>
> can most likely be replaced by sizeof(union nct6694_can_rx) or
> sizeof(union nct6694_can_tx).
>
Okay, I'll make the modifications in the next patch.
> > > + unsigned char can_idx;
> > > + bool tx_busy;
> >
...
> > > +static void nct6694_can_read_fifo(struct net_device *ndev)
> > > +{
> > > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
> > > + struct net_device_stats *stats = &ndev->stats;
> > > + struct canfd_frame *cf;
> > > + struct sk_buff *skb;
> > > + int can_idx = priv->can_idx;
> > > + u32 id;
> > > + int ret;
> > > + u8 fd_format = 0;
> > bool - no need to init
> > > +
> > > + guard(mutex)(&priv->lock);
> > > +
> > > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > > + NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
> > > + sizeof(struct nct6694_can_xmit), xmit);
> > > + if (ret < 0)
> > > + return;
> > > +
> > > + /* Check type of frame and create skb */
> > > + fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
> > > + if (fd_format)
> > > + skb = alloc_canfd_skb(ndev, &cf);
> > > + else
> > > + skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
> > > +
> > > + if (!skb) {
> > > + stats->rx_dropped++;
> > > + return;
> > > + }
> > > +
> > > + cf->len = xmit->dlc;
> >
> > what does xmit->dlc contain? The DLC or the length?
>
> +1
>
> Also, do not trust the device data. Even if SPI attacks are less
> common, make sure to sanitize this length.
>
> cf->len = canfd_sanitize_len(xmit->dlc);
>
> Or
>
> cf->len = canfd_sanitize_len(xmit->dlc);
>
> if xmit->dlc is in fact a DLC.
>
Excuse me, the xmit->dlc is actual data length.
Does it need to be fixed?
> > > +
...
> > > +static int nct6694_can_handle_lec_err(struct net_device *ndev, u8 bus_err)
> > > +{
> > > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > + struct net_device_stats *stats = &ndev->stats;
> > > + struct can_frame *cf;
> > > + struct sk_buff *skb;
> > > +
> > > + if (bus_err == NCT6694_CAN_EVT_NO_ERROR)
> > > + return 0;
> > > +
> > > + priv->can.can_stats.bus_error++;
> > > + stats->rx_errors++;
> > > +
> > > + /* Propagate the error condition to the CAN stack. */
> > > + skb = alloc_can_err_skb(ndev, &cf);
> > > +
> > > + if (unlikely(!skb))
> > > + return 0;
>
> Do not exit if the memory allocation fails. Instead, do the error
> handling to increase the statistics.
>
> Look at what other CAN drivers are doing.
>
Okay, I'll make the modifications in the next patch.
> > > + /* Read the error counter register and check for new errors. */
> > > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > > +
> > > + switch (bus_err) {
> > > + case NCT6694_CAN_EVT_CRC_ERROR:
> > > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_STUFF_ERROR:
> > > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_ACK_ERROR:
> > > + cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_FORM_ERROR:
> > > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_BIT_ERROR:
> > > + cf->data[2] |= CAN_ERR_PROT_BIT |
> > > + CAN_ERR_PROT_BIT0 |
> > > + CAN_ERR_PROT_BIT1;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_TIMEOUT_ERROR:
> > > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > > + break;
> > > +
> > > + case NCT6694_CAN_EVT_UNKNOWN_ERROR:
> > > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > > + /*
> > > + * It means 'unspecified'(the value is '0').
> > > + * But it is not sure if it's ok to send an error package
> > > + * without specific error bit.
> > > + */
> > > + break;
> > > +
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + /* Reset the error counter, ack the IRQ and re-enable the counter. */
> > > + stats->rx_packets++;
> > > + stats->rx_bytes += cf->can_dlc;
>
> The CAN error frames are not regular packets. Do not increase the RX
> stats. Instead, increase the error stats.
>
Okay, I'll make the modifications in the next patch.
> > > + netif_receive_skb(skb);
> > > +
> > > + return 1;
> > > +}
> > > +
> > > +static int nct6694_can_handle_state_change(struct net_device *ndev,
> > > + enum can_state new_state)
> > > +{
> > > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > + struct net_device_stats *stats = &ndev->stats;
> > > + struct can_frame *cf;
> > > + struct sk_buff *skb;
> > > + struct can_berr_counter bec;
> > > +
> > > + switch (new_state) {
> > > + case CAN_STATE_ERROR_ACTIVE:
> > > + priv->can.can_stats.error_warning++;
> > > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > > + break;
> > > + case CAN_STATE_ERROR_WARNING:
> > > + priv->can.can_stats.error_warning++;
> > > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > > + break;
> > > + case CAN_STATE_ERROR_PASSIVE:
> > > + priv->can.can_stats.error_passive++;
> > > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > > + break;
> > > + case CAN_STATE_BUS_OFF:
> > > + priv->can.state = CAN_STATE_BUS_OFF;
> > > + priv->can.can_stats.bus_off++;
> > > + can_bus_off(ndev);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + /* propagate the error condition to the CAN stack */
> > > + skb = alloc_can_err_skb(ndev, &cf);
> > > + if (unlikely(!skb))
> > > + return 0;
>
> Same as above: handle the statistics even if the allocation fails.
>
Okay, I'll make the modifications in the next patch.
> > > + nct6694_can_get_berr_counter(ndev, &bec);
> > > +
> > > + switch (new_state) {
> > > + case CAN_STATE_ERROR_WARNING:
> > > + /* error warning state */
> > > + cf->can_id |= CAN_ERR_CRTL;
> > > + cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
> > > + CAN_ERR_CRTL_RX_WARNING;
>
> Prefer an if/else here instead of the ternary operator. It is more readable.
>
Fix it in v4.
> > > + cf->data[6] = bec.txerr;
> > > + cf->data[7] = bec.rxerr;
> > > + break;
> > > + case CAN_STATE_ERROR_PASSIVE:
> > > + /* error passive state */
> > > + cf->can_id |= CAN_ERR_CRTL;
> > > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > > + if (bec.txerr > 127)
> > > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > > + cf->data[6] = bec.txerr;
> > > + cf->data[7] = bec.rxerr;
> > > + break;
> > > + case CAN_STATE_BUS_OFF:
> > > + /* bus-off state */
> > > + cf->can_id |= CAN_ERR_BUSOFF;
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + stats->rx_packets++;
> > > + stats->rx_bytes += cf->can_dlc;
>
> Do not increase the RX stats when you receive a CAN event.
>
Okay, I'll make the modifications in the next patch.
> > > + netif_receive_skb(skb);
> > > +
> > > + return 1;
> > > +}
...
> > > +static int nct6694_can_start(struct net_device *ndev)
> > > +{
> > > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > + struct nct6694_can_setting *setting = (struct nct6694_can_setting *)priv->tx_buf;
> > > + const struct can_bittiming *n_bt = &priv->can.bittiming;
> > > + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > > + int ret;
> > > +
> > > + guard(mutex)(&priv->lock);
> > > +
> > > + memset(priv->tx_buf, 0, sizeof(struct nct6694_can_setting));
>
> When you memset(), use this pattern:
>
> memset(setting, 0, sizeof(*setting));
>
> Apply this throughout your driver.
>
Okay, I'll make the modifications in the next patch.
> > > + setting->nbr = cpu_to_le32(n_bt->bitrate);
> > > + setting->dbr = cpu_to_le32(d_bt->bitrate);
> > > +
...
> > > +module_platform_driver(nct6694_can_driver);
> > > +
> > > +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> > > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:nct6694-can");
>
>
> Yours sincerely,
> Vincent Mailhol
Best regards,
Ming
On 16/12/2024 at 15:58, Ming Yu wrote: > Dear Vincent, > > Thank you for your comments, > > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月11日 週三 下午11:25寫道： (...) >>>> + cf->len = xmit->dlc; >>> >>> what does xmit->dlc contain? The DLC or the length? >> >> +1 >> >> Also, do not trust the device data. Even if SPI attacks are less >> common, make sure to sanitize this length. >> >> cf->len = canfd_sanitize_len(xmit->dlc); >> >> Or >> >> cf->len = canfd_sanitize_len(xmit->dlc); >> >> if xmit->dlc is in fact a DLC. >> > > Excuse me, the xmit->dlc is actual data length. > Does it need to be fixed? Yes, name is xmit->len. DLC has a different meaning. Yours sincerely, Vincent Mailhol
© 2016 - 2025 Red Hat, Inc.