Hi Peter and Michal,
On Fry. 21 Apr. 2023 at 12:14, Peter Hong <peter_hong@xxxxxxxxxxxxx> wrote:
>
> 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.
Ack. That is why it is good to remove the opaque u8* buffer. It helped
to identify that.
> >> +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 ?
@Michal: You requested RTC in:
https://lore.kernel.org/linux-can/ZBgKSqaFiImtTThv@localhost.localdomain/
I looked at the kernel documentation but I could not find "Reverse
Chistmas Tree". Can you point me to where this is defined?
In the above case, I do not think RCT should apply.
I think that this:
struct net_device *netdev = priv->netdev;
struct net_device_stats *stats = &netdev->stats;
Is better than that:
struct net_device *netdev = priv->netdev;
struct net_device_stats *stats;
stats = &netdev->stats;
Arbitrarily splitting the definition and assignment does not make sense to me.
Thank you for your comments.
> >> +
> >> + /* 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.
I did not fully understand you. Regardless this is a nitpick. If you
are convinced that tx_dropped is the correct way, let it be like that.
Yours sincerely,
Vincent Mailhol
