Re: [PATCH v3 4/7] can: Add Nuvoton NCT6694 CAN support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Dear Vincent,

Thank you for your comments,

Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2024年12月11日 週三 下午11:25寫道：
>
...
> > > +/* Host interface */
> > > +#define NCT6694_CAN_MOD                      0x05
> > > +
> > > +/* Message Channel*/
> > > +/* Command 00h */
>
> Instead of this comment, explain what the command 00h does.
>
> Also better to give a memorable name instead of CMD0. For example:
> CMD_TX, CMD_RX…
>
> If possible, make it match the structure names.
>

Okay, I'll make the modifications in the next patch.

> > > +#define NCT6694_CAN_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000)
> > > +#define NCT6694_CAN_CTRL1_MON                BIT(0)
> > > +#define NCT6694_CAN_CTRL1_NISO               BIT(1)
> > > +#define NCT6694_CAN_CTRL1_LBCK               BIT(2)
> > > +
> > > +/* Command 01h */
> > > +#define NCT6694_CAN_CMD1_OFFSET              0x0001
> > > +
> > > +/* Command 02h */
> > > +#define NCT6694_CAN_CMD2_OFFSET(idx, mask)                   \
> > > +     ({ typeof(mask) mask_ = (mask);                         \
> > > +        idx ? ((0x80 | (mask_ & 0xFF)) << 8 | 0x02) :        \
> > > +              ((0x00 | (mask_ & 0xFF)) << 8 | 0x02); })
> > > +
> > > +#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_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)
> > > +
> > > +/* Command 10h */
> > > +#define NCT6694_CAN_CMD10_OFFSET(buf_cnt)    \
> > > +     (((buf_cnt) & 0xFF) << 8 | 0x10)
> > > +#define NCT6694_CAN_TAG_CAN0         0xC0
> > > +#define NCT6694_CAN_TAG_CAN1         0xC1
> > > +#define NCT6694_CAN_FLAG_EFF         BIT(0)
> > > +#define NCT6694_CAN_FLAG_RTR         BIT(1)
> > > +#define NCT6694_CAN_FLAG_FD          BIT(2)
> > > +#define NCT6694_CAN_FLAG_BRS         BIT(3)
> > > +#define NCT6694_CAN_FLAG_ERR         BIT(4)
> > > +
> > > +/* Command 11h */
> > > +#define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt)                       \
> > > +     ({ typeof(buf_cnt) buf_cnt_ = (buf_cnt);                \
> > > +        idx ? ((0x80 | (buf_cnt_ & 0xFF)) << 8 | 0x11) :     \
> > > +              ((0x00 | (buf_cnt_ & 0xFF)) << 8 | 0x11); })
>
> Simplify this. Do something like:
>
>   #define NCT6694_CAN_CMD11_OFFSET(idx, buf_cnt) \
>           (idx ? 0x80 : 0x00) | \
>           (buf_cnt & 0xFF)) << 8 | 0x11) \
>
> (apply this also to NCT6694_CAN_CMD2_OFFSET())
>

Understood! I will fix these in v4.

> > > +#define NCT6694_CAN_RX_QUOTA         64
> > > +
> > > +enum nct6694_event_err {
> > > +     NCT6694_CAN_EVT_NO_ERROR,
> >                       ^^^ add _ERR_
> > > +     NCT6694_CAN_EVT_CRC_ERROR,
> > > +     NCT6694_CAN_EVT_STUFF_ERROR,
> > > +     NCT6694_CAN_EVT_ACK_ERROR,
> > > +     NCT6694_CAN_EVT_FORM_ERROR,
> > > +     NCT6694_CAN_EVT_BIT_ERROR,
> > > +     NCT6694_CAN_EVT_TIMEOUT_ERROR,
> > > +     NCT6694_CAN_EVT_UNKNOWN_ERROR,
> > > +};
> > > +
> > > +enum nct6694_event_status {
> > > +     NCT6694_CAN_EVT_ERROR_ACTIVE,
> >                       ^^^ add _STATUS_
> > > +     NCT6694_CAN_EVT_ERROR_PASSIVE,
> > > +     NCT6694_CAN_EVT_BUS_OFF,
> > > +     NCT6694_CAN_EVT_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;
> > > +     __le16 reserved;
> > u8 reserved[2];
> > > +     __le32 can_clk;
> > > +};
> > > +
> > > +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.
> >
> > > +
> > > +struct __packed nct6694_can_xmit {
>
> Is this struct for both TX and RX? If so, name it something like
>
>   struct nct6694_can_frame
>
> The term xmit is only used for transmission, not for reception.
>

Okay, I'll make the modifications in the next patch.

> > > +     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?
> >
> > > +};
> > > +
> > > +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?
>
> +1
>
> mutexes are good if you want to keep the lock for a long time.
>
> For short period, spinlock are more performant:
>
>           spinlock_t lock;
>

The lock ensures that data accessed by nct6694_read_msg() and
nct6694_write_msg() is not overwritten.

Since nct6694_read_msg() and nct6694_write_msg() use usb_bulk_msg(),
which may cause the process  to sleep, spinlock is not used.

> > > +     struct sk_buff *tx_skb;
> > > +     struct workqueue_struct *wq;
> > > +     struct work_struct tx_work;
> > > +     unsigned char *tx_buf;
> > void *
> > > +     unsigned char *rx_buf;
> > void *
>
> Rather than void*, tx_buf and rx_buf can be replaced by an union:
>
>   union nct6694_can_rx {
>           struct nct6694_can_event event;
>           struct nct6694_can_xmit xmit;
>           struct nct6694_can_information info;
>   };
>
> (same for nct6694_can_tx)
>
> Then in struct nct6694_can_priv, you will just have:
>
>   union nct6694_can_tx tx;
>   union nct6694_can_rx rx;
>
> With this,
>
>   NCT6694_MAX_PACKET_SZ
>
> can most likely be replaced by sizeof(union nct6694_can_rx) or
> sizeof(union nct6694_can_tx).
>

Okay, I'll make the modifications in the next patch.

> > > +     unsigned char can_idx;
> > > +     bool tx_busy;
> >
...
> > > +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
> > > +
> > > +     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?
>
> +1
>
> Also, do not trust the device data. Even if SPI attacks are less
> common, make sure to sanitize this length.
>
>   cf->len = canfd_sanitize_len(xmit->dlc);
>
> Or
>
>   cf->len = canfd_sanitize_len(xmit->dlc);
>
> if xmit->dlc is in fact a DLC.
>

Excuse me, the xmit->dlc is actual data length.
Does it need to be fixed?

> > > +
...
> > > +static int nct6694_can_handle_lec_err(struct net_device *ndev, u8 bus_err)
> > > +{
> > > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > +     struct net_device_stats *stats = &ndev->stats;
> > > +     struct can_frame *cf;
> > > +     struct sk_buff *skb;
> > > +
> > > +     if (bus_err == NCT6694_CAN_EVT_NO_ERROR)
> > > +             return 0;
> > > +
> > > +     priv->can.can_stats.bus_error++;
> > > +     stats->rx_errors++;
> > > +
> > > +     /* Propagate the error condition to the CAN stack. */
> > > +     skb = alloc_can_err_skb(ndev, &cf);
> > > +
> > > +     if (unlikely(!skb))
> > > +             return 0;
>
> Do not exit if the memory allocation fails. Instead, do the error
> handling to increase the statistics.
>
> Look at what other CAN drivers are doing.
>

Okay, I'll make the modifications in the next patch.

> > > +     /* Read the error counter register and check for new errors. */
> > > +     cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > > +
> > > +     switch (bus_err) {
> > > +     case NCT6694_CAN_EVT_CRC_ERROR:
> > > +             cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_STUFF_ERROR:
> > > +             cf->data[2] |= CAN_ERR_PROT_STUFF;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_ACK_ERROR:
> > > +             cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_FORM_ERROR:
> > > +             cf->data[2] |= CAN_ERR_PROT_FORM;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_BIT_ERROR:
> > > +             cf->data[2] |= CAN_ERR_PROT_BIT |
> > > +                            CAN_ERR_PROT_BIT0 |
> > > +                            CAN_ERR_PROT_BIT1;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_TIMEOUT_ERROR:
> > > +             cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > > +             break;
> > > +
> > > +     case NCT6694_CAN_EVT_UNKNOWN_ERROR:
> > > +             cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > > +             /*
> > > +              * It means 'unspecified'(the value is '0').
> > > +              * But it is not sure if it's ok to send an error package
> > > +              * without specific error bit.
> > > +              */
> > > +             break;
> > > +
> > > +     default:
> > > +             break;
> > > +     }
> > > +
> > > +     /* Reset the error counter, ack the IRQ and re-enable the counter. */
> > > +     stats->rx_packets++;
> > > +     stats->rx_bytes += cf->can_dlc;
>
> The CAN error frames are not regular packets. Do not increase the RX
> stats. Instead, increase the error stats.
>

Okay, I'll make the modifications in the next patch.

> > > +     netif_receive_skb(skb);
> > > +
> > > +     return 1;
> > > +}
> > > +
> > > +static int nct6694_can_handle_state_change(struct net_device *ndev,
> > > +                                        enum can_state new_state)
> > > +{
> > > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > +     struct net_device_stats *stats = &ndev->stats;
> > > +     struct can_frame *cf;
> > > +     struct sk_buff *skb;
> > > +     struct can_berr_counter bec;
> > > +
> > > +     switch (new_state) {
> > > +     case CAN_STATE_ERROR_ACTIVE:
> > > +             priv->can.can_stats.error_warning++;
> > > +             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > > +             break;
> > > +     case CAN_STATE_ERROR_WARNING:
> > > +             priv->can.can_stats.error_warning++;
> > > +             priv->can.state = CAN_STATE_ERROR_WARNING;
> > > +             break;
> > > +     case CAN_STATE_ERROR_PASSIVE:
> > > +             priv->can.can_stats.error_passive++;
> > > +             priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > > +             break;
> > > +     case CAN_STATE_BUS_OFF:
> > > +             priv->can.state = CAN_STATE_BUS_OFF;
> > > +             priv->can.can_stats.bus_off++;
> > > +             can_bus_off(ndev);
> > > +             break;
> > > +     default:
> > > +             break;
> > > +     }
> > > +
> > > +     /* propagate the error condition to the CAN stack */
> > > +     skb = alloc_can_err_skb(ndev, &cf);
> > > +     if (unlikely(!skb))
> > > +             return 0;
>
> Same as above: handle the statistics even if the allocation fails.
>

Okay, I'll make the modifications in the next patch.

> > > +     nct6694_can_get_berr_counter(ndev, &bec);
> > > +
> > > +     switch (new_state) {
> > > +     case CAN_STATE_ERROR_WARNING:
> > > +             /* error warning state */
> > > +             cf->can_id |= CAN_ERR_CRTL;
> > > +             cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
> > > +                                                     CAN_ERR_CRTL_RX_WARNING;
>
> Prefer an if/else here instead of the ternary operator. It is more readable.
>

Fix it in v4.

> > > +             cf->data[6] = bec.txerr;
> > > +             cf->data[7] = bec.rxerr;
> > > +             break;
> > > +     case CAN_STATE_ERROR_PASSIVE:
> > > +             /* error passive state */
> > > +             cf->can_id |= CAN_ERR_CRTL;
> > > +             cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > > +             if (bec.txerr > 127)
> > > +                     cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > > +             cf->data[6] = bec.txerr;
> > > +             cf->data[7] = bec.rxerr;
> > > +             break;
> > > +     case CAN_STATE_BUS_OFF:
> > > +             /* bus-off state */
> > > +             cf->can_id |= CAN_ERR_BUSOFF;
> > > +             break;
> > > +     default:
> > > +             break;
> > > +     }
> > > +
> > > +     stats->rx_packets++;
> > > +     stats->rx_bytes += cf->can_dlc;
>
> Do not increase the RX stats when you receive a CAN event.
>

Okay, I'll make the modifications in the next patch.

> > > +     netif_receive_skb(skb);
> > > +
> > > +     return 1;
> > > +}
...
> > > +static int nct6694_can_start(struct net_device *ndev)
> > > +{
> > > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > > +     struct nct6694_can_setting *setting = (struct nct6694_can_setting *)priv->tx_buf;
> > > +     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(priv->tx_buf, 0, sizeof(struct nct6694_can_setting));
>
> When you memset(), use this pattern:
>
>           memset(setting, 0, sizeof(*setting));
>
> Apply this throughout your driver.
>

Okay, I'll make the modifications in the next patch.

> > > +     setting->nbr = cpu_to_le32(n_bt->bitrate);
> > > +     setting->dbr = cpu_to_le32(d_bt->bitrate);
> > > +
...
> > > +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");
>
>
> Yours sincerely,
> Vincent Mailhol

Best regards,
Ming
[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux