Dear Vincent,
Thank you for your comments,
Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2024年12月27日 週五 下午11:59寫道：
>
> > +
> > +struct __packed nct6694_can_event_channel {
> > + u8 err;
> > + u8 status;
> > + u8 tx_evt;
> > + u8 rx_evt;
> > + u8 rec;
> > + u8 tec;
> > + u8 reserved[2];
> > +};
> > +
> > +struct __packed nct6694_can_event {
> > + struct nct6694_can_event_channel evt_ch[2];
> > +};
>
> Remove this intermediate struct...
>
> > +struct __packed nct6694_can_frame {
> > + u8 tag;
> > + u8 flag;
> > + u8 reserved;
> > + u8 length;
> > + __le32 id;
> > + u8 data[64];
> > +};
> > +
> > +union nct6694_can_tx {
> > + struct nct6694_can_frame frame;
> > + struct nct6694_can_setting setting;
> > +};
> > +
> > +union nct6694_can_rx {
> > + struct nct6694_can_event event;
>
> ... and instead, dircectly declare the array here:
>
> struct nct6694_can_event event[2];
>
Okay! Fix it in v5.
> > + struct nct6694_can_frame frame;
> > + struct nct6694_can_information info;
> > +};
> > +
> > +struct nct6694_can_priv {
> > + struct can_priv can; /* must be the first member */
> > + struct can_rx_offload offload;
> > + struct net_device *ndev;
> > + struct nct6694 *nct6694;
> > + struct mutex lock;
> > + struct sk_buff *tx_skb;
> > + struct workqueue_struct *wq;
> > + struct work_struct tx_work;
> > + union nct6694_can_tx *tx;
> > + union nct6694_can_rx *rx;
> > + unsigned char can_idx;
> > + bool tx_busy;
>
> tx_busy is only set when the network queue is stopped, right? Isn't it
> possible to use netif_tx_queue_stopped() instead of tx_busy?
>
Yes, I'll make the modification in v5.
> > +};
> > +
...
> > +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);
> > +
> > + if (status & NCT6694_CAN_EVT_STS_BUS_OFF)
> > + new_state = CAN_STATE_BUS_OFF;
> > + else if (status & NCT6694_CAN_EVT_STS_ERROR_PASSIVE)
> > + new_state = CAN_STATE_ERROR_PASSIVE;
> > + else if (status & NCT6694_CAN_EVT_STS_WARNING)
> > + new_state = CAN_STATE_ERROR_WARNING;
The procedure for handling state changes is incorrect. I'll fix it in
the next patch.
(e.g.)
switch (status) {
case NCT6694_CAN_EVT_STS_BUS_OFF):
new_state = CAN_STATE_BUS_OFFF;
break;
...
}
> > +
> > + /* 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;
> > + can_change_state(ndev, cf, tx_state, rx_state);
> > +
> > + if (new_state == CAN_STATE_BUS_OFF) {
> > + 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 void nct6694_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);
> > + 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;
> > + }
>
> Aren't you missing something here? You are populating a can frame but
> you are returning without using it.
>
Sorry for forgetting to process rx offload function, I'll add
nct6694_can_rx_offload() here in the v5.
> > +}
> > +
...
> > +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 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)) {
> > + 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 {
> > + 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 the payload if the frame is a remote frame.
>
Fix it in the v5.
> > + }
> ^^^^^^^^
>
> Bad indentation. Did you run script/checkpatch.pl before sending?
>
I already ran the script, but I'm not sure why it didn't report this error.
I'll fix it in the next patch.
> > + err = nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
> > + NCT6694_CAN_DELIVER(1),
> > + sizeof(*frame),
> > + 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_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + nct6694_can_clean(ndev);
> > + nct6694_can_start(ndev);
> > + netif_wake_queue(ndev);
> > + break;
>
> Nitpick: here, directly return 0...
>
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
>
> ... and then remove that line.
>
Fix it in v5.
> > +}
> > +
Best regards,
Ming
