Re: [PATCH V5] can: usb: f81604: add Fintek F81604 support

From: Peter Hong <peter_hong@fintek.com.tw>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: <wg@grandegger.com>, <mkl@pengutronix.de>,
	<michal.swiatkowski@linux.intel.com>,
	<Steen.Hegelund@microchip.com>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<frank.jungclaus@esd.eu>, <linux-kernel@vger.kernel.org>,
	<linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
	<hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V5] can: usb: f81604: add Fintek F81604 support
Date: Fri, 21 Apr 2023 11:14:17 +0800	[thread overview]
Message-ID: <51991fc1-0746-608f-b3bb-78b64e6d1a3e@fintek.com.tw> (raw)
In-Reply-To: <CAMZ6RqKWrtBMFSD=BzGuCbvj=+3X-A-oW9haJ7=4kyL2AbEuHQ@mail.gmail.com>

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@fintek.com.tw> 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,