| Message ID | 20241210104524.2466586-5-tmyu0@nuvoton.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD drivers | expand |
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 */ > +#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
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 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
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
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
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");
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