| Message ID | 20250114033010.2445925-5-a0282524688@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD drivers | expand |
Dear Vincent, Thank you for your reply, I'll add comments to describe these locks in the next patch, Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月14日 週二 下午4:06寫道： > ... > > +config CAN_NCT6694 > > + tristate "Nuvoton NCT6694 Socket CANfd support" > > + depends on MFD_NCT6694 > > Your driver uses the CAN rx offload. You need to select it here. > > select CAN_RX_OFFLOAD > Understood! I'll add it in v6. > > + 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. > > Here, the name is nct6694_canfd... > > > config CAN_PEAK_USB > > tristate "PEAK PCAN-USB/USB Pro interfaces for CAN 2.0b/CAN-FD" > > help > > diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile > > index 8b11088e9a59..fcafb1ac262e 100644 > > --- a/drivers/net/can/usb/Makefile > > +++ b/drivers/net/can/usb/Makefile > > @@ -11,5 +11,6 @@ obj-$(CONFIG_CAN_F81604) += f81604.o > > obj-$(CONFIG_CAN_GS_USB) += gs_usb.o > > obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/ > > obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o > > +obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o > > obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/ > > obj-$(CONFIG_CAN_UCAN) += ucan.o > > diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c > > new file mode 100644 > > index 000000000000..7a15c39021ff > > --- /dev/null > > +++ b/drivers/net/can/usb/nct6694_canfd.c > > @@ -0,0 +1,856 @@ > > +// 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/can/rx-offload.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" > > ... but here, it is nct6694-can. > > Use a consistent name between the module name and the driver name. > Okay, Fix it in v6. > > +/* > > + * USB command module type for NCT6694 CANfd controller. > > + * This defines the module type used for communication with the NCT6694 > > + * CANfd controller over the USB interface. > > + */ > > +#define NCT6694_CAN_MOD 0x05 > > + > > +/* Command 00h - CAN Setting and Initialization */ > > +#define NCT6694_CAN_SETTING 0x00 > > +#define NCT6694_CAN_SETTING_SEL(idx) (idx ? 0x01 : 0x00) > > What are the possible values for idx? Isn't it only 0 or 1? If so, no > need for this NCT6694_CAN_SETTING_SEL() macro. Directly assign the > channel index to the selector field. > Okay, Fix it in v6. > > +#define NCT6694_CAN_SETTING_CTRL1_MON BIT(0) > > +#define NCT6694_CAN_SETTING_CTRL1_NISO BIT(1) > > +#define NCT6694_CAN_SETTING_CTRL1_LBCK BIT(2) > > + > > +/* Command 01h - CAN Information */ > > +#define NCT6694_CAN_INFORMATION 0x01 > > +#define NCT6694_CAN_INFORMATION_SEL 0x00 > > + > > +/* Command 02h - CAN Event */ > > +#define NCT6694_CAN_EVENT 0x02 > > +#define NCT6694_CAN_EVENT_SEL(idx, mask) \ > > + ((idx ? 0x80 : 0x00) | ((mask) & 0xFF)) > > Can idx and mask really overlap? Shouldn't this be: > > #define NCT6694_CAN_EVENT_SEL(idx, mask) \ > ((idx ? 0x80 : 0x00) | ((mask) & 0x7F)) > Sorry, you're right, I'll fix it in v6. > > +#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_EVENT_MASK GENMASK(3, 0) > > +#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) /* Read-clear*/ > > Some of those macro are not used: > > drivers/net/can/usb/nct6694_canfd.c:52: warning: macro > "NCT6694_CAN_EVT_RX_HALF_FULL" is not used [-Wunused-macros] > 52 | #define NCT6694_CAN_EVT_RX_HALF_FULL BIT(6) /* Read-clear */ > | > drivers/net/can/usb/nct6694_canfd.c:43: warning: macro > "NCT6694_CAN_EVENT_ERR" is not used [-Wunused-macros] > 43 | #define NCT6694_CAN_EVENT_ERR BIT(0) > | > drivers/net/can/usb/nct6694_canfd.c:44: warning: macro > "NCT6694_CAN_EVENT_STATUS" is not used [-Wunused-macros] > 44 | #define NCT6694_CAN_EVENT_STATUS BIT(1) > | > drivers/net/can/usb/nct6694_canfd.c:46: warning: macro > "NCT6694_CAN_EVENT_RX_EVT" is not used [-Wunused-macros] > 46 | #define NCT6694_CAN_EVENT_RX_EVT BIT(3) > | > drivers/net/can/usb/nct6694_canfd.c:45: warning: macro > "NCT6694_CAN_EVENT_TX_EVT" is not used [-Wunused-macros] > 45 | #define NCT6694_CAN_EVENT_TX_EVT BIT(2) > | > > Is this OK? > Yes, these macros are replaced by NCT6694_CAN_EVENT_MASK, I'll drop them in the next patch. > > +/* Command 10h - CAN Deliver */ > > +#define NCT6694_CAN_DELIVER 0x10 > > +#define NCT6694_CAN_DELIVER_SEL(buf_cnt) \ > > + ((buf_cnt) & 0xFF) > > + > > +/* Command 11h - CAN Receive */ > > +#define NCT6694_CAN_RECEIVE 0x11 > > +#define NCT6694_CAN_RECEIVE_SEL(idx, buf_cnt) \ > > + ((idx ? 0x80 : 0x00) | ((buf_cnt) & 0xFF)) > > Can idx and buf_cnt really overlap? Shouldn't this be: > > #define NCT6694_CAN_RECEIVE_SEL(idx, buf_cnt) \ > ((idx ? 0x80 : 0x00) | ((buf_cnt) & 0x7F)) > Fix it in v6. > > +#define NCT6694_CAN_FRAME_TAG_CAN0 0xC0 > > +#define NCT6694_CAN_FRAME_TAG_CAN1 0xC1 > > +#define NCT6694_CAN_FRAME_FLAG_EFF BIT(0) > > +#define NCT6694_CAN_FRAME_FLAG_RTR BIT(1) > > +#define NCT6694_CAN_FRAME_FLAG_FD BIT(2) > > +#define NCT6694_CAN_FRAME_FLAG_BRS BIT(3) > > +#define NCT6694_CAN_FRAME_FLAG_ERR BIT(4) > > + > > +#define NCT6694_NAPI_WEIGHT 32 > > + > > +enum nct6694_event_err { > > + NCT6694_CAN_EVT_ERR_NO_ERROR = 0, > > + NCT6694_CAN_EVT_ERR_CRC_ERROR, > > + NCT6694_CAN_EVT_ERR_STUFF_ERROR, > > + NCT6694_CAN_EVT_ERR_ACK_ERROR, > > + NCT6694_CAN_EVT_ERR_FORM_ERROR, > > + NCT6694_CAN_EVT_ERR_BIT_ERROR, > > + NCT6694_CAN_EVT_ERR_TIMEOUT_ERROR, > > + NCT6694_CAN_EVT_ERR_UNKNOWN_ERROR, > > +}; > > + > > +enum nct6694_event_status { > > + NCT6694_CAN_EVT_STS_ERROR_ACTIVE = 0, > > + NCT6694_CAN_EVT_STS_ERROR_PASSIVE, > > + NCT6694_CAN_EVT_STS_BUS_OFF, > > + NCT6694_CAN_EVT_STS_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; > > + u8 reserved[2]; > > + __le32 can_clk; > > +}; > > + > > +struct __packed nct6694_can_event { > > + u8 err; > > + u8 status; > > + u8 tx_evt; > > + u8 rx_evt; > > + u8 rec; > > + u8 tec; > > + u8 reserved[2]; > > +}; > > + > > +struct __packed nct6694_can_frame { > > + u8 tag; > > + u8 flag; > > + u8 reserved; > > + u8 length; > > + __le32 id; > > + u8 data[64]; > > Nitpick, use CANFD_MAX_DLEN here: > > u8 data[CANFD_MAX_DLEN]; > Fix it in v6. > > +}; > > + ... > > +static void nct6694_can_rx(struct net_device *ndev, u8 rx_evt) > > +{ > > + struct nct6694_can_priv *priv = netdev_priv(ndev); > > + struct nct6694_can_frame *frame = &priv->rx->frame; > > + struct nct6694_cmd_header cmd_hd = { > > + .mod = NCT6694_CAN_MOD, > > + .cmd = NCT6694_CAN_RECEIVE, > > + .sel = NCT6694_CAN_RECEIVE_SEL(priv->can_idx, 1), > > + .len = cpu_to_le16(sizeof(*frame)) > > + }; > > + struct canfd_frame *cfd; > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + int ret; > > + > > + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, frame); > > + if (ret) > > + return; > > + > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_FD) { > > Reduce scope of variable when possible: move declaration of cfd here: > > struct canfd_frame *cfd; > Okay! Fix it in v6. > > + skb = alloc_canfd_skb(priv->ndev, &cfd); > > + if (!skb) > > + return; > > + > > + cfd->can_id = le32_to_cpu(frame->id); > > + cfd->len = frame->length; > > No. I asked you to sanitize the length in this message: > > https://lore.kernel.org/linux-can/8d66cf66-5564-4272-8c3e-51b715c3d785@wanadoo.fr/ > > Never use the length as is. > Sorry! I misunderstood your meaning. I'll Fix it to cfd->len = canfd_sanitize_len(frame->length). > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF) > > + cfd->can_id |= CAN_EFF_FLAG; > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_BRS) > > + cfd->flags |= CANFD_BRS; > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_ERR) > > + cfd->flags |= CANFD_ESI; > > + > > + memcpy(cfd->data, frame->data, cfd->len); > > + } else { > > Reduce scope of variable when possible: move declaration of cf here: > > struct canfd_frame *cf; > Fix it in v6. > > + skb = alloc_can_skb(priv->ndev, &cf); > > + if (!skb) > > + return; > > + > > + cf->can_id = le32_to_cpu(frame->id); > > + cf->len = frame->length; > > Ditto, sanitize the length. > Fix it in v6. > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF) > > + cf->can_id |= CAN_EFF_FLAG; > > + if (frame->flag & NCT6694_CAN_FRAME_FLAG_RTR) > > + cf->can_id |= CAN_RTR_FLAG; > > + > > + memcpy(cf->data, frame->data, cf->len); > > Only copy can data if the frame is not an RTR frame. > > if (frame->flag & NCT6694_CAN_FRAME_FLAG_RTR) > cf->can_id |= CAN_RTR_FLAG; > else > memcpy(cf->data, frame->data, cf->len); > > I already asked you to do this in below comment: > > https://lore.kernel.org/linux-can/a25ea362-142f-4e27-8194-787d9829f607@wanadoo.fr/ > Sorry for forgetting the part, I'll fix it in the next patch. > > + } > > + > > + nct6694_can_rx_offload(&priv->offload, skb); > > +} > > + > > +static void nct6694_can_clean(struct net_device *ndev) > > +{ > > + struct nct6694_can_priv *priv = netdev_priv(ndev); > > + > > + if (priv->tx_skb || netif_queue_stopped(ndev)) > > + ndev->stats.tx_errors++; > > + dev_kfree_skb(priv->tx_skb); > > + priv->tx_skb = NULL; > > +} > > + > > +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 = priv->rx->event; > > + struct nct6694_cmd_header cmd_hd; > > + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; > > + int ret; > > + > > + guard(mutex)(&priv->lock); > > + > > + cmd_hd = (struct nct6694_cmd_header) { > > + .mod = NCT6694_CAN_MOD, > > + .cmd = NCT6694_CAN_EVENT, > > + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), > > + .len = cpu_to_le16(sizeof(priv->rx->event)) > > + }; > > + > > + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); > > + if (ret < 0) > > + return ret; > > You are holding the priv->lock mutex before calling > nct6694_read_msg(). But nct6694_read_msg() then holds the > nct6694->access_lock mutex. Why do you need a double mutex here? What > kind of race scenario are you trying to prevent here? > I think priv->lock need to be placed here to prevent priv->rx from being assigned by other functions, and nct6694->access_lock ensures that the nct6694_read_msg() transaction is completed. But in this case, cmd_hd does not need to be in priv->lock's scope. > > + bec->rxerr = evt[priv->can_idx].rec; > > + bec->txerr = evt[priv->can_idx].tec; > > + > > + return 0; > > +} > > + > > +static void nct6694_can_handle_state_change(struct net_device *ndev, > > + u8 status) > > +{ > > + struct nct6694_can_priv *priv = netdev_priv(ndev); > > + enum can_state new_state = priv->can.state; > > + enum can_state rx_state, tx_state; > > + struct can_berr_counter bec; > > + struct can_frame *cf; > > + struct sk_buff *skb; > > + > > + nct6694_can_get_berr_counter(ndev, &bec); > > + can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state); > > Here, you set up tx_state and rx_state... > > > + switch (status) { > > + case NCT6694_CAN_EVT_STS_ERROR_ACTIVE: > > + new_state = CAN_STATE_ERROR_ACTIVE; > > + break; > > + case NCT6694_CAN_EVT_STS_ERROR_PASSIVE: > > + new_state = CAN_STATE_ERROR_PASSIVE; > > + break; > > + case NCT6694_CAN_EVT_STS_BUS_OFF: > > + new_state = CAN_STATE_BUS_OFF; > > + break; > > + case NCT6694_CAN_EVT_STS_WARNING: > > + new_state = CAN_STATE_ERROR_WARNING; > > + break; > > + default: > > + netdev_err(ndev, "Receive unknown CAN status event.\n"); > > + return; > > + } > > + > > + /* state hasn't changed */ > > + if (new_state == priv->can.state) > > + return; > > + > > + skb = alloc_can_err_skb(ndev, &cf); > > + > > + tx_state = bec.txerr >= bec.rxerr ? new_state : 0; > > + rx_state = bec.txerr <= bec.rxerr ? new_state : 0; > > ... but you never used the values returned by > can_state_get_by_berr_counter() and just overwrote the tx and rx > state. > > What is the logic here? Why do you need to manually adjust those two > values? Isn't the logic in can_change_state() sufficient? > > > + can_change_state(ndev, cf, tx_state, rx_state); > > + > > + if (new_state == CAN_STATE_BUS_OFF) { > > Same for the new_state. The function can_change_state() calculate the > new state from tx_state and rx_state and save it under > can_priv->state. But here, you do your own calculation. > > Only keep one of the two. If your device already tells you the state, > then fine! Just use the information from your device and do not use > can_change_state(). Here, you are doing double work resulting in a > weird mix. > Okay! I will revert nct6694_can_handle_state_change() back to the v3 version. > > + can_bus_off(ndev); > > + } else if (skb) { > > + cf->can_id |= CAN_ERR_CNT; > > + cf->data[6] = bec.txerr; > > + cf->data[7] = bec.rxerr; > > + } > > + > > + nct6694_can_rx_offload(&priv->offload, skb); > > +} > > + ... > > +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 = priv->rx->event; > > + struct nct6694_cmd_header cmd_hd; > > + u8 tx_evt, rx_evt, bus_err, can_status; > > + u8 mask_sts = NCT6694_CAN_EVENT_MASK; > > No need for the mask_sts variable. Directly use NCT6694_CAN_EVENT_MASK. > Okay! Fix it in v6. > > + irqreturn_t handled = IRQ_NONE; > > + int can_idx = priv->can_idx; > > + int ret; > > + > > + scoped_guard(mutex, &priv->lock) { > > Reduce scope of variable when possible: move the declarations of > cmd_hd and ret here. > Okay! Fix it in v6. > > + cmd_hd = (struct nct6694_cmd_header) { > > + .mod = NCT6694_CAN_MOD, > > + .cmd = NCT6694_CAN_EVENT, > > + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask_sts), > > + .len = cpu_to_le16(sizeof(priv->rx->event)) > > + }; > > + ... > > +static void nct6694_can_tx(struct net_device *ndev) > > +{ > > + struct nct6694_can_priv *priv = netdev_priv(ndev); > > + struct nct6694_can_frame *frame = &priv->tx->frame; > > + struct nct6694_cmd_header cmd_hd = { > > + .mod = NCT6694_CAN_MOD, > > + .cmd = NCT6694_CAN_DELIVER, > > + .sel = NCT6694_CAN_DELIVER_SEL(1), > > + .len = cpu_to_le16(sizeof(*frame)) > > + }; > > + struct net_device_stats *stats = &ndev->stats; > > + struct sk_buff *skb = priv->tx_skb; > > + struct canfd_frame *cfd; > > + struct can_frame *cf; > > + u32 txid; > > + int err; > > + > > + memset(frame, 0, sizeof(*frame)); > > + > > + if (priv->can_idx == 0) > > + frame->tag = NCT6694_CAN_FRAME_TAG_CAN0; > > + else > > + frame->tag = NCT6694_CAN_FRAME_TAG_CAN1; > > + > > + if (can_is_canfd_skb(skb)) { > > Reduce scope of variable when possible: move declaration of cfd here: > > struct canfd_frame *cfd; > Okay! Fix it in v6. > > + cfd = (struct canfd_frame *)priv->tx_skb->data; > > + > > + if (cfd->flags & CANFD_BRS) > > + frame->flag |= NCT6694_CAN_FRAME_FLAG_BRS; > > + > > + if (cfd->can_id & CAN_EFF_FLAG) { > > + txid = cfd->can_id & CAN_EFF_MASK; > > + frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF; > > + } else { > > + txid = cfd->can_id & CAN_SFF_MASK; > > + } > > + frame->flag |= NCT6694_CAN_FRAME_FLAG_FD; > > + frame->id = cpu_to_le32(txid); > > + frame->length = cfd->len; > > + > > + memcpy(frame->data, cfd->data, cfd->len); > > + } else { > > Reduce scope of variable when possible: move declaration of cf here: > > struct canfd_frame *cf; > Okay! Fix it in v6. > > + cf = (struct can_frame *)priv->tx_skb->data; > > + > > + if (cf->can_id & CAN_RTR_FLAG) > > + frame->flag |= NCT6694_CAN_FRAME_FLAG_RTR; > > + > > + if (cf->can_id & CAN_EFF_FLAG) { > > + txid = cf->can_id & CAN_EFF_MASK; > > + frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF; > > + } else { > > + txid = cf->can_id & CAN_SFF_MASK; > > + } > > + frame->id = cpu_to_le32(txid); > > + frame->length = cf->len; > > + > > + memcpy(frame->data, cf->data, cf->len); > > Don't copy cf->data if the can frame is a RTR frame. > Okay! Fix it in v6. > > + } > > + > > + err = nct6694_write_msg(priv->nct6694, &cmd_hd, frame); > > + if (err) { > > + netdev_err(ndev, "%s: Tx FIFO full!\n", __func__); > > + can_free_echo_skb(ndev, 0, NULL); > > + stats->tx_dropped++; > > + stats->tx_errors++; > > + netif_wake_queue(ndev); > > + } > > +} > > + ... > > +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 irq; > > + > > + 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; > > Your device has two CAN interfaces, right? Do not forget to populate > netdev->dev_port. > > netdev->dev_port = cell->id; > Okay! I'll add it in v6. > > + priv = netdev_priv(ndev); > > + priv->nct6694 = nct6694; > > + priv->ndev = ndev; > > + Best regards, Ming
On 14/01/2025 at 19:46, Ming Yu wrote: > Dear Vincent, > > Thank you for your reply, > I'll add comments to describe these locks in the next patch, > > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月14日 週二 下午4:06寫道： (...) >>> +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 = priv->rx->event; >>> + struct nct6694_cmd_header cmd_hd; >>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; >>> + int ret; >>> + >>> + guard(mutex)(&priv->lock); >>> + >>> + cmd_hd = (struct nct6694_cmd_header) { >>> + .mod = NCT6694_CAN_MOD, >>> + .cmd = NCT6694_CAN_EVENT, >>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), >>> + .len = cpu_to_le16(sizeof(priv->rx->event)) >>> + }; >>> + >>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); >>> + if (ret < 0) >>> + return ret; >> >> You are holding the priv->lock mutex before calling >> nct6694_read_msg(). But nct6694_read_msg() then holds the >> nct6694->access_lock mutex. Why do you need a double mutex here? What >> kind of race scenario are you trying to prevent here? >> > > I think priv->lock need to be placed here to prevent priv->rx from > being assigned by other functions, and nct6694->access_lock ensures > that the nct6694_read_msg() transaction is completed. > But in this case, cmd_hd does not need to be in priv->lock's scope. So, the only reason for holding priv->lock is because priv->rx is shared between functions. struct nct6694_can_event is only 8 bytes. And you only need it for the life time of the function so it can simply be declared on the stack: struct nct6694_can_event evt; and with this, no more need to hold the lock. And the same thing also applies to the other functions. Here, by trying to optimize the memory for only a few bytes, you are getting a huge penalty on the performance by putting locks on all the functions. This is not a good tradeoff. >>> + bec->rxerr = evt[priv->can_idx].rec; >>> + bec->txerr = evt[priv->can_idx].tec; >>> + >>> + return 0; >>> +} Yours sincerely, Vincent Mailhol
Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月14日 週二 下午11:12寫道： > ... > >>> +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 = priv->rx->event; > >>> + struct nct6694_cmd_header cmd_hd; > >>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; > >>> + int ret; > >>> + > >>> + guard(mutex)(&priv->lock); > >>> + > >>> + cmd_hd = (struct nct6694_cmd_header) { > >>> + .mod = NCT6694_CAN_MOD, > >>> + .cmd = NCT6694_CAN_EVENT, > >>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), > >>> + .len = cpu_to_le16(sizeof(priv->rx->event)) > >>> + }; > >>> + > >>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); > >>> + if (ret < 0) > >>> + return ret; > >> > >> You are holding the priv->lock mutex before calling > >> nct6694_read_msg(). But nct6694_read_msg() then holds the > >> nct6694->access_lock mutex. Why do you need a double mutex here? What > >> kind of race scenario are you trying to prevent here? > >> > > > > I think priv->lock need to be placed here to prevent priv->rx from > > being assigned by other functions, and nct6694->access_lock ensures > > that the nct6694_read_msg() transaction is completed. > > But in this case, cmd_hd does not need to be in priv->lock's scope. > > So, the only reason for holding priv->lock is because priv->rx is shared > between functions. > > struct nct6694_can_event is only 8 bytes. And you only need it for the > life time of the function so it can simply be declared on the stack: > > struct nct6694_can_event evt; > > and with this, no more need to hold the lock. And the same thing also > applies to the other functions. > > Here, by trying to optimize the memory for only a few bytes, you are > getting a huge penalty on the performance by putting locks on all the > functions. This is not a good tradeoff. > Since nct6694_read_msg()/nct6694_write_msg() process URBs via usb_bulk_msg(), the transferred data must not be located on the stack. For more details about allocating buffers for transmitting data, please refer to the link: https://lore.kernel.org/linux-can/20241028-observant-gentle-doberman-0a2baa-mkl@pengutronix.de/ Thanks, Ming
On 15/01/2025 at 11:11, Ming Yu wrote: > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月14日 週二 下午11:12寫道： >> > ... >>>>> +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 = priv->rx->event; >>>>> + struct nct6694_cmd_header cmd_hd; >>>>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; >>>>> + int ret; >>>>> + >>>>> + guard(mutex)(&priv->lock); >>>>> + >>>>> + cmd_hd = (struct nct6694_cmd_header) { >>>>> + .mod = NCT6694_CAN_MOD, >>>>> + .cmd = NCT6694_CAN_EVENT, >>>>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), >>>>> + .len = cpu_to_le16(sizeof(priv->rx->event)) >>>>> + }; >>>>> + >>>>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); >>>>> + if (ret < 0) >>>>> + return ret; >>>> >>>> You are holding the priv->lock mutex before calling >>>> nct6694_read_msg(). But nct6694_read_msg() then holds the >>>> nct6694->access_lock mutex. Why do you need a double mutex here? What >>>> kind of race scenario are you trying to prevent here? >>>> >>> >>> I think priv->lock need to be placed here to prevent priv->rx from >>> being assigned by other functions, and nct6694->access_lock ensures >>> that the nct6694_read_msg() transaction is completed. >>> But in this case, cmd_hd does not need to be in priv->lock's scope. >> >> So, the only reason for holding priv->lock is because priv->rx is shared >> between functions. >> >> struct nct6694_can_event is only 8 bytes. And you only need it for the >> life time of the function so it can simply be declared on the stack: >> >> struct nct6694_can_event evt; >> >> and with this, no more need to hold the lock. And the same thing also >> applies to the other functions. >> >> Here, by trying to optimize the memory for only a few bytes, you are >> getting a huge penalty on the performance by putting locks on all the >> functions. This is not a good tradeoff. >> > > Since nct6694_read_msg()/nct6694_write_msg() process URBs via > usb_bulk_msg(), the transferred data must not be located on the stack. > For more details about allocating buffers for transmitting data, > please refer to the link: > https://lore.kernel.org/linux-can/20241028-observant-gentle-doberman-0a2baa-mkl@pengutronix.de/ Ack, I forgot that you can not use stack memory in usb_bulk_msg(). Then, instead, you can either: - do a dynamic memory allocation directly in the function (good for when you are outside of the hot path, for example struct nct6694_can_setting) - and for the other structures which are part of the hot path (typically struct nct6694_can_frame) continue to use a dynamically allocated buffer stored in your priv but change the type of nct6694_can_tx and nct6694_can_rx from union to structures. And no more overlaps, thus no more need for the mutex. Yours sincerely, Vincent Mailhol
Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月15日 週三 上午11:36寫道： > >>>>> +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 = priv->rx->event; > >>>>> + struct nct6694_cmd_header cmd_hd; > >>>>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; > >>>>> + int ret; > >>>>> + > >>>>> + guard(mutex)(&priv->lock); > >>>>> + > >>>>> + cmd_hd = (struct nct6694_cmd_header) { > >>>>> + .mod = NCT6694_CAN_MOD, > >>>>> + .cmd = NCT6694_CAN_EVENT, > >>>>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), > >>>>> + .len = cpu_to_le16(sizeof(priv->rx->event)) > >>>>> + }; > >>>>> + > >>>>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); > >>>>> + if (ret < 0) > >>>>> + return ret; > >>>> > >>>> You are holding the priv->lock mutex before calling > >>>> nct6694_read_msg(). But nct6694_read_msg() then holds the > >>>> nct6694->access_lock mutex. Why do you need a double mutex here? What > >>>> kind of race scenario are you trying to prevent here? > >>>> > >>> > >>> I think priv->lock need to be placed here to prevent priv->rx from > >>> being assigned by other functions, and nct6694->access_lock ensures > >>> that the nct6694_read_msg() transaction is completed. > >>> But in this case, cmd_hd does not need to be in priv->lock's scope. > >> > >> So, the only reason for holding priv->lock is because priv->rx is shared > >> between functions. > >> > >> struct nct6694_can_event is only 8 bytes. And you only need it for the > >> life time of the function so it can simply be declared on the stack: > >> > >> struct nct6694_can_event evt; > >> > >> and with this, no more need to hold the lock. And the same thing also > >> applies to the other functions. > >> > >> Here, by trying to optimize the memory for only a few bytes, you are > >> getting a huge penalty on the performance by putting locks on all the > >> functions. This is not a good tradeoff. > >> > > > > Since nct6694_read_msg()/nct6694_write_msg() process URBs via > > usb_bulk_msg(), the transferred data must not be located on the stack. > > For more details about allocating buffers for transmitting data, > > please refer to the link: > > https://lore.kernel.org/linux-can/20241028-observant-gentle-doberman-0a2baa-mkl@pengutronix.de/ > > Ack, I forgot that you can not use stack memory in usb_bulk_msg(). > > Then, instead, you can either: > > - do a dynamic memory allocation directly in the function (good for > when you are outside of the hot path, for example struct > nct6694_can_setting) > > - and for the other structures which are part of the hot path > (typically struct nct6694_can_frame) continue to use a dynamically > allocated buffer stored in your priv but change the type of > nct6694_can_tx and nct6694_can_rx from union to structures. > > And no more overlaps, thus no more need for the mutex. > Understood, I will remove the unions and add members to private structure in the next patch. e.g. struct nct6694_can_priv { struct can_priv can; ... struct nct6694_can_frame tx; struct nct6694_can_frame rx; }; And do dynamic memory allocation for struct nct6694_can_setting and struct nct6694_can_information. In addition, I would like to know your thoughts on how struct nct6694_can_event[2] should be handled? It is utilized in both nct6694_can_get_berr_counter() and nct6694_can_irq(), with the latter being called more frequently during runtime. Thanks, Ming
On Wed. 15 Jan 2025 at 14:35, Ming Yu <a0282524688@gmail.com> wrote: > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月15日 週三 上午11:36寫道： > > >>>>> +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 = priv->rx->event; > > >>>>> + struct nct6694_cmd_header cmd_hd; > > >>>>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; > > >>>>> + int ret; > > >>>>> + > > >>>>> + guard(mutex)(&priv->lock); > > >>>>> + > > >>>>> + cmd_hd = (struct nct6694_cmd_header) { > > >>>>> + .mod = NCT6694_CAN_MOD, > > >>>>> + .cmd = NCT6694_CAN_EVENT, > > >>>>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), > > >>>>> + .len = cpu_to_le16(sizeof(priv->rx->event)) > > >>>>> + }; > > >>>>> + > > >>>>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); > > >>>>> + if (ret < 0) > > >>>>> + return ret; > > >>>> > > >>>> You are holding the priv->lock mutex before calling > > >>>> nct6694_read_msg(). But nct6694_read_msg() then holds the > > >>>> nct6694->access_lock mutex. Why do you need a double mutex here? What > > >>>> kind of race scenario are you trying to prevent here? > > >>>> > > >>> > > >>> I think priv->lock need to be placed here to prevent priv->rx from > > >>> being assigned by other functions, and nct6694->access_lock ensures > > >>> that the nct6694_read_msg() transaction is completed. > > >>> But in this case, cmd_hd does not need to be in priv->lock's scope. > > >> > > >> So, the only reason for holding priv->lock is because priv->rx is shared > > >> between functions. > > >> > > >> struct nct6694_can_event is only 8 bytes. And you only need it for the > > >> life time of the function so it can simply be declared on the stack: > > >> > > >> struct nct6694_can_event evt; > > >> > > >> and with this, no more need to hold the lock. And the same thing also > > >> applies to the other functions. > > >> > > >> Here, by trying to optimize the memory for only a few bytes, you are > > >> getting a huge penalty on the performance by putting locks on all the > > >> functions. This is not a good tradeoff. > > >> > > > > > > Since nct6694_read_msg()/nct6694_write_msg() process URBs via > > > usb_bulk_msg(), the transferred data must not be located on the stack. > > > For more details about allocating buffers for transmitting data, > > > please refer to the link: > > > https://lore.kernel.org/linux-can/20241028-observant-gentle-doberman-0a2baa-mkl@pengutronix.de/ > > > > Ack, I forgot that you can not use stack memory in usb_bulk_msg(). > > > > Then, instead, you can either: > > > > - do a dynamic memory allocation directly in the function (good for > > when you are outside of the hot path, for example struct > > nct6694_can_setting) > > > > - and for the other structures which are part of the hot path > > (typically struct nct6694_can_frame) continue to use a dynamically > > allocated buffer stored in your priv but change the type of > > nct6694_can_tx and nct6694_can_rx from union to structures. > > > > And no more overlaps, thus no more need for the mutex. > > > > Understood, I will remove the unions and add members to private > structure in the next patch. > e.g. > struct nct6694_can_priv { > struct can_priv can; > ... > struct nct6694_can_frame tx; > struct nct6694_can_frame rx; > }; > And do dynamic memory allocation for struct nct6694_can_setting and > struct nct6694_can_information. > > In addition, I would like to know your thoughts on how struct > nct6694_can_event[2] should be handled? > It is utilized in both nct6694_can_get_berr_counter() and > nct6694_can_irq(), with the latter being called more frequently during > runtime. For the nct6694_can_event in nct6694_can_irq(), I would say it is part of the hot path and thus you can have it in your struct nct6694_can_priv. For the nct6694_can_get_berr_counter(), the easiest is actually to just add the error counter structure to your nct6694_can_priv: struct can_berr_counter berr_cnt; Each time you receive an event, you update this local error counter copy, and this way, in your nct6694_can_get_berr_counter(), no more need to query your device, you just return the berr_cnt which is saved locally. Yours sincerely, Vincent Mailhol
Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月15日 週三 下午2:44寫道： > > On Wed. 15 Jan 2025 at 14:35, Ming Yu <a0282524688@gmail.com> wrote: > > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月15日 週三 上午11:36寫道： > > > >>>>> +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 = priv->rx->event; > > > >>>>> + struct nct6694_cmd_header cmd_hd; > > > >>>>> + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; > > > >>>>> + int ret; > > > >>>>> + > > > >>>>> + guard(mutex)(&priv->lock); > > > >>>>> + > > > >>>>> + cmd_hd = (struct nct6694_cmd_header) { > > > >>>>> + .mod = NCT6694_CAN_MOD, > > > >>>>> + .cmd = NCT6694_CAN_EVENT, > > > >>>>> + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), > > > >>>>> + .len = cpu_to_le16(sizeof(priv->rx->event)) > > > >>>>> + }; > > > >>>>> + > > > >>>>> + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); > > > >>>>> + if (ret < 0) > > > >>>>> + return ret; > > > >>>> > > > >>>> You are holding the priv->lock mutex before calling > > > >>>> nct6694_read_msg(). But nct6694_read_msg() then holds the > > > >>>> nct6694->access_lock mutex. Why do you need a double mutex here? What > > > >>>> kind of race scenario are you trying to prevent here? > > > >>>> > > > >>> > > > >>> I think priv->lock need to be placed here to prevent priv->rx from > > > >>> being assigned by other functions, and nct6694->access_lock ensures > > > >>> that the nct6694_read_msg() transaction is completed. > > > >>> But in this case, cmd_hd does not need to be in priv->lock's scope. > > > >> > > > >> So, the only reason for holding priv->lock is because priv->rx is shared > > > >> between functions. > > > >> > > > >> struct nct6694_can_event is only 8 bytes. And you only need it for the > > > >> life time of the function so it can simply be declared on the stack: > > > >> > > > >> struct nct6694_can_event evt; > > > >> > > > >> and with this, no more need to hold the lock. And the same thing also > > > >> applies to the other functions. > > > >> > > > >> Here, by trying to optimize the memory for only a few bytes, you are > > > >> getting a huge penalty on the performance by putting locks on all the > > > >> functions. This is not a good tradeoff. > > > >> > > > > > > > > Since nct6694_read_msg()/nct6694_write_msg() process URBs via > > > > usb_bulk_msg(), the transferred data must not be located on the stack. > > > > For more details about allocating buffers for transmitting data, > > > > please refer to the link: > > > > https://lore.kernel.org/linux-can/20241028-observant-gentle-doberman-0a2baa-mkl@pengutronix.de/ > > > > > > Ack, I forgot that you can not use stack memory in usb_bulk_msg(). > > > > > > Then, instead, you can either: > > > > > > - do a dynamic memory allocation directly in the function (good for > > > when you are outside of the hot path, for example struct > > > nct6694_can_setting) > > > > > > - and for the other structures which are part of the hot path > > > (typically struct nct6694_can_frame) continue to use a dynamically > > > allocated buffer stored in your priv but change the type of > > > nct6694_can_tx and nct6694_can_rx from union to structures. > > > > > > And no more overlaps, thus no more need for the mutex. > > > > > > > Understood, I will remove the unions and add members to private > > structure in the next patch. > > e.g. > > struct nct6694_can_priv { > > struct can_priv can; > > ... > > struct nct6694_can_frame tx; > > struct nct6694_can_frame rx; > > }; > > And do dynamic memory allocation for struct nct6694_can_setting and > > struct nct6694_can_information. > > > > In addition, I would like to know your thoughts on how struct > > nct6694_can_event[2] should be handled? > > It is utilized in both nct6694_can_get_berr_counter() and > > nct6694_can_irq(), with the latter being called more frequently during > > runtime. > > For the nct6694_can_event in nct6694_can_irq(), I would say it is part > of the hot path and thus you can have it in your struct > nct6694_can_priv. > > For the nct6694_can_get_berr_counter(), the easiest is actually to > just add the error counter structure to your nct6694_can_priv: > > struct can_berr_counter berr_cnt; > > Each time you receive an event, you update this local error counter > copy, and this way, in your nct6694_can_get_berr_counter(), no more > need to query your device, you just return the berr_cnt which is saved > locally. > Understood! I will make these modifications in the next patch. Best regards, Ming
On 14/01/2025 at 12:30, Ming Yu wrote: (...) > +static void nct6694_can_clean(struct net_device *ndev) > +{ > + struct nct6694_can_priv *priv = netdev_priv(ndev); > + > + if (priv->tx_skb || netif_queue_stopped(ndev)) > + ndev->stats.tx_errors++; > + dev_kfree_skb(priv->tx_skb); Use: can_flush_echo_skb(ndev); (related to the following comments). > + priv->tx_skb = NULL; > +} (...) > +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; > + > + guard(mutex)(&priv->lock); > + > + if (priv->tx_skb) { > + if (priv->can.state == CAN_STATE_BUS_OFF) { Just stop the queue when the can bus is off so that you do not have do check the bus status each time a frame is sent. > + nct6694_can_clean(ndev); > + } else { > + nct6694_can_tx(ndev); > + can_put_echo_skb(priv->tx_skb, ndev, 0, 0); > + priv->tx_skb = NULL; > + } > + } > +} > + > +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 (can_dev_dropped_skb(ndev, skb)) > + return NETDEV_TX_OK; > + > + if (priv->tx_skb) { > + netdev_err(ndev, "hard_xmit called while tx busy\n"); > + return NETDEV_TX_BUSY; > + } > + > + netif_stop_queue(ndev); > + priv->tx_skb = skb; Here, you can directly do: can_put_echo_skb(skb, ndev, 0, 0); The skb remains accessible under priv->can.echo_skb[0]. With this, you can remove the priv->tx_skb field. > + queue_work(priv->wq, &priv->tx_work); > + > + return NETDEV_TX_OK; > +} Yours sincerely, Vincent Mailhol
Hi Vincent, I will remove priv->tx_skb in the next patch, but it seems that can_flush_echo_skb() has not been EXPORT_SYMBOL_GPL(). I would like to know if nct6694_can_clean() requires modification. Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2025年1月16日 週四 上午12:45寫道： > > +static void nct6694_can_clean(struct net_device *ndev) > > +{ > > + struct nct6694_can_priv *priv = netdev_priv(ndev); > > + > > + if (priv->tx_skb || netif_queue_stopped(ndev)) > > + ndev->stats.tx_errors++; > > + dev_kfree_skb(priv->tx_skb); > > Use: > > can_flush_echo_skb(ndev); > > (related to the following comments). > > > + priv->tx_skb = NULL; > > +} > > (...) > > > +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; > > + > > + guard(mutex)(&priv->lock); > > + > > + if (priv->tx_skb) { > > + if (priv->can.state == CAN_STATE_BUS_OFF) { > > Just stop the queue when the can bus is off so that you do not have do > check the bus status each time a frame is sent. > > > + nct6694_can_clean(ndev); > > + } else { > > + nct6694_can_tx(ndev); > > + can_put_echo_skb(priv->tx_skb, ndev, 0, 0); > > + priv->tx_skb = NULL; > > + } > > + } > > +} > > + > > +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 (can_dev_dropped_skb(ndev, skb)) > > + return NETDEV_TX_OK; > > + > > + if (priv->tx_skb) { > > + netdev_err(ndev, "hard_xmit called while tx busy\n"); > > + return NETDEV_TX_BUSY; > > + } > > + > > + netif_stop_queue(ndev); > > + priv->tx_skb = skb; > > Here, you can directly do: > > can_put_echo_skb(skb, ndev, 0, 0); > > The skb remains accessible under priv->can.echo_skb[0]. With this, you > can remove the priv->tx_skb field. > > > + queue_work(priv->wq, &priv->tx_work); > > + > > + return NETDEV_TX_OK; > > +} > Thanks, Ming
On 16/01/2025 at 15:34, Ming Yu wrote: > Hi Vincent, > > I will remove priv->tx_skb in the next patch, but it seems that > can_flush_echo_skb() has not been EXPORT_SYMBOL_GPL(). > > I would like to know if nct6694_can_clean() requires modification. Indeed, you can can_free_echo_skb(ndev, 0, NULL) instead of can_flush_echo_skb(ndev). That one is exported. Yours sincerely, Vincent Mailhol
diff --git a/MAINTAINERS b/MAINTAINERS index 4e72f749cdf2..6e9b78202d6f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16724,6 +16724,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/usb/nct6694_canfd.c F: include/linux/mfd/nct6694.h NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig index 9dae0c71a2e1..53254012cdc4 100644 --- a/drivers/net/can/usb/Kconfig +++ b/drivers/net/can/usb/Kconfig @@ -133,6 +133,16 @@ config CAN_MCBA_USB This driver supports the CAN BUS Analyzer interface from Microchip (http://www.microchip.com/development-tools/). +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_PEAK_USB tristate "PEAK PCAN-USB/USB Pro interfaces for CAN 2.0b/CAN-FD" help diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile index 8b11088e9a59..fcafb1ac262e 100644 --- a/drivers/net/can/usb/Makefile +++ b/drivers/net/can/usb/Makefile @@ -11,5 +11,6 @@ obj-$(CONFIG_CAN_F81604) += f81604.o obj-$(CONFIG_CAN_GS_USB) += gs_usb.o obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/ obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o +obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/ obj-$(CONFIG_CAN_UCAN) += ucan.o diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c new file mode 100644 index 000000000000..7a15c39021ff --- /dev/null +++ b/drivers/net/can/usb/nct6694_canfd.c @@ -0,0 +1,856 @@ +// 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/can/rx-offload.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" + +/* + * USB command module type for NCT6694 CANfd controller. + * This defines the module type used for communication with the NCT6694 + * CANfd controller over the USB interface. + */ +#define NCT6694_CAN_MOD 0x05 + +/* Command 00h - CAN Setting and Initialization */ +#define NCT6694_CAN_SETTING 0x00 +#define NCT6694_CAN_SETTING_SEL(idx) (idx ? 0x01 : 0x00) +#define NCT6694_CAN_SETTING_CTRL1_MON BIT(0) +#define NCT6694_CAN_SETTING_CTRL1_NISO BIT(1) +#define NCT6694_CAN_SETTING_CTRL1_LBCK BIT(2) + +/* Command 01h - CAN Information */ +#define NCT6694_CAN_INFORMATION 0x01 +#define NCT6694_CAN_INFORMATION_SEL 0x00 + +/* Command 02h - CAN Event */ +#define NCT6694_CAN_EVENT 0x02 +#define NCT6694_CAN_EVENT_SEL(idx, mask) \ + ((idx ? 0x80 : 0x00) | ((mask) & 0xFF)) +#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_EVENT_MASK GENMASK(3, 0) +#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) /* Read-clear*/ + +/* Command 10h - CAN Deliver */ +#define NCT6694_CAN_DELIVER 0x10 +#define NCT6694_CAN_DELIVER_SEL(buf_cnt) \ + ((buf_cnt) & 0xFF) + +/* Command 11h - CAN Receive */ +#define NCT6694_CAN_RECEIVE 0x11 +#define NCT6694_CAN_RECEIVE_SEL(idx, buf_cnt) \ + ((idx ? 0x80 : 0x00) | ((buf_cnt) & 0xFF)) + +#define NCT6694_CAN_FRAME_TAG_CAN0 0xC0 +#define NCT6694_CAN_FRAME_TAG_CAN1 0xC1 +#define NCT6694_CAN_FRAME_FLAG_EFF BIT(0) +#define NCT6694_CAN_FRAME_FLAG_RTR BIT(1) +#define NCT6694_CAN_FRAME_FLAG_FD BIT(2) +#define NCT6694_CAN_FRAME_FLAG_BRS BIT(3) +#define NCT6694_CAN_FRAME_FLAG_ERR BIT(4) + +#define NCT6694_NAPI_WEIGHT 32 + +enum nct6694_event_err { + NCT6694_CAN_EVT_ERR_NO_ERROR = 0, + NCT6694_CAN_EVT_ERR_CRC_ERROR, + NCT6694_CAN_EVT_ERR_STUFF_ERROR, + NCT6694_CAN_EVT_ERR_ACK_ERROR, + NCT6694_CAN_EVT_ERR_FORM_ERROR, + NCT6694_CAN_EVT_ERR_BIT_ERROR, + NCT6694_CAN_EVT_ERR_TIMEOUT_ERROR, + NCT6694_CAN_EVT_ERR_UNKNOWN_ERROR, +}; + +enum nct6694_event_status { + NCT6694_CAN_EVT_STS_ERROR_ACTIVE = 0, + NCT6694_CAN_EVT_STS_ERROR_PASSIVE, + NCT6694_CAN_EVT_STS_BUS_OFF, + NCT6694_CAN_EVT_STS_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; + u8 reserved[2]; + __le32 can_clk; +}; + +struct __packed nct6694_can_event { + u8 err; + u8 status; + u8 tx_evt; + u8 rx_evt; + u8 rec; + u8 tec; + u8 reserved[2]; +}; + +struct __packed nct6694_can_frame { + u8 tag; + u8 flag; + u8 reserved; + u8 length; + __le32 id; + u8 data[64]; +}; + +union __packed nct6694_can_tx { + struct nct6694_can_frame frame; + struct nct6694_can_setting setting; +}; + +union __packed nct6694_can_rx { + struct nct6694_can_event event[2]; + struct nct6694_can_frame frame; + struct nct6694_can_information info; +}; + +struct nct6694_can_priv { + struct can_priv can; /* must be the first member */ + struct can_rx_offload offload; + struct net_device *ndev; + struct nct6694 *nct6694; + struct mutex lock; + struct sk_buff *tx_skb; + struct workqueue_struct *wq; + struct work_struct tx_work; + union nct6694_can_tx *tx; + union nct6694_can_rx *rx; + unsigned char can_idx; +}; + +static inline struct nct6694_can_priv *rx_offload_to_priv(struct can_rx_offload *offload) +{ + return container_of(offload, struct nct6694_can_priv, offload); +} + +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_rx_offload(struct can_rx_offload *offload, + struct sk_buff *skb) +{ + struct nct6694_can_priv *priv = rx_offload_to_priv(offload); + int ret; + + ret = can_rx_offload_queue_tail(offload, skb); + if (ret) + priv->ndev->stats.rx_fifo_errors++; +} + +static void nct6694_can_handle_lost_msg(struct net_device *ndev) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + struct net_device_stats *stats = &ndev->stats; + struct can_frame *cf; + struct sk_buff *skb; + + netdev_err(ndev, "RX FIFO overflow, message(s) lost.\n"); + + stats->rx_errors++; + stats->rx_over_errors++; + + skb = alloc_can_err_skb(ndev, &cf); + if (!skb) + return; + + cf->can_id |= CAN_ERR_CRTL; + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW; + + nct6694_can_rx_offload(&priv->offload, skb); +} + +static void nct6694_can_rx(struct net_device *ndev, u8 rx_evt) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + struct nct6694_can_frame *frame = &priv->rx->frame; + struct nct6694_cmd_header cmd_hd = { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_RECEIVE, + .sel = NCT6694_CAN_RECEIVE_SEL(priv->can_idx, 1), + .len = cpu_to_le16(sizeof(*frame)) + }; + struct canfd_frame *cfd; + struct can_frame *cf; + struct sk_buff *skb; + int ret; + + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, frame); + if (ret) + return; + + if (frame->flag & NCT6694_CAN_FRAME_FLAG_FD) { + skb = alloc_canfd_skb(priv->ndev, &cfd); + if (!skb) + return; + + cfd->can_id = le32_to_cpu(frame->id); + cfd->len = frame->length; + if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF) + cfd->can_id |= CAN_EFF_FLAG; + if (frame->flag & NCT6694_CAN_FRAME_FLAG_BRS) + cfd->flags |= CANFD_BRS; + if (frame->flag & NCT6694_CAN_FRAME_FLAG_ERR) + cfd->flags |= CANFD_ESI; + + memcpy(cfd->data, frame->data, cfd->len); + } else { + skb = alloc_can_skb(priv->ndev, &cf); + if (!skb) + return; + + cf->can_id = le32_to_cpu(frame->id); + cf->len = frame->length; + if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF) + cf->can_id |= CAN_EFF_FLAG; + if (frame->flag & NCT6694_CAN_FRAME_FLAG_RTR) + cf->can_id |= CAN_RTR_FLAG; + + memcpy(cf->data, frame->data, cf->len); + } + + nct6694_can_rx_offload(&priv->offload, skb); +} + +static void nct6694_can_clean(struct net_device *ndev) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + + if (priv->tx_skb || netif_queue_stopped(ndev)) + ndev->stats.tx_errors++; + dev_kfree_skb(priv->tx_skb); + priv->tx_skb = NULL; +} + +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 = priv->rx->event; + struct nct6694_cmd_header cmd_hd; + u8 mask = NCT6694_CAN_EVENT_REC | NCT6694_CAN_EVENT_TEC; + int ret; + + guard(mutex)(&priv->lock); + + cmd_hd = (struct nct6694_cmd_header) { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_EVENT, + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask), + .len = cpu_to_le16(sizeof(priv->rx->event)) + }; + + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); + if (ret < 0) + return ret; + + bec->rxerr = evt[priv->can_idx].rec; + bec->txerr = evt[priv->can_idx].tec; + + return 0; +} + +static void nct6694_can_handle_state_change(struct net_device *ndev, + u8 status) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + enum can_state new_state = priv->can.state; + enum can_state rx_state, tx_state; + struct can_berr_counter bec; + struct can_frame *cf; + struct sk_buff *skb; + + nct6694_can_get_berr_counter(ndev, &bec); + can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state); + + switch (status) { + case NCT6694_CAN_EVT_STS_ERROR_ACTIVE: + new_state = CAN_STATE_ERROR_ACTIVE; + break; + case NCT6694_CAN_EVT_STS_ERROR_PASSIVE: + new_state = CAN_STATE_ERROR_PASSIVE; + break; + case NCT6694_CAN_EVT_STS_BUS_OFF: + new_state = CAN_STATE_BUS_OFF; + break; + case NCT6694_CAN_EVT_STS_WARNING: + new_state = CAN_STATE_ERROR_WARNING; + break; + default: + netdev_err(ndev, "Receive unknown CAN status event.\n"); + return; + } + + /* state hasn't changed */ + if (new_state == priv->can.state) + return; + + skb = alloc_can_err_skb(ndev, &cf); + + tx_state = bec.txerr >= bec.rxerr ? new_state : 0; + rx_state = bec.txerr <= bec.rxerr ? new_state : 0; + can_change_state(ndev, cf, tx_state, rx_state); + + if (new_state == CAN_STATE_BUS_OFF) { + can_bus_off(ndev); + } else if (skb) { + cf->can_id |= CAN_ERR_CNT; + cf->data[6] = bec.txerr; + cf->data[7] = bec.rxerr; + } + + nct6694_can_rx_offload(&priv->offload, skb); +} + +static void nct6694_handle_bus_err(struct net_device *ndev, u8 bus_err) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + struct can_frame *cf; + struct sk_buff *skb; + + if (bus_err == NCT6694_CAN_EVT_ERR_NO_ERROR) + return; + + priv->can.can_stats.bus_error++; + + skb = alloc_can_err_skb(ndev, &cf); + if (skb) + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; + + switch (bus_err) { + case NCT6694_CAN_EVT_ERR_CRC_ERROR: + netdev_dbg(ndev, "CRC error\n"); + ndev->stats.rx_errors++; + if (skb) + cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ; + break; + + case NCT6694_CAN_EVT_ERR_STUFF_ERROR: + netdev_dbg(ndev, "Stuff error\n"); + ndev->stats.rx_errors++; + if (skb) + cf->data[2] |= CAN_ERR_PROT_STUFF; + break; + + case NCT6694_CAN_EVT_ERR_ACK_ERROR: + netdev_dbg(ndev, "Ack error\n"); + ndev->stats.tx_errors++; + if (skb) { + cf->can_id |= CAN_ERR_ACK; + cf->data[2] |= CAN_ERR_PROT_TX; + } + break; + + case NCT6694_CAN_EVT_ERR_FORM_ERROR: + netdev_dbg(ndev, "Form error\n"); + ndev->stats.rx_errors++; + if (skb) + cf->data[2] |= CAN_ERR_PROT_FORM; + break; + + case NCT6694_CAN_EVT_ERR_BIT_ERROR: + netdev_dbg(ndev, "Bit error\n"); + ndev->stats.tx_errors++; + if (skb) + cf->data[2] |= CAN_ERR_PROT_TX | CAN_ERR_PROT_BIT; + break; + + default: + break; + } + + nct6694_can_rx_offload(&priv->offload, skb); +} + +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++; + 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 = priv->rx->event; + struct nct6694_cmd_header cmd_hd; + u8 tx_evt, rx_evt, bus_err, can_status; + u8 mask_sts = NCT6694_CAN_EVENT_MASK; + irqreturn_t handled = IRQ_NONE; + int can_idx = priv->can_idx; + int ret; + + scoped_guard(mutex, &priv->lock) { + cmd_hd = (struct nct6694_cmd_header) { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_EVENT, + .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, mask_sts), + .len = cpu_to_le16(sizeof(priv->rx->event)) + }; + + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt); + if (ret < 0) + return handled; + + tx_evt = evt[can_idx].tx_evt; + rx_evt = evt[can_idx].rx_evt; + bus_err = evt[can_idx].err; + can_status = evt[can_idx].status; + } + + if (rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) { + nct6694_can_rx(ndev, rx_evt); + handled = IRQ_HANDLED; + } + + if (rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST) { + nct6694_can_handle_lost_msg(ndev); + handled = IRQ_HANDLED; + } + + if (can_status) { + nct6694_can_handle_state_change(ndev, can_status); + handled = IRQ_HANDLED; + } + + if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) { + nct6694_handle_bus_err(ndev, bus_err); + handled = IRQ_HANDLED; + } + + if (handled) + can_rx_offload_threaded_irq_finish(&priv->offload); + + if (tx_evt & NCT6694_CAN_EVT_TX_FIFO_EMPTY) + nct6694_can_tx_irq(ndev); + + return handled; +} + +static void nct6694_can_tx(struct net_device *ndev) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + struct nct6694_can_frame *frame = &priv->tx->frame; + struct nct6694_cmd_header cmd_hd = { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_DELIVER, + .sel = NCT6694_CAN_DELIVER_SEL(1), + .len = cpu_to_le16(sizeof(*frame)) + }; + struct net_device_stats *stats = &ndev->stats; + struct sk_buff *skb = priv->tx_skb; + struct canfd_frame *cfd; + struct can_frame *cf; + u32 txid; + int err; + + memset(frame, 0, sizeof(*frame)); + + if (priv->can_idx == 0) + frame->tag = NCT6694_CAN_FRAME_TAG_CAN0; + else + frame->tag = NCT6694_CAN_FRAME_TAG_CAN1; + + if (can_is_canfd_skb(skb)) { + cfd = (struct canfd_frame *)priv->tx_skb->data; + + if (cfd->flags & CANFD_BRS) + frame->flag |= NCT6694_CAN_FRAME_FLAG_BRS; + + if (cfd->can_id & CAN_EFF_FLAG) { + txid = cfd->can_id & CAN_EFF_MASK; + frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF; + } else { + txid = cfd->can_id & CAN_SFF_MASK; + } + frame->flag |= NCT6694_CAN_FRAME_FLAG_FD; + frame->id = cpu_to_le32(txid); + frame->length = cfd->len; + + memcpy(frame->data, cfd->data, cfd->len); + } else { + cf = (struct can_frame *)priv->tx_skb->data; + + if (cf->can_id & CAN_RTR_FLAG) + frame->flag |= NCT6694_CAN_FRAME_FLAG_RTR; + + if (cf->can_id & CAN_EFF_FLAG) { + txid = cf->can_id & CAN_EFF_MASK; + frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF; + } else { + txid = cf->can_id & CAN_SFF_MASK; + } + frame->id = cpu_to_le32(txid); + frame->length = cf->len; + + memcpy(frame->data, cf->data, cf->len); + } + + err = nct6694_write_msg(priv->nct6694, &cmd_hd, frame); + if (err) { + netdev_err(ndev, "%s: Tx FIFO full!\n", __func__); + can_free_echo_skb(ndev, 0, NULL); + stats->tx_dropped++; + stats->tx_errors++; + netif_wake_queue(ndev); + } +} + +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; + + guard(mutex)(&priv->lock); + + if (priv->tx_skb) { + if (priv->can.state == CAN_STATE_BUS_OFF) { + nct6694_can_clean(ndev); + } else { + nct6694_can_tx(ndev); + can_put_echo_skb(priv->tx_skb, ndev, 0, 0); + priv->tx_skb = NULL; + } + } +} + +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 (can_dev_dropped_skb(ndev, skb)) + return NETDEV_TX_OK; + + if (priv->tx_skb) { + netdev_err(ndev, "hard_xmit called while tx busy\n"); + return NETDEV_TX_BUSY; + } + + netif_stop_queue(ndev); + priv->tx_skb = skb; + queue_work(priv->wq, &priv->tx_work); + + return NETDEV_TX_OK; +} + +static int nct6694_can_start(struct net_device *ndev) +{ + struct nct6694_can_priv *priv = netdev_priv(ndev); + struct nct6694_can_setting *setting = &priv->tx->setting; + struct nct6694_cmd_header cmd_hd = { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_SETTING, + .sel = NCT6694_CAN_SETTING_SEL(priv->can_idx), + .len = cpu_to_le16(sizeof(*setting)) + }; + 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(setting, 0, sizeof(*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_SETTING_CTRL1_MON); + + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && + priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO) + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_NISO); + + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) + setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_LBCK); + + ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting); + if (ret) + return ret; + + priv->can.state = CAN_STATE_ERROR_ACTIVE; + + return ret; +} + +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; + can_rx_offload_disable(&priv->offload); + 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; + + can_rx_offload_enable(&priv->offload); + + 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; + + 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: + can_rx_offload_disable(&priv->offload); + close_candev(ndev); + return ret; +} + +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 = &priv->rx->info; + struct nct6694_cmd_header cmd_hd = { + .mod = NCT6694_CAN_MOD, + .cmd = NCT6694_CAN_INFORMATION, + .sel = NCT6694_CAN_INFORMATION_SEL, + .len = cpu_to_le16(sizeof(*info)) + }; + int ret; + + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, 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 irq; + + 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 = devm_kzalloc(&pdev->dev, sizeof(union nct6694_can_tx), + GFP_KERNEL); + if (!priv->tx) { + ret = -ENOMEM; + goto free_candev; + } + + priv->rx = devm_kzalloc(&pdev->dev, sizeof(union nct6694_can_rx), + GFP_KERNEL); + if (!priv->rx) { + ret = -ENOMEM; + goto free_candev; + } + + can_clk = nct6694_can_get_clock(priv); + if (can_clk < 0) { + ret = dev_err_probe(&pdev->dev, can_clk, + "Failed to get clock\n"); + goto free_candev; + } + + devm_mutex_init(&pdev->dev, &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; + + ret = can_rx_offload_add_manual(ndev, &priv->offload, + NCT6694_NAPI_WEIGHT); + if (ret) { + dev_err_probe(&pdev->dev, ret, "Failed to add rx_offload\n"); + goto free_candev; + } + + platform_set_drvdata(pdev, priv); + SET_NETDEV_DEV(priv->ndev, &pdev->dev); + + ret = register_candev(priv->ndev); + if (ret) + goto del_rx_offload; + + return 0; + +del_rx_offload: + can_rx_offload_del(&priv->offload); +free_candev: + free_candev(ndev); + return ret; +} + +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); + can_rx_offload_del(&priv->offload); + 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 <a0282524688@gmail.com> --- MAINTAINERS | 1 + drivers/net/can/usb/Kconfig | 10 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/nct6694_canfd.c | 856 ++++++++++++++++++++++++++++ 4 files changed, 868 insertions(+) create mode 100644 drivers/net/can/usb/nct6694_canfd.c