Hi Vincent,
Vincent MAILHOL 於 2023/4/20 下午 08:02 寫道:
Hi Peter,
Here are my comments. Now, it is mostly nitpicks. I guess that this is
the final round.
On Thu. 20 avr. 2023 at 11:44, Ji-Ze Hong (Peter Hong)
<peter_hong@xxxxxxxxxxxxx> wrote:
+static void f81604_read_bulk_callback(struct urb *urb)
+{
+ struct f81604_can_frame *frame = urb->transfer_buffer;
+ struct net_device *netdev = urb->context;
+ int ret;
+
+ if (!netif_device_present(netdev))
+ return;
+
+ if (urb->status)
+ netdev_info(netdev, "%s: URB aborted %pe\n", __func__,
+ ERR_PTR(urb->status));
+
+ switch (urb->status) {
+ case 0: /* success */
+ break;
+
+ case -ENOENT:
+ case -EPIPE:
+ case -EPROTO:
+ case -ESHUTDOWN:
+ return;
+
+ default:
+ goto resubmit_urb;
+ }
+
+ if (urb->actual_length != F81604_DATA_SIZE) {
It is more readable to use sizeof() instead of a macro.
if (urb->actual_length != sizeof(*frame)) {
+ netdev_warn(netdev, "URB length %u not equal to %u\n",
+ urb->actual_length, F81604_DATA_SIZE);
Idem.
+ goto resubmit_urb;
+ }
In v4, actual_length was allowed to be any multiple of
F81604_DATA_SIZE and f81604_process_rx_packet() had a loop to iterate
through all the messages.
Why did this disappear in v5?
I had over design it. The F81604 will only report 1 frame at 1 bulk-in,
So I change it to
process 1 frame only.
+static void f81604_handle_tx(struct f81604_port_priv *priv,
+ struct f81604_int_data *data)
+{
+ struct net_device *netdev = priv->netdev;
+ struct net_device_stats *stats;
+
+ stats = &netdev->stats;
Merge the declaration with the initialization.
If I merge initialization into declaration, it's may violation RCT?
How could I change about this ?
+
+ /* transmission buffer released */
+ if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
+ !(data->sr & F81604_SJA1000_SR_TCS)) {
+ stats->tx_errors++;
+ can_free_echo_skb(netdev, 0, NULL);
+ } else {
+ /* transmission complete */
+ stats->tx_bytes += can_get_echo_skb(netdev, 0, NULL);
+ stats->tx_packets++;
+ }
+
+ netif_wake_queue(netdev);
+}
+
+static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv,
+ struct f81604_int_data *data)
+{
+ enum can_state can_state = priv->can.state;
+ struct net_device *netdev = priv->netdev;
+ enum can_state tx_state, rx_state;
+ struct net_device_stats *stats;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+
+ stats = &netdev->stats;
Merge the declaration with the initialization.
Especially, here it is odd that can_state and netdev are initialized
during declaration and that only stats is initialized separately.
idem
+ tx_state = data->txerr >= data->rxerr ? can_state : 0;
+ rx_state = data->txerr <= data->rxerr ? can_state : 0;
+
+ can_change_state(netdev, cf, tx_state, rx_state);
+
+ if (can_state == CAN_STATE_BUS_OFF)
+ can_bus_off(netdev);
+ }
+
+ if (priv->clear_flags)
+ schedule_work(&priv->clear_reg_work);
+
+ if (skb)
+ netif_rx(skb);
+}
+
+static void f81604_read_int_callback(struct urb *urb)
+{
+ struct f81604_int_data *data = urb->transfer_buffer;
+ struct net_device *netdev = urb->context;
+ struct f81604_port_priv *priv;
+ int ret;
+
+ priv = netdev_priv(netdev);
Merge the declaration with the initialization.
idem
+ id = (cf->can_id & CAN_SFF_MASK) << F81604_SFF_SHIFT;
+ put_unaligned_be16(id, &frame->sff.id);
+
+ if (!(cf->can_id & CAN_RTR_FLAG))
+ memcpy(&frame->sff.data, cf->data, cf->len);
+ }
+
+ can_put_echo_skb(skb, netdev, 0, 0);
+
+ ret = usb_submit_urb(write_urb, GFP_ATOMIC);
+ if (ret) {
+ netdev_err(netdev, "%s: failed to resubmit tx bulk urb: %pe\n",
+ __func__, ERR_PTR(ret));
+
+ can_free_echo_skb(netdev, 0, NULL);
+ stats->tx_dropped++;
Stats is only used once. Maybe better to not declare a variable and do:
netdev->stats.tx_dropped++;
Also, more than a drop, this looks like an error. So:
netdev->stats.tx_errors++;
Due to lable nomem_urb and tx_dropped/ tx_errors will not only use once,
so I'll remain it.
Thanks,
