Dear Marc,
Thank you for your comments,
Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 於 2024年12月11日 週三 下午5:59寫道：
>
> On 10.12.2024 18:45:21, Ming Yu wrote:
> > This driver supports Socket CANfd functionality for NCT6694 MFD
> > device based on USB interface.
>
> Please use the rx-offload helper, otherwise the CAN frames might be
> revived out of order.
>
Understood. I'll make the modifications in the next patch.
> >
...
> > +
> > +struct __packed nct6694_can_event {
> > + u8 err1;
> > + u8 status1;
> > + u8 tx_evt1;
> > + u8 rx_evt1;
> > + u8 rec1;
> > + u8 tec1;
> > + u8 reserved1[2];
> > + u8 err2;
> > + u8 status2;
> > + u8 tx_evt2;
> > + u8 rx_evt2;
> > + u8 rec2;
> > + u8 tec2;
> > + u8 reserved2[2];
> > +};
>
> Create an extra struct that only describes a channel
>
> struct __packed nct6694_can_event_channel {
> u8 err;
> u8 status;
> u8 tx_evt;
> u8 rx_evt;
> u8 rec;
> u8 tec;
> u8 reserved[2];
> }
>
> and put an array of 2 into struct __packed nct6694_can_event.
>
Fix it in v4.
> > +
> > +struct __packed nct6694_can_xmit {
> > + u8 tag;
> > + u8 flag;
> > + u8 reserved;
> > + u8 dlc;
> > + __le32 id;
> > + u8 data[64];
> > + u8 msg_buf[72];
>
> Why is the message so long? What's in the msg_buf?
>
It's deprecated, I'll drop it in v4.
> > +};
> > +
> > +struct nct6694_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct net_device *ndev;
> > + struct nct6694 *nct6694;
> > + struct mutex lock;
>
> What does lock protect?
>
The lock is used to protect tx_buf and rx_buf for each CAN device.
> > + struct sk_buff *tx_skb;
> > + struct workqueue_struct *wq;
> > + struct work_struct tx_work;
> > + unsigned char *tx_buf;
> void *
> > + unsigned char *rx_buf;
> void *
> > + unsigned char can_idx;
> > + bool tx_busy;
>
> IMHO it makes no sense to have tx_skb and tx_busy
>
Okay! I will revisit these to evaluate whether they are still necessary.
> > +};
> > +
...
> > +static void nct6694_can_read_fifo(struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->rx_buf;
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct canfd_frame *cf;
> > + struct sk_buff *skb;
> > + int can_idx = priv->can_idx;
> > + u32 id;
> > + int ret;
> > + u8 fd_format = 0;
> bool - no need to init
Fix it in v4.
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD11_OFFSET(can_idx, 1),
> > + sizeof(struct nct6694_can_xmit), xmit);
> > + if (ret < 0)
> > + return;
> > +
> > + /* Check type of frame and create skb */
> > + fd_format = xmit->flag & NCT6694_CAN_FLAG_FD;
> > + if (fd_format)
> > + skb = alloc_canfd_skb(ndev, &cf);
> > + else
> > + skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
> > +
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
> > + }
> > +
> > + cf->len = xmit->dlc;
>
> what does xmit->dlc contain? The DLC or the length?
>
It reflects the actual data length.
> > +
> > + /* Get ID and set flag by its type(Standard ID format or Ext ID format) */
> > + id = le32_to_cpu(xmit->id);
> > + if (xmit->flag & NCT6694_CAN_FLAG_EFF) {
> > + /*
> > + * In case the Extended ID frame is received, the standard
> > + * and extended part of the ID are swapped in the register,
> > + * so swap them back to obtain the correct ID.
> > + */
>
> You comment doesn't match the code.
>
Fix it in v4.
> > + id |= CAN_EFF_FLAG;
> > + }
> > +
> > + cf->can_id = id;
> > +
> > + /* Set ESI flag */
> > + if (xmit->flag & NCT6694_CAN_FLAG_ERR) {
> > + cf->flags |= CANFD_ESI;
> > + netdev_dbg(ndev, "ESI Error\n");
> > + }
> > +
> > + /* Set RTR and BRS */
> > + if (!fd_format && (xmit->flag & NCT6694_CAN_FLAG_RTR)) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + if (xmit->flag & NCT6694_CAN_FLAG_BRS)
> > + cf->flags |= CANFD_BRS;
> > +
> > + memcpy(cf->data, xmit->data, cf->len);
> > +
> > + stats->rx_bytes += cf->len;
> > + }
> > +
> > + stats->rx_packets++;
> > +
> > + netif_receive_skb(skb);
> > +}
...
> > +static int nct6694_can_poll(struct net_device *ndev, int quota)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_event *evt = (struct nct6694_can_event *)priv->rx_buf;
> > + int can_idx = priv->can_idx;
> > + int work_done = 0, ret;
> > + u8 evt_mask = NCT6694_CAN_EVENT_ERR | NCT6694_CAN_EVENT_STATUS;
> > + u8 bus_err, can_status;
> > +
> > + scoped_guard(mutex, &priv->lock) {
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD2_OFFSET(can_idx, evt_mask),
> > + sizeof(struct nct6694_can_event), evt);
> > + if (ret < 0)
> > + return IRQ_NONE;
>
> propagate the error
>
Fix it in v4.
> > +
> > + if (can_idx) {
> > + bus_err = evt->err2;
> > + can_status = evt->status2;
> > + } else {
> > + bus_err = evt->err1;
> > + can_status = evt->status1;
> > + }
> > + }
> > +
...
> > +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> > + struct net_device *ndev)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > + if (priv->tx_skb || priv->tx_busy) {
> > + netdev_err(ndev, "hard_xmit called while tx busy\n");
> > + return NETDEV_TX_BUSY;
> > + }
> > +
> > + if (can_dev_dropped_skb(ndev, skb))
> > + return NETDEV_TX_OK;
>
> please drop first
>
Fix it in v4.
> > +
> > + netif_stop_queue(ndev);
> > + priv->tx_skb = skb;
> > + queue_work(priv->wq, &priv->tx_work);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static void nct6694_can_tx(struct net_device *ndev, struct canfd_frame *cf)
> > +{
> > + struct nct6694_can_priv *priv = netdev_priv(ndev);
> > + struct nct6694_can_xmit *xmit = (struct nct6694_can_xmit *)priv->tx_buf;
> > + u32 txid = 0;
> > +
> > + memset(xmit, 0, sizeof(struct nct6694_can_xmit));
> > +
> > + if (priv->can_idx == 0)
> > + xmit->tag = NCT6694_CAN_TAG_CAN0<;
> > + else
> > + xmit->tag = NCT6694_CAN_TAG_CAN1;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + txid = cf->can_id & CAN_EFF_MASK;
> > + /*
> > + * In case the Extended ID frame is transmitted, the
> > + * standard and extended part of the ID are swapped
> > + * in the register, so swap them back to send the
> > + * correct ID.
>
> You comment doesn't match the code.
>
Fix it in v4.
> > + */
> > + xmit->flag |= NCT6694_CAN_FLAG_EFF;
> > + } else {
> > + txid = cf->can_id & CAN_SFF_MASK;
> > + }
> > +
> > + xmit->id = cpu_to_le32(txid);
> > + xmit->dlc = cf->len;
> > +
> > + if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
>
> No need to check ctrlmode
>
Fix it in v4.
> > + can_is_canfd_skb(priv->tx_skb)) {
> > + xmit->flag |= NCT6694_CAN_FLAG_FD;
> > + if (cf->flags & CANFD_BRS)
> > + xmit->flag |= NCT6694_CAN_FLAG_BRS;
> > + }
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + xmit->flag |= NCT6694_CAN_FLAG_RTR;
>
> you can move this into the !can_is_canfd_skb branch of the if
>
Fix it in v4.
> > +
> > + memcpy(xmit->data, cf->data, cf->len);
> > +
> > + nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD10_OFFSET(1),
> > + sizeof(struct nct6694_can_xmit),
> > + xmit);
> > +}
> > +
> > +static void nct6694_can_tx_work(struct work_struct *work)
> > +{
> > + struct nct6694_can_priv *priv = container_of(work,
> > + struct nct6694_can_priv,
> > + tx_work);
> > + struct net_device *ndev = priv->ndev;
> > + struct canfd_frame *cf;
> > +
> > + guard(mutex)(&priv->lock);
> > +
> > + if (priv->tx_skb) {
> > + if (priv->can.state == CAN_STATE_BUS_OFF) {
> > + nct6694_can_clean(ndev);
> > + } else {
> > + cf = (struct canfd_frame *)priv->tx_skb->data;
> > + nct6694_can_tx(ndev, cf);
> > + priv->tx_busy = true;
> > + can_put_echo_skb(priv->tx_skb, ndev, 0, 0);
> > + priv->tx_skb = NULL;
> > + }
> > + }
> > +}
> > +
> > +static const struct net_device_ops nct6694_can_netdev_ops = {
> > + .ndo_open = nct6694_can_open,
> > + .ndo_stop = nct6694_can_stop,
> > + .ndo_start_xmit = nct6694_can_start_xmit,
> > + .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static const struct ethtool_ops nct6694_can_ethtool_ops = {
> > + .get_ts_info = ethtool_op_get_ts_info,
> > +};
> > +
> > +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> > +{
> > + struct nct6694_can_information *info = (struct nct6694_can_information *)priv->rx_buf;
> > + int ret;
> > +
> > + ret = nct6694_read_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_CMD1_OFFSET,
> > + sizeof(struct nct6694_can_information),
> > + info);
> > + if (ret)
> > + return ret;
> > +
> > + return le32_to_cpu(info->can_clk);
> > +}
> > +
> > +static int nct6694_can_probe(struct platform_device *pdev)
> > +{
> > + const struct mfd_cell *cell = mfd_get_cell(pdev);
> > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> > + struct nct6694_can_priv *priv;
> > + struct net_device *ndev;
> > + int ret, irq, can_clk;
> > +
> > + irq = irq_create_mapping(nct6694->domain,
> > + NCT6694_IRQ_CAN1 + cell->id);
> > + if (!irq)
> > + return -EINVAL;
>
> propagate error value
>
Fix it in v4.
> > +
> > + ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> > + if (!ndev)
> > + return -ENOMEM;
> > +
> > + ndev->irq = irq;
> > + ndev->flags |= IFF_ECHO;
> > + ndev->netdev_ops = &nct6694_can_netdev_ops;
> > + ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->nct6694 = nct6694;
> > + priv->ndev = ndev;
> > +
> > + priv->tx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
>
> devm_kzalloc()
> > + if (!priv->tx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + priv->rx_buf = devm_kcalloc(&pdev->dev, NCT6694_MAX_PACKET_SZ,
> > + sizeof(unsigned char), GFP_KERNEL);
> devm_kzalloc()
>
Fix it in v4.
> > + if (!priv->rx_buf) {
> > + ret = -ENOMEM;
> > + goto free_candev;
> > + }
> > +
> > + can_clk = nct6694_can_get_clock(priv);
> > + if (can_clk < 0) {
> > + ret = -EIO;
>
> propagate the error value, don't overwrite it
>
> move the dev_err_probe() here.
>
Fix it in v4.
> > + goto free_candev;
> > + }
> > +
> > + mutex_init(&priv->lock);
> > + INIT_WORK(&priv->tx_work, nct6694_can_tx_work);
> > +
> > + priv->can_idx = cell->id;
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = can_clk;
> > + priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> > + priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> > + priv->can.do_set_mode = nct6694_can_set_mode;
> > + priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> > +
> > + priv->can.ctrlmode = CAN_CTRLMODE_FD;
> > +
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_FD |
> > + CAN_CTRLMODE_FD_NON_ISO |
> > + CAN_CTRLMODE_BERR_REPORTING;
> > +
> > + platform_set_drvdata(pdev, priv);
> > + SET_NETDEV_DEV(priv->ndev, &pdev->dev);
> > +
> > + ret = register_candev(priv->ndev);
> > + if (ret)
> > + goto free_candev;
> > +
> > + return 0;
> > +
> > +free_candev:
> > + free_candev(ndev);
> > + return dev_err_probe(&pdev->dev, ret, "Probe failed\n");
>
> Move the dev_err_probe() with an appropriate error message to where the
> error occurs. If malloc fails, the kernel already prints for you, so
> here it's only nct6694_can_get_clock() only.
>
Understood. I'll make the modifications in the next patch.
> > +}
> > +
> > +static void nct6694_can_remove(struct platform_device *pdev)
> > +{
> > + struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> > +
> > + cancel_work_sync(&priv->tx_work);
> > + unregister_candev(priv->ndev);
> > + free_candev(priv->ndev);
> > +}
> > +
> > +static struct platform_driver nct6694_can_driver = {
> > + .driver = {
> > + .name = DRVNAME,
> > + },
> > + .probe = nct6694_can_probe,
> > + .remove = nct6694_can_remove,
> > +};
> > +
> > +module_platform_driver(nct6694_can_driver);
> > +
> > +MODULE_DESCRIPTION("USB-CAN FD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>");
> > +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
