| 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: <CAOoeyxWSsy0Q0Y7iJE8-DZM5Yvcdto8mncFkM8X4BvVMEgfUiQ@mail.gmail.com>
Date: Mon, 17 Mar 2025 10:24:11 +0800
From: Ming Yu <a0282524688@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl,
andi.shyti@...nel.org, mailhol.vincent@...adoo.fr, 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 v8 4/7] can: Add Nuvoton NCT6694 CANFD support
Marc Kleine-Budde <mkl@...gutronix.de> 於 2025年2月28日 週五 下午6:44寫道：
>
> > +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");
>
> Please make it a netdev_dbg()
>
Fix it in v9.
> > +
> > + 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;
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CAN_MOD,
> > + .cmd = NCT6694_CAN_RECEIVE,
> > + .sel = NCT6694_CAN_RECEIVE_SEL(ndev->dev_port, 1),
> > + .len = cpu_to_le16(sizeof(*frame))
> > + };
> > + 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) {
> > + struct canfd_frame *cfd;
> > +
> > + skb = alloc_canfd_skb(priv->ndev, &cfd);
> > + if (!skb)
>
> stats->rx_dropped++;
>
Add it in v9.
> > + return;
> > +
> > + cfd->can_id = le32_to_cpu(frame->id);
> > + 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 {
> > + struct can_frame *cf;
> > +
> > + skb = alloc_can_skb(priv->ndev, &cf);
> > + if (!skb)
>
> stats->rx_dropped++;
>
Add it in v9.
> > + return;
> > +
> > + cf->can_id = le32_to_cpu(frame->id);
> > + cf->len = can_cc_dlc2len(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;
> > + else
> > + memcpy(cf->data, frame->data, cf->len);
> > + }
> > +
> > + nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +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);
> > +
> > + *bec = priv->bec;
> > +
> > + return 0;
> > +}
> > +
> > +static void nct6694_can_handle_state_change(struct net_device *ndev,
> > + enum can_state new_state)
> > +{
>
> I just replied to one of your questions of the last series:
>
> | https://lore.kernel.org/20250228-magic-seahorse-of-abracadabra-f2a402-mkl@pengutronix.de
>
Understood, I will make the modifications in the next patch.
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct can_berr_counter bec;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > +
> > + nct6694_can_get_berr_counter(ndev, &bec);
> > +
> > + switch (new_state) {
> > + case CAN_STATE_ERROR_ACTIVE:
> > + priv->can.can_stats.error_warning++;
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + if (cf)
> > + cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
> > + break;
> > + case CAN_STATE_ERROR_WARNING:
> > + priv->can.can_stats.error_warning++;
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + if (cf) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + if (bec.txerr > bec.rxerr)
> > + cf->data[1] = CAN_ERR_CRTL_TX_WARNING;
> > + else
> > + cf->data[1] = CAN_ERR_CRTL_RX_WARNING;
> > + cf->data[6] = bec.txerr;
> > + cf->data[7] = bec.rxerr;
> > + }
> > + break;
> > + case CAN_STATE_ERROR_PASSIVE:
> > + priv->can.can_stats.error_passive++;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + if (cf) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > + if (bec.txerr >= CAN_ERROR_PASSIVE_THRESHOLD)
> > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > + cf->data[6] = bec.txerr;
> > + cf->data[7] = bec.rxerr;
> > + }
> > + break;
> > + case CAN_STATE_BUS_OFF:
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + priv->can.can_stats.bus_off++;
> > + if (cf)
> > + cf->can_id |= CAN_ERR_BUSOFF;
>
> no need for can_free_echo_skb() and netif_stop_queue()
>
Fix these in v9.
> > + can_free_echo_skb(ndev, 0, NULL);
> > + netif_stop_queue(ndev);
>
> > + can_bus_off(ndev);
> > + break;
> > + default:
> > + break;
> > + }
> > +
>
> if (skb)
>
Add it in v9.
> > + nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_handle_state_errors(struct net_device *ndev, u8 status)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (status == NCT6694_CAN_EVT_STS_ERROR_ACTIVE &&
> > + priv->can.state != CAN_STATE_ERROR_ACTIVE) {
> > + netdev_dbg(ndev, "Error, entered active state\n");
> > + nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_ACTIVE);
> > + }
> > +
> > + if (status == NCT6694_CAN_EVT_STS_WARNING &&
> > + priv->can.state != CAN_STATE_ERROR_WARNING) {
> > + netdev_dbg(ndev, "Error, entered warning state\n");
> > + nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_WARNING);
> > + }
> > +
> > + if (status == NCT6694_CAN_EVT_STS_ERROR_PASSIVE &&
> > + priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> > + netdev_dbg(ndev, "Error, entered passive state\n");
> > + nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_PASSIVE);
> > + }
> > +
> > + if (status == NCT6694_CAN_EVT_STS_BUS_OFF &&
> > + priv->can.state != CAN_STATE_BUS_OFF) {
> > + netdev_dbg(ndev, "Error, entered bus-off state\n");
> > + nct6694_can_handle_state_change(ndev, CAN_STATE_BUS_OFF);
> > + }
> > +}
> > +
> > +static void nct6694_can_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);
>
> make it: "if (cf)" instead of "if (skb)" then interacting with the "cf".
>
Fix it in v9.
> > + 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;
> > + }
> > +
> > + if (skb)
> > + 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;
> > +
> > + stats->tx_bytes += can_rx_offload_get_echo_skb_queue_tail(&priv->offload,
> > + 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 *event = &priv->event[ndev->dev_port];
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CAN_MOD,
> > + .cmd = NCT6694_CAN_EVENT,
> > + .sel = NCT6694_CAN_EVENT_SEL(ndev->dev_port, NCT6694_CAN_EVENT_MASK),
> > + .len = cpu_to_le16(sizeof(priv->event))
> > + };
> > + irqreturn_t handled = IRQ_NONE;
> > + int ret;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, priv->event);
> > + if (ret < 0)
> > + return handled;
> > +
> > + if (event->rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) {
> > + nct6694_can_rx(ndev, event->rx_evt);
>
> just for symmetry name rename nct6694_can_rx() to nct6694_can_handle_rx()
>
Fix it in v9.
> > + handled = IRQ_HANDLED;
> > + }
> > +
> > + if (event->rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST) {
> > + nct6694_can_handle_lost_msg(ndev);
> > + handled = IRQ_HANDLED;
> > + }
> > +
> > + if (event->status) {
> > + nct6694_can_handle_state_errors(ndev, event->status);
>
> I think you should call nct6694_can_handle_state_change() directly and
> get rid of the code on nct6694_can_handle_state_errors().
>
Fix it in v9.
> > + handled = IRQ_HANDLED;
> > + }
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
> > + nct6694_can_handle_bus_err(ndev, event->err);
> > + handled = IRQ_HANDLED;
> > + }
>
> The logic needs to be refined. If there was an IRQ you must say you've
> handled it, event if user space doesn't care for bus errors.
>
> The better solution is to disabled event->err related IRQs in the
> hardware, if CAN_CTRLMODE_BERR_REPORTING is disabled, so you don't get
> an IRQ in the first place.
>
> if (event->err != NCT6694_CAN_EVT_ERR_NO_ERROR) {
> if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> nct6694_can_handle_bus_err(ndev, event->err);
> handled = IRQ_HANDLED;
> }
>
Fix it in v9.
> > +
> > + if (event->tx_evt & NCT6694_CAN_EVT_TX_FIFO_EMPTY) {
> > + nct6694_can_tx_irq(ndev);
>
> just for symmetry name rename nct6694_can_tx() to nct6694_can_handle_tx()
>
Fix it in v9.
> > + handled = IRQ_HANDLED;
> > + }
> > +
> > + if (handled)
> > + can_rx_offload_threaded_irq_finish(&priv->offload);
> > +
> > + priv->bec.rxerr = event->rec;
> > + priv->bec.txerr = event->tec;
> > +
> > + return handled;
> > +}
> > +
> > +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 nct6694_can_frame *frame = &priv->tx;
> > + struct net_device *ndev = priv->ndev;
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct sk_buff *skb = priv->can.echo_skb[0];
> > + static const 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))
> > + };
> > + u32 txid;
> > + int err;
> > +
> > + memset(frame, 0, sizeof(*frame));
> > +
> > + frame->tag = NCT6694_CAN_FRAME_TAG(ndev->dev_port);
> > +
> > + if (can_is_canfd_skb(skb)) {
> > + struct canfd_frame *cfd = (struct canfd_frame *)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;
>
> frame->length = canfd_sanitize_len(cfd->len);
>
Fix it in v9.
> > +
> > + memcpy(frame->data, cfd->data, cfd->len);
>
> memcpy(frame->data, cfd->data, frame->length);
>
Fix it in v9.
> > + } else {
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > +
> > + 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;
> > + }
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + frame->flag |= NCT6694_CAN_FRAME_FLAG_RTR;
> > + else
> > + memcpy(frame->data, cf->data, cf->len);
> > +
> > + frame->id = cpu_to_le32(txid);
> > + frame->length = cf->len;
> > + }
> > +
> > + err = nct6694_write_msg(priv->nct6694, &cmd_hd, frame);
> > + if (err) {
> > + netdev_err(ndev, "%s: TX FIFO is full!\n", __func__);
>
> The FIFO should not be full at this time. There also could be an USB
> error due to unlug or EMI. I think you should remove this error message.
>
Fix it in v9.
> > + can_free_echo_skb(ndev, 0, NULL);
> > + stats->tx_dropped++;
> > + stats->tx_errors++;
> > + netif_wake_queue(ndev);
> > + }
> > +}
> > +
> > +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;
> > +
> > + netif_stop_queue(ndev);
> > + can_put_echo_skb(skb, ndev, 0, 0);
> > + 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);
> > + const struct can_bittiming *d_bt = &priv->can.data_bittiming;
> > + const struct can_bittiming *n_bt = &priv->can.bittiming;
> > + struct nct6694_can_setting *setting __free(kfree) = NULL;
> > + const struct nct6694_cmd_header cmd_hd = {
> > + .mod = NCT6694_CAN_MOD,
> > + .cmd = NCT6694_CAN_SETTING,
> > + .sel = ndev->dev_port,
> > + .len = cpu_to_le16(sizeof(*setting))
> > + };
> > + int ret;
> > +
> > + setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > + if (!setting)
> > + return -ENOMEM;
> > +
> > + 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_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 0;
> > +}
> > +
> > +static int nct6694_can_stop(struct net_device *ndev)
>
> static void
>
Fix it in v9.
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + priv->can.ctrlmode = CAN_CTRLMODE_LISTENONLY;
>
> As Vincent already pointed out, bring you device into listen only mode.
> Please add a comment why you do it, because it cannot be stopped.
>
Okay! I will add the comment in the next patch.
> > + netif_stop_queue(ndev);
> > + free_irq(ndev->irq, ndev);
> > + destroy_workqueue(priv->wq);
> > + can_rx_offload_disable(&priv->offload);
> > + 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)
> > +{
> > + int ret;
> > +
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + ret = nct6694_can_start(ndev);
>
> if (ret)
> return ret;
>
> netif_wake_queue(ndev);
>
Fix it in v9.
> > + if (!ret && netif_queue_stopped(ndev))
> > + netif_wake_queue(ndev);
> > + break;
> > +
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +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;
> > + }
> > +
> > + 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 __free(kfree) = NULL;
> > + static const 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, can_clk;
> > +
> > + info = kzalloc(sizeof(*info), GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, &cmd_hd, info);
> > + if (ret)
> > + return ret;
> > +
> > + can_clk = le32_to_cpu(info->can_clk);
> > +
> > + return 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_CAN0 + 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->dev_port = cell->id;
> > + ndev->netdev_ops = &nct6694_can_netdev_ops;
> > + ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->nct6694 = nct6694;
> > + priv->ndev = ndev;
> > +
> > + 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;
> > + }
> > +
> > + INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
>
> please remove CAN_STATE_STOPPED, it's not needed
>
Fix it in v9.
> > + 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_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING |
> > + CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO;
>
> Does your device run in CAN-FD mode all the time? If so, please use
> can_set_static_ctrlmode() to set it after priv->can.ctrlmode_supported
> and remove CAN_CTRLMODE_FD from ctrlmode_supported.
>
Our device is designed to allow users to dynamically switch between
Classical CAN and CAN-FD mode via ip link set ... fd on/off.
Therefore, CAN_CTRLMODE_FD needs to remain in ctrlmode_supported, and
can_set_static_ctrlmode() is not suitable in this case.
Please let me know if you have any concerns about this approach.
> > +
> > + 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 rx_offload_del;
> > +
> > + return 0;
> > +
> > +rx_offload_del:
> > + 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);
> > +
> > + 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@...oton.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:nct6694-can");
>
> There should be no need for the module alias.
>
Remove it in v9.
Thanks,
Ming
Powered by blists - more mailing lists