| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAOoeyxW=k35-bkeqNmhyZwUxjy=g3irTBS5mbXLxqp1Stx-Zfg@mail.gmail.com>
Date: Tue, 14 Jan 2025 18:46:40 +0800
From: Ming Yu <a0282524688@...il.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
andi.shyti@...nel.org, mkl@...gutronix.de, andrew+netdev@...n.ch,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com,
alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-rtc@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v5 4/7] can: Add Nuvoton NCT6694 CAN support
Dear Vincent,
Thank you for your reply,
I'll add comments to describe these locks in the next patch,
Vincent Mailhol <mailhol.vincent@...adoo.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
Powered by blists - more mailing lists