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

From: Peter Hong
Date: Mon Mar 27 2023 - 23:19:58 EST


Hi Vincent,

Vincent MAILHOL 於 2023/3/27 下午 06:27 寫道:
eff->id is a 32 bit value. It is not aligned. So, you must always use
{get|set}_unaligned_be32() to manipulate this value.
N.B. on x86 architecture, unaligned access is fine, but some other
architecture may throw a fault. Read this for more details:

https://docs.kernel.org/arm/mem_alignment.html

for the consistency of the code, could I also add get/put_unaligned_be16 in SFF
sections ?

+static int f81604_set_reset_mode(struct net_device *netdev)
+{
+ struct f81604_port_priv *priv = netdev_priv(netdev);
+ int status, i;
+ u8 tmp;
+
+ /* disable interrupts */
+ status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
+ SJA1000_IER, IRQ_OFF);
+ if (status)
+ return status;
+
+ for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
Thanks for removing F81604_USB_MAX_RETRY.

Yet, I still would like to understand why you need one hundred tries?
Is this some paranoiac safenet? Or does the device really need so many
attempts to operate reliably? If those are needed, I would like to
understand the root cause.

This section is copy from sja1000.c. In my test, the operation/reset may retry 1 times.
I'll reduce it from 100 to 10 times.


+ int status, len;
+
+ if (can_dropped_invalid_skb(netdev, skb))
+ return NETDEV_TX_OK;
+
+ netif_stop_queue(netdev);
In your driver, you send the CAN frames one at a time and wait for the
rx_handler to restart the queue. This approach dramatically degrades
the throughput. Is this a device limitation? Is the device not able to
manage more than one frame at a time?


This device will not NAK on TX frame not complete, it only NAK on TX endpoint
memory not processed, so we'll send next frame unitl TX complete(TI) interrupt
received.

The device can polling status register via TX/RX endpoint, but it's more complex.
We'll plan to do it when first driver landing in mainstream.

+static int f81604_set_termination(struct net_device *netdev, u16 term)
+{
+ struct f81604_port_priv *port_priv = netdev_priv(netdev);
+ struct f81604_priv *priv;
+ u8 mask, data = 0;
+ int r;
+
+ priv = usb_get_intfdata(port_priv->intf);
+
+ if (netdev->dev_id == 0)
+ mask = F81604_CAN0_TERM;
+ else
+ mask = F81604_CAN1_TERM;
+
+ if (term == F81604_TERMINATION_ENABLED)
+ data = mask;
+
+ mutex_lock(&priv->mutex);
Did you witness a race condition?

As far as I know, this call back is only called while the network
stack big kernel lock (a.k.a. rtnl_lock) is being hold.
If you have doubt, try adding a:

ASSERT_RTNL()

If this assert works, then another mutex is not needed.

It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
in f81604_probe() -> f81604_set_termination(), not called via ip command:
    ip link set dev can0 type can termination 120
    ip link set dev can0 type can termination 0

so I'll still use mutex on here.

+ port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
+ port_priv->can.ctrlmode_supported =
+ CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
+ CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
that you can send and receive DLC greater than 8?

Sorry, I had misunderstand the define. This device is only support 0~8 data length,
so I'll remove CAN_CTRLMODE_CC_LEN8_DLC in future patch.

Thanks,