Dear Marc,

Thank you for your comments,

Marc Kleine-Budde [off-list ref] 於 2024年12月11日 週三 下午5:59寫道：

On 10.12.2024 18:45:21, Ming Yu wrote:

quoted

This driver supports Socket CANfd functionality for NCT6694 MFD
device based on USB interface.

Please use the rx-offload helper, otherwise the CAN frames might be
revived out of order.

Understood. I'll make the modifications in the next patch.

quoted

...

quoted

+
+struct __packed nct6694_can_event {
+     u8 err1;
+     u8 status1;
+     u8 tx_evt1;
+     u8 rx_evt1;
+     u8 rec1;
+     u8 tec1;
+     u8 reserved1[2];
+     u8 err2;
+     u8 status2;
+     u8 tx_evt2;
+     u8 rx_evt2;
+     u8 rec2;
+     u8 tec2;
+     u8 reserved2[2];
+};

Create an extra struct that only describes a channel

struct __packed nct6694_can_event_channel {
        u8 err;
        u8 status;
        u8 tx_evt;
        u8 rx_evt;
        u8 rec;
        u8 tec;
        u8 reserved[2];
}

and put an array of 2 into struct __packed nct6694_can_event.

Fix it in v4.

quoted

+
+struct __packed nct6694_can_xmit {
+     u8 tag;
+     u8 flag;
+     u8 reserved;
+     u8 dlc;
+     __le32 id;
+     u8 data[64];
+     u8 msg_buf[72];

Why is the message so long? What's in the msg_buf?

It's deprecated, I'll drop it in v4.

quoted

+};
+
+struct nct6694_can_priv {
+     struct can_priv can;    /* must be the first member */
+     struct net_device *ndev;
+     struct nct6694 *nct6694;
+     struct mutex lock;

What does lock protect?

The lock is used to protect tx_buf and rx_buf for each CAN device.

quoted

+     struct sk_buff *tx_skb;
+     struct workqueue_struct *wq;
+     struct work_struct tx_work;
+     unsigned char *tx_buf;

void *

quoted

+     unsigned char *rx_buf;

void *

quoted

+     unsigned char can_idx;
+     bool tx_busy;

IMHO it makes no sense to have tx_skb and tx_busy

Okay! I will revisit these to evaluate whether they are still necessary.

quoted

+};
+

...

quoted

+static void nct6694_can_read_fifo(struct net_device *ndev)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);
+     struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
+     struct net_device_stats *stats = &ndev->stats;
+     struct canfd_frame *cf;
+     struct sk_buff *skb;
+     int can_idx = priv->can_idx;
+     u32 id;
+     int ret;
+     u8 fd_format = 0;

bool - no need to init

Fix it in v4.

quoted

+
+     guard(mutex)(&priv->lock);
+
+     ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+                            NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
+                            sizeof(struct nct6694_can_xmit), xmit);
+     if (ret < 0)
+             return;
+
+     /* Check type of frame and create skb */
+     fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
+     if (fd_format)
+             skb = alloc_canfd_skb(ndev, &cf);
+     else
+             skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
+
+     if (!skb) {
+             stats->rx_dropped++;
+             return;
+     }
+
+     cf->len = xmit->dlc;

what does xmit->dlc contain? The DLC or the length?

It reflects the actual data length.

quoted

+
+     /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
+     id = le32_to_cpu(xmit->id);
+     if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
+             /*
+              * In case the Extended ID frame is received, the standard
+              * and extended part of the ID are swapped in the register,
+              * so swap them back to obtain the correct ID.
+              */

You comment doesn't match the code.

Fix it in v4.

quoted

+             id |= CAN_EFF_FLAG;
+     }
+
+     cf->can_id = id;
+
+     /* Set ESI flag */
+     if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
+             cf->flags |= CANFD_ESI;
+             netdev_dbg(ndev, "ESI Error\n");
+     }
+
+     /* Set RTR and BRS */
+     if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
+             cf->can_id |= CAN_RTR_FLAG;
+     } else {
+             if (xmit->flag & NCT6694_CAN_FLAG_BRS)
+                     cf->flags |= CANFD_BRS;
+
+             memcpy(cf->data, xmit->data, cf->len);
+
+             stats->rx_bytes += cf->len;
+     }
+
+     stats->rx_packets++;
+
+     netif_receive_skb(skb);
+}

...

quoted

+static int nct6694_can_poll(struct net_device *ndev, int quota)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);
+     struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
+     int can_idx = priv->can_idx;
+     int work_done = 0, ret;
+     u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
+     u8 bus_err, can_status;
+
+     scoped_guard(mutex, &priv->lock) {
+             ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+                                    NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
+                                    sizeof(struct nct6694_can_event), evt);
+             if (ret < 0)
+                     return IRQ_NONE;

propagate the error

Fix it in v4.

quoted

+
+             if (can_idx) {
+                     bus_err = evt->err2;
+                     can_status = evt->status2;
+             } else {
+                     bus_err = evt->err1;
+                     can_status = evt->status1;
+             }
+     }
+

...

quoted

+static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
+                                       struct net_device *ndev)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);
+
+     if (priv->tx_skb || priv->tx_busy) {
+             netdev_err(ndev, "hard_xmit called while tx busy\n");
+             return NETDEV_TX_BUSY;
+     }
+
+     if (can_dev_dropped_skb(ndev, skb))
+             return NETDEV_TX_OK;

please drop first

Fix it in v4.

quoted

+
+     netif_stop_queue(ndev);
+     priv->tx_skb = skb;
+     queue_work(priv->wq, &priv->tx_work);
+
+     return NETDEV_TX_OK;
+}
+
+static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);
+     struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
+     u32 txid = 0;
+
+     memset(xmit, 0, sizeof(struct nct6694_can_xmit));
+
+     if (priv->can_idx == 0)
+             xmit->tag = NCT6694_CAN_TAG_CAN0<;
+     else
+             xmit->tag = NCT6694_CAN_TAG_CAN1;
+
+     if (cf->can_id & CAN_EFF_FLAG) {
+             txid = cf->can_id & CAN_EFF_MASK;
+             /*
+              * In case the Extended ID frame is transmitted, the
+              * standard and extended part of the ID are swapped
+              * in the register, so swap them back to send the
+              * correct ID.

You comment doesn't match the code.

Fix it in v4.

quoted

+              */
+             xmit->flag |= NCT6694_CAN_FLAG_EFF;
+     } else {
+             txid = cf->can_id & CAN_SFF_MASK;
+     }
+
+     xmit->id = cpu_to_le32(txid);
+     xmit->dlc = cf->len;
+
+     if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&

No need to check ctrlmode

Fix it in v4.

quoted

+         can_is_canfd_skb(priv->tx_skb)) {
+             xmit->flag |= NCT6694_CAN_FLAG_FD;
+             if (cf->flags & CANFD_BRS)
+                     xmit->flag |= NCT6694_CAN_FLAG_BRS;
+     }
+
+     if (cf->can_id & CAN_RTR_FLAG)
+             xmit->flag |= NCT6694_CAN_FLAG_RTR;

you can move this into the !can_is_canfd_skb branch of the if

Fix it in v4.

quoted

+
+     memcpy(xmit->data, cf->data, cf->len);
+
+     nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
+                       NCT6694_CAN_CMD10_OFFSET(1),
+                       sizeof(struct nct6694_can_xmit),
+                       xmit);
+}
+
+static void nct6694_can_tx_work(struct work_struct *work)
+{
+     struct nct6694_can_priv *priv = container_of(work,
+                                                  struct nct6694_can_priv,
+                                                  tx_work);
+     struct net_device *ndev = priv->ndev;
+     struct canfd_frame *cf;
+
+     guard(mutex)(&priv->lock);
+
+     if (priv->tx_skb) {
+             if (priv->can.state == CAN_STATE_BUS_OFF) {
+                     nct6694_can_clean(ndev);
+             } else {
+                     cf = (struct canfd_frame *)priv->tx_skb->data;
+                     nct6694_can_tx(ndev, cf);
+                     priv->tx_busy = true;
+                     can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
+                     priv->tx_skb = NULL;
+             }
+     }
+}
+
+static const struct net_device_ops nct6694_can_netdev_ops = {
+     .ndo_open = nct6694_can_open,
+     .ndo_stop = nct6694_can_stop,
+     .ndo_start_xmit = nct6694_can_start_xmit,
+     .ndo_change_mtu = can_change_mtu,
+};
+
+static const struct ethtool_ops nct6694_can_ethtool_ops = {
+     .get_ts_info = ethtool_op_get_ts_info,
+};
+
+static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
+{
+     struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
+     int ret;
+
+     ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
+                            NCT6694_CAN_CMD1_OFFSET,
+                            sizeof(struct nct6694_can_information),
+                            info);
+     if (ret)
+             return ret;
+
+     return le32_to_cpu(info->can_clk);
+}
+
+static int nct6694_can_probe(struct platform_device *pdev)
+{
+     const struct mfd_cell *cell = mfd_get_cell(pdev);
+     struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
+     struct nct6694_can_priv *priv;
+     struct net_device *ndev;
+     int ret, irq, can_clk;
+
+     irq = irq_create_mapping(nct6694->domain,
+                              NCT6694_IRQ_CAN1 + cell->id);
+     if (!irq)
+             return -EINVAL;

propagate error value

Fix it in v4.

quoted

+
+     ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
+     if (!ndev)
+             return -ENOMEM;
+
+     ndev->irq = irq;
+     ndev->flags |= IFF_ECHO;
+     ndev->netdev_ops = &nct6694_can_netdev_ops;
+     ndev->ethtool_ops = &nct6694_can_ethtool_ops;
+
+     priv = netdev_priv(ndev);
+     priv->nct6694 = nct6694;
+     priv->ndev = ndev;
+
+     priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
+                                 sizeof(unsigned char), GFP_KERNEL);

devm_kzalloc()

quoted

+     if (!priv->tx_buf) {
+             ret = -ENOMEM;
+             goto free_candev;
+     }
+
+     priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
+                                 sizeof(unsigned char), GFP_KERNEL);

devm_kzalloc()

Fix it in v4.

quoted

+     if (!priv->rx_buf) {
+             ret = -ENOMEM;
+             goto free_candev;
+     }
+
+     can_clk = nct6694_can_get_clock(priv);
+     if (can_clk < 0) {
+             ret = -EIO;

propagate the error value, don't overwrite it

move the dev_err_probe() here.

Fix it in v4.

quoted

+             goto free_candev;
+     }
+
+     mutex_init(&priv->lock);
+     INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
+
+     priv->can_idx = cell->id;
+     priv->can.state = CAN_STATE_STOPPED;
+     priv->can.clock.freq = can_clk;
+     priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
+     priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
+     priv->can.do_set_mode = nct6694_can_set_mode;
+     priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
+
+     priv->can.ctrlmode = CAN_CTRLMODE_FD;
+
+     priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK            |
+                                    CAN_CTRLMODE_LISTENONLY          |
+                                    CAN_CTRLMODE_FD                  |
+                                    CAN_CTRLMODE_FD_NON_ISO          |
+                                    CAN_CTRLMODE_BERR_REPORTING;
+
+     platform_set_drvdata(pdev, priv);
+     SET_NETDEV_DEV(priv->ndev, &pdev->dev);
+
+     ret = register_candev(priv->ndev);
+     if (ret)
+             goto free_candev;
+
+     return 0;
+
+free_candev:
+     free_candev(ndev);
+     return dev_err_probe(&pdev->dev, ret, "Probe failed\n");

Move the dev_err_probe() with an appropriate error message to where the
error occurs. If malloc fails, the kernel already prints for you, so
here it's only nct6694_can_get_clock() only.

Understood. I'll make the modifications in the next patch.

quoted

+}
+
+static void nct6694_can_remove(struct platform_device *pdev)
+{
+     struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
+
+     cancel_work_sync(&priv->tx_work);
+     unregister_candev(priv->ndev);
+     free_candev(priv->ndev);
+}
+
+static struct platform_driver nct6694_can_driver = {
+     .driver = {
+             .name   = DRVNAME,
+     },
+     .probe          = nct6694_can_probe,
+     .remove         = nct6694_can_remove,
+};
+
+module_platform_driver(nct6694_can_driver);
+
+MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
+MODULE_AUTHOR("Ming Yu [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nct6694-can");
--
2.34.1

regards,
Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Best regards,
Ming