Re: [PATCH v13 01/11] can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces
From: Vincent MAILHOL <hidden>
Date: 2021-03-20 11:35:41
On Fri. 19 Mar 2021 at 23:57, Marc Kleine-Budde [off-list ref] wrote:
On 3/19/21 1:41 PM, Marc Kleine-Budde wrote:quoted
From: Vincent Mailhol <redacted> This patch adds the core support for various USB CAN interfaces from ETAS GmbH (https://www.etas.com/en/products/es58x.php). The next patches add the glue code drivers for the individual interfaces. Co-developed-by: Arunachalam Santhanam <redacted> Signed-off-by: Arunachalam Santhanam <redacted> Signed-off-by: Vincent Mailhol <redacted> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> ---[...]quoted
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c new file mode 100644 index 000000000000..f2dc943bb0b2 --- /dev/null +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c[...]quoted
+/** + * es58x_timestamp_to_ns() - Convert a timestamp value received from a + * ES58X device to nanoseconds. + * @timestamp: Timestamp received from a ES58X device. + * + * The timestamp received from ES58X is expressed in multiple of 0.5 + * micro seconds. This function converts it in to nanoseconds. + * + * Return: Timestamp value in nanoseconds. + */ +static u64 es58x_timestamp_to_ns(u64 timestamp) +{ + const u64 es58x_timestamp_ns_mult_coef = 500ULL; + + return es58x_timestamp_ns_mult_coef * timestamp; +} + +/** + * es58x_set_skb_timestamp() - Set the hardware timestamp of an skb. + * @netdev: CAN network device. + * @skb: socket buffer of a CAN message. + * @timestamp: Timestamp received from an ES58X device. + * + * Used for both received and loopback messages. + * + * Return: zero on success, -EFAULT if @skb is NULL.That's not correct. Please make sure to call it with skb != NULL and make it a void function.
Ack. All calls to that function already checked that the skb was not NULL. I will make the function void.
Does it make sense for you to use of struct cyclecounter/struct timecounter. Have a look at: https://lore.kernel.org/linux-can/20210304160328.2752293-6-mkl@pengutronix.de/ (local) As your device already provides a u64 timestamp you don't need the worker.
I saw your patch but because my device already uses a 64 bit counter, I did not see a benefit to do so. Which problem would the struct cyclecounter/struct timecounter solve for my driver?
quoted
+ */ +static int es58x_set_skb_timestamp(struct net_device *netdev, + struct sk_buff *skb, u64 timestamp) +{ + struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev; + struct skb_shared_hwtstamps *hwts; + + hwts = skb_hwtstamps(skb); + /* Ignoring overflow (overflow on 64 bits timestamp with nano + * second precision would occur after more than 500 years). + */ + hwts->hwtstamp = ns_to_ktime(es58x_timestamp_to_ns(timestamp) + + es58x_dev->realtime_diff_ns); + + return 0; +} + +/** + * es58x_rx_timestamp() - Handle a received timestamp. + * @es58x_dev: ES58X device. + * @timestamp: Timestamp received from a ES58X device. + * + * Calculate the difference between the ES58X device and the kernel + * internal clocks. This difference will be later used as an offset to + * convert the timestamps of RX and loopback messages to match the + * kernel system time (e.g. convert to UNIX time).I'm not sure if we want to have the timestamp based on the kernel system time. The problem I see is that your clock is not synchronized to your system clock and will probably drift, so you might accumulate an offset. When looking at candump output, a timestamp that more or less current time gives a false sense of correctness, but if the timestamp is short after 01.0.1970 you don't expect it to be correct. But this is a different problem.....
Here, we are discussing tastes and colors. I am perfectly aware that the two quartz used by the device and the kernel are not in sync and will drift away from each other. However, when looking at my traces, I like to have a hint of when an event occurs. In the worst case, the quartz might drift away up to four seconds a day, but the date, the hours and the minutes stay accurate. So it is a discussion of making it convenient versus making it dummy proof. If there is a strong consensus not to base the hardware timestamp on the kernel, then I will remove it. But if I have the choice, I prefer to keep it as it is.
quoted
+ * + * Return: zero on success. + */ +int es58x_rx_timestamp(struct es58x_device *es58x_dev, u64 timestamp)voidquoted
+{ + u64 ktime_real_ns = ktime_get_real_ns(); + u64 device_timestamp = es58x_timestamp_to_ns(timestamp); + + dev_dbg(es58x_dev->dev, "%s: request round-trip time: %llu ns\n", + __func__, ktime_real_ns - es58x_dev->ktime_req_ns); + + es58x_dev->realtime_diff_ns = + (es58x_dev->ktime_req_ns + ktime_real_ns) / 2 - device_timestamp; + es58x_dev->ktime_req_ns = 0; + + dev_dbg(es58x_dev->dev, + "%s: Device timestamp: %llu, diff with kernel: %llu\n", + __func__, device_timestamp, es58x_dev->realtime_diff_ns); + + return 0; +} + +/** + * es58x_set_realtime_diff_ns() - Calculate difference between the + * clocks of the ES58X device and the kernel + * @es58x_dev: ES58X device. + * + * Request a timestamp from the ES58X device. Once the answer is + * received, the timestamp difference will be set by the callback + * function es58x_rx_timestamp(). + * + * Return: zero on success, errno when any error occurs. + */ +static int es58x_set_realtime_diff_ns(struct es58x_device *es58x_dev) +{ + if (es58x_dev->ktime_req_ns) { + dev_warn(es58x_dev->dev, + "%s: Previous request to set timestamp has not completed yet\n", + __func__); + return -EBUSY;The caller ignores the return value.
ACK. Now the caller will check the return value.
quoted
+ } + + es58x_dev->ktime_req_ns = ktime_get_real_ns(); + return es58x_dev->ops->get_timestamp(es58x_dev); +} + +/** + * es58x_is_can_state_active() - Is the network device in an active + * CAN state? + * @netdev: CAN network device. + * + * The device is considered active if it is able to send or receive + * CAN frames, that is to say if it is in any of + * CAN_STATE_ERROR_ACTIVE, CAN_STATE_ERROR_WARNING or + * CAN_STATE_ERROR_PASSIVE states. + * + * Caution: when recovering from a bus-off, + * net/core/dev.c#can_restart() will call + * net/core/dev.c#can_flush_echo_skb() without using any kind of + * locks. For this reason, it is critical to guaranty that no TX or^^^^^^^^ guarantee
ACK.
quoted
+ * loopback operations (i.e. any access to priv->echo_skb[]) can be + * done while this function is returning false. + * + * Return: true if the device is active, else returns false. + */ +static bool es58x_is_can_state_active(struct net_device *netdev) +{ + return es58x_priv(netdev)->can.state < CAN_STATE_BUS_OFF; +} + +/** + * es58x_is_echo_skb_threshold_reached() - Determine the limit of how + * many skb slots can be taken before we should stop the network + * queue. + * @priv: ES58X private parameters related to the network device. + * + * We need to save enough free skb slots in order to be able to do + * bulk send. This function can be used to determine when to wake or + * stop the network queue in regard to the number of skb slots already + * taken if the loopback FIFO. + * + * Return: boolean. + */ +static bool es58x_is_echo_skb_threshold_reached(struct es58x_priv *priv) +{ + unsigned long num_echo_skb = priv->tx_head - priv->tx_tail; + unsigned long threshold = priv->can.echo_skb_max - + priv->es58x_dev->param->tx_bulk_max + 1;you're using u32 for tx_head and tail, no need for unsinged long here. better make all unsigned int.
I made those two u32.
quoted
+ + return num_echo_skb >= threshold; +} + +/** + * es58x_add_skb_idx() - Increment an index of the loopback FIFO. + * @priv: ES58X private parameters related to the network device. + * @idx: address of the index to be incremented. + * @a: the increment. Must be positive and less or equal to + * @priv->can.echo_skb_max. + * + * Do a modulus addition: set *@idx to (*@idx + @a) % + * @priv->can.echo_skb_max. + * + * Rationale: the modulus operator % takes a decent amount of CPU + * cycles (c.f. other division functions such as + * include/linux/math64.h:iter_div_u64_rem()). + */ +static __always_inline void es58x_add_skb_idx(struct es58x_priv *priv, + u16 *idx, u16 a)unused?
Yes, Jimmy also pointed that issue. I will remove it.
quoted
+{ + *idx += a; + if (*idx >= priv->can.echo_skb_max) + *idx -= priv->can.echo_skb_max; +} + +/** + * es58x_can_free_echo_skb_tail() - Remove the oldest echo skb of the + * loopback FIFO. + * @netdev: CAN network device. + * + * Naming convention: the tail is the beginning of the FIFO, i.e. the + * first skb to have entered the FIFO. + */ +static void es58x_can_free_echo_skb_tail(struct net_device *netdev) +{ + struct es58x_priv *priv = es58x_priv(netdev); + u16 fifo_mask = priv->es58x_dev->param->fifo_mask; + struct sk_buff *skb = priv->can.echo_skb[priv->tx_tail & fifo_mask]; + + netdev_completed_queue(netdev, 1, can_skb_prv(skb)->frame_len); + can_free_echo_skb(netdev, priv->tx_tail & fifo_mask);Plese use can_free_echo_skb() from: https://lore.kernel.org/r/20210319142700.305648-1-mkl@pengutronix.de (local)
ACK.
quoted
+ + priv->tx_tail++; + + netdev->stats.tx_dropped++; +} + +/** + * es58x_can_get_echo_skb_recovery() - Try to re-sync the loopback FIFO. + * @netdev: CAN network device. + * @rcv_packet_idx: Index + * + * This function should not be called under normal circumstances. In + * the unlikely case that one or several URB packages get dropped by + * the device, the index will get out of sync. Try to recover by + * dropping the echo skb packets with older indexes. + * + * Return: zero if recovery was successful, -EINVAL otherwise. + */ +static int es58x_can_get_echo_skb_recovery(struct net_device *netdev, + u32 rcv_packet_idx) +{ + struct es58x_priv *priv = es58x_priv(netdev); + int ret = 0; + + netdev->stats.tx_errors++; + + if (net_ratelimit()) + netdev_warn(netdev, + "Bad loopback packet index: %u. First index: %u, end index %u, num_echo_skb: %02u/%02u\n", + rcv_packet_idx, priv->tx_tail, priv->tx_head, + priv->tx_head - priv->tx_tail, + priv->can.echo_skb_max); + + if (rcv_packet_idx < priv->tx_tail) {This doesn't look wrap around safe. I think you have to do the subtract-cast-to-signed trick here, too.
ACK. If a wrap around occurs, the next if catches it, but then the wrong warning message is displayed. I will make this also wrap around safe.
quoted
+ if (net_ratelimit()) + netdev_warn(netdev, + "Received loopback is from the past. Ignoring it\n"); + ret = -EINVAL; + } else if ((s32)(rcv_packet_idx - priv->tx_head) >= 0LL) {(s32) is not LL, I think it simple 0 is ok here.
Technically, the int type is only guaranteed to be 16 bits. 0LL is definitely overkill. 0L would be the best if we stick to the C standard. But in the kernel, I think that it is implied that integers are 32 bits at least so your suggestion of simply putting 0 is fine :)
quoted
+ if (net_ratelimit()) + netdev_err(netdev, + "Received loopback is from the future. Ignoring it\n"); + ret = -EINVAL; + } else { + if (net_ratelimit()) + netdev_warn(netdev, + "Loopback recovery: dropping %u echo skb from index %u to %u\n", + rcv_packet_idx - priv->tx_tail, + priv->tx_tail, rcv_packet_idx - 1); + while (priv->tx_tail != rcv_packet_idx) { + if (priv->tx_tail == priv->tx_head) + return -EINVAL; + es58x_can_free_echo_skb_tail(netdev); + } + } + return ret; +} + +/** + * es58x_can_get_echo_skb() - Get the skb from the loopback FIFO and + * loop it back locally. + * @netdev: CAN network device. + * @rcv_packet_idx: Index of the first packet received from the device. + * @tstamps: Array of hardware timestamps received from a ES58X device. + * @pkts: Number of packets (and so, length of @tstamps). + * + * Callback function for when we receive a self reception acknowledgment. + * Retrieves the skb from the loopback FIFO, sets its hardware timestamp + * (the actual time it was sent) and loops it back locally. + * + * The device has to be active (i.e. network interface UP and not in + * bus off state or restarting). + * + * Packet indexes must be consecutive (i.e. index of first packet is + * @rcv_packet_idx, index of second packet is @rcv_packet_idx + 1 and + * index of last packet is @rcv_packet_idx + @pkts - 1). + * + * Return: zero on success. + */ +int es58x_can_get_echo_skb(struct net_device *netdev, u32 rcv_packet_idx, + u64 *tstamps, unsigned int pkts) +{ + struct es58x_priv *priv = es58x_priv(netdev); + unsigned int rx_total_frame_len = 0; + unsigned int num_echo_skb = priv->tx_head - priv->tx_tail; + int i; + int ret = 0; + u16 fifo_mask = priv->es58x_dev->param->fifo_mask; + + if (!netif_running(netdev)) { + if (net_ratelimit()) + netdev_info(netdev, + "%s: %s is down, dropping %d loopback packets\n", + __func__, netdev->name, pkts); + netdev->stats.tx_dropped++; + return 0; + } else if (!es58x_is_can_state_active(netdev)) { + if (net_ratelimit()) + netdev_dbg(netdev, + "Bus is off or device is restarting. Ignoring %u loopback packets from index %u\n", + pkts, rcv_packet_idx); + /* stats.tx_dropped will be (or was already) + * incremented by + * drivers/net/can/net/dev.c:can_flush_echo_skb(). + */ + return 0; + } else if (num_echo_skb == 0) { + if (net_ratelimit()) + netdev_warn(netdev, + "Received %u loopback packets from index: %u but echo skb queue is empty.\n", + pkts, rcv_packet_idx); + netdev->stats.tx_dropped += pkts; + return 0; + } + + if (priv->tx_tail != rcv_packet_idx) { + ret = es58x_can_get_echo_skb_recovery(netdev, rcv_packet_idx); + if (ret < 0) { + if (net_ratelimit()) + netdev_warn(netdev, + "Could not find echo skb for loopback packet index: %u\n", + rcv_packet_idx); + return 0; + } + } + if (num_echo_skb < pkts) { + int pkts_drop = pkts - num_echo_skb; + + if (net_ratelimit()) + netdev_err(netdev, + "Received %u loopback packets but have only %d echo skb. Dropping %d echo skb\n", + pkts, num_echo_skb, pkts_drop); + netdev->stats.tx_dropped += pkts_drop; + pkts -= pkts_drop; + } + + for (i = 0; i < pkts; i++) { + unsigned int skb_idx = priv->tx_tail & fifo_mask; + struct sk_buff *skb = priv->can.echo_skb[skb_idx]; + unsigned int frame_len = 0; + + if (skb) + es58x_set_skb_timestamp(netdev, skb, tstamps[i]);I think we need a nice funtion to set the timestamp of a TX echo skb. But that's a different patch. :D
Yes, I had the same thing in mind. But the next thing on my todo list is the TDC.
quoted
+ + netdev->stats.tx_bytes += can_get_echo_skb(netdev, skb_idx, + &frame_len); + rx_total_frame_len += frame_len; + + priv->tx_tail++; + } + + netdev_completed_queue(netdev, pkts, rx_total_frame_len); + netdev->stats.tx_packets += pkts; + + priv->err_passive_before_rtx_success = 0; + if (!es58x_is_echo_skb_threshold_reached(priv)) + netif_wake_queue(netdev); + + return ret; +} + +/** + * es58x_can_flush_echo_skb() - Reset the loopback FIFO. + * @netdev: CAN network device. + * + * The echo_skb array of struct can_priv will be flushed by + * drivers/net/can/dev.c:can_flush_echo_skb(). This function resets + * the parameters of the struct es58x_priv of our device and reset the + * queue (c.f. BQL). + */ +static void es58x_can_flush_echo_skb(struct net_device *netdev) +{ + struct es58x_priv *priv = es58x_priv(netdev); + + priv->tx_tail = 0; + priv->tx_head = 0; + netdev_reset_queue(netdev); +} + +/** + * es58x_flush_pending_tx_msg() - Reset the buffer for transmission messages. + * @netdev: CAN network device. + * + * es58x_start_xmit() will queue up to tx_bulk_max messages in + * &tx_urb buffer and do a bulk send of all messages in one single URB + * (c.f. xmit_more flag). When the device recovers from a bus off + * state or when the device stops, the tx_urb buffer might still have + * pending messages in it and thus need to be flushed. + */ +static void es58x_flush_pending_tx_msg(struct net_device *netdev) +{ + struct es58x_priv *priv = es58x_priv(netdev); + struct es58x_device *es58x_dev = priv->es58x_dev; + int i; + u16 fifo_mask = priv->es58x_dev->param->fifo_mask; + + if (priv->tx_urb) { + netdev_warn(netdev, "%s: dropping %d TX messages\n", + __func__, priv->tx_can_msg_cnt); + netdev->stats.tx_dropped += priv->tx_can_msg_cnt; + for (i = priv->tx_head; + i < priv->tx_head + priv->tx_can_msg_cnt; i++) + can_free_echo_skb(netdev, i & fifo_mask); + + usb_anchor_urb(priv->tx_urb, &priv->es58x_dev->tx_urbs_idle); + atomic_inc(&es58x_dev->tx_urbs_idle_cnt); + usb_free_urb(priv->tx_urb); + } + priv->tx_urb = NULL; +} + +/** + * es58x_tx_ack_msg() - Handle acknowledgment messages. + * @netdev: CAN network device. + * @tx_free_entries: Number of free entries in the device transmit FIFO. + * @rx_cmd_ret_u32: error code as returned by the ES58X device. + * + * ES58X sends an acknowledgment message after a transmission request + * is done. This is mandatory for the ES581.4 but is optional (and + * deactivated in this driver) for the ES58X_FD family. + * + * Under normal circumstances, this function should never throw an + * error message. + * + * Return: zero on success, errno when any error occurs. + */ +int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries, + enum es58x_ret_u32 rx_cmd_ret_u32) +{ + struct es58x_priv *priv = es58x_priv(netdev); + + if (tx_free_entries <= priv->es58x_dev->param->tx_bulk_max) { + if (net_ratelimit()) + netdev_err(netdev, + "Only %d entries left in device queue, num_echo_skb: %d/%d\n", + tx_free_entries, + priv->tx_head - priv->tx_tail, + priv->can.echo_skb_max); + netif_stop_queue(netdev); + } + + return es58x_rx_cmd_ret_u32(netdev, ES58X_RET_TYPE_TX_MSG, + rx_cmd_ret_u32); +} + +/** + * es58x_rx_can_msg() - Handle a received a CAN message. + * @netdev: CAN network device. + * @timestamp: Hardware time stamp (only relevant in rx branches). + * @data: CAN payload. + * @can_id: CAN ID. + * @es58x_flags: Please refer to enum es58x_flag. + * @dlc: Data Length Code (raw value). + * + * Fill up a CAN skb and post it. + * + * This function handles the case where the DLC of a classical CAN + * frame is greater than CAN_MAX_DLEN (c.f. the len8_dlc field of + * struct can_frame). + * + * Return: zero on success. + */ +int es58x_rx_can_msg(struct net_device *netdev, u64 timestamp, const u8 *data, + canid_t can_id, enum es58x_flag es58x_flags, u8 dlc) +{ + struct canfd_frame *cfd; + struct can_frame *ccf; + struct sk_buff *skb; + u8 len; + bool is_can_fd = !!(es58x_flags & ES58X_FLAG_FD_DATA); + + if (dlc > CAN_MAX_RAW_DLC) { + netdev_err(netdev, + "%s: DLC is %d but maximum should be %d\n", + __func__, dlc, CAN_MAX_RAW_DLC); + return -EMSGSIZE; + } + + if (is_can_fd) { + len = can_fd_dlc2len(dlc); + skb = alloc_canfd_skb(netdev, &cfd); + } else { + len = can_cc_dlc2len(dlc); + skb = alloc_can_skb(netdev, &ccf); + cfd = (struct canfd_frame *)ccf; + } + + if (!skb) { + netdev->stats.rx_dropped++; + return -ENOMEM; + } + cfd->can_id = can_id; + if (es58x_flags & ES58X_FLAG_EFF) + cfd->can_id |= CAN_EFF_FLAG; + if (is_can_fd) { + cfd->len = len; + if (es58x_flags & ES58X_FLAG_FD_BRS) + cfd->flags |= CANFD_BRS; + if (es58x_flags & ES58X_FLAG_FD_ESI) + cfd->flags |= CANFD_ESI; + } else { + can_frame_set_cc_len(ccf, dlc, es58x_priv(netdev)->can.ctrlmode); + if (es58x_flags & ES58X_FLAG_RTR) { + ccf->can_id |= CAN_RTR_FLAG; + len = 0; + } + } + memcpy(cfd->data, data, len); + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += len; + + es58x_set_skb_timestamp(netdev, skb, timestamp); + netif_rx(skb); + + es58x_priv(netdev)->err_passive_before_rtx_success = 0; + + return 0; +} + +/** + * es58x_rx_err_msg() - Handle a received CAN event or error message. + * @netdev: CAN network device. + * @error: Error code. + * @event: Event code. + * @timestamp: Timestamp received from a ES58X device. + * + * Handle the errors and events received by the ES58X device, create + * a CAN error skb and post it. + * + * In some rare cases the devices might get stucked alternating between + * CAN_STATE_ERROR_PASSIVE and CAN_STATE_ERROR_WARNING. To prevent + * this behavior, we force a bus off state if the device goes in + * CAN_STATE_ERROR_WARNING for ES58X_MAX_CONSECUTIVE_WARN consecutive + * times with no successful transmission or reception in between. + * + * Once the device is in bus off state, the only way to restart it is + * through the drivers/net/can/dev.c:can_restart() function. The + * device is technically capable to recover by itself under certain + * circumstances, however, allowing self recovery would create + * complex race conditions with drivers/net/can/dev.c:can_restart() + * and thus was not implemented. To activate automatic restart, please + * set the restart-ms parameter (e.g. ip link set can0 type can + * restart-ms 100). + * + * If the bus is really instable, this function would try to send a + * lot of log messages. Those are rate limited (i.e. you will see + * messages such as "net_ratelimit: XXX callbacks suppressed" in + * dmesg). + * + * Return: zero on success, errno when any error occurs. + */ +int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error, + enum es58x_event event, u64 timestamp) +{ + struct es58x_priv *priv = es58x_priv(netdev); + struct can_priv *can = netdev_priv(netdev); + struct can_device_stats *can_stats = &can->can_stats; + struct can_frame *cf; + struct sk_buff *skb; + + if (!netif_running(netdev)) { + if (net_ratelimit()) + netdev_info(netdev, "%s: %s is down, dropping packet\n", + __func__, netdev->name); + netdev->stats.rx_dropped++; + return 0; + } + + if (error == ES58X_ERR_OK && event == ES58X_EVENT_OK) { + netdev_err(netdev, "%s: Both error and event are zero\n", + __func__); + return -EINVAL; + } + + skb = alloc_can_err_skb(netdev, &cf); + if (!skb) + return -ENOMEM;Please do the stats handling, even if there is no skb.
ACK.
quoted
+ + switch (error) { + case ES58X_ERR_OK: /* 0: No error */ + break; + + case ES58X_ERR_PROT_STUFF:I think you can remove the ratelimiting in front of the debug output.
When the bus is really unstable, there can be hundreds of messages per second. I really prefer to keep it here.
quoted
+ if (net_ratelimit()) + netdev_dbg(netdev, "Error BITSUFF\n"); + cf->data[2] |= CAN_ERR_PROT_STUFF; + break; + + case ES58X_ERR_PROT_FORM: + if (net_ratelimit()) + netdev_dbg(netdev, "Error FORMAT\n"); + cf->data[2] |= CAN_ERR_PROT_FORM; + break; + + case ES58X_ERR_ACK: + if (net_ratelimit()) + netdev_dbg(netdev, "Error ACK\n"); + cf->can_id |= CAN_ERR_ACK; + break; + + case ES58X_ERR_PROT_BIT: + if (net_ratelimit()) + netdev_dbg(netdev, "Error BIT\n"); + cf->data[2] |= CAN_ERR_PROT_BIT; + break; + + case ES58X_ERR_PROT_CRC: + if (net_ratelimit()) + netdev_dbg(netdev, "Error CRC\n"); + cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ; + break; + + case ES58X_ERR_PROT_BIT1: + if (net_ratelimit()) + netdev_dbg(netdev, + "Error: expected a recessive bit but monitored a dominant one\n"); + cf->data[2] |= CAN_ERR_PROT_BIT1; + break; + + case ES58X_ERR_PROT_BIT0: + if (net_ratelimit()) + netdev_dbg(netdev, + "Error expected a dominant bit but monitored a recessive one\n"); + cf->data[2] |= CAN_ERR_PROT_BIT0; + break; + + case ES58X_ERR_PROT_OVERLOAD: + if (net_ratelimit()) + netdev_dbg(netdev, "Error OVERLOAD\n"); + cf->data[2] |= CAN_ERR_PROT_OVERLOAD; + break; + + case ES58X_ERR_PROT_UNSPEC: + if (net_ratelimit()) + netdev_dbg(netdev, "Unspecified error\n"); + cf->can_id |= CAN_ERR_PROT; + break; + + default: + if (net_ratelimit()) + netdev_err(netdev, + "%s: Unspecified error code 0x%04X\n", + __func__, (int)error); + cf->can_id |= CAN_ERR_PROT; + break; + } + + switch (event) { + case ES58X_EVENT_OK: /* 0: No event */ + break; + + case ES58X_EVENT_CRTL_ACTIVE: + if (can->state == CAN_STATE_BUS_OFF) { + netdev_err(netdev, + "%s: state transition: BUS OFF -> ACTIVE\n", + __func__);Your device should not go autoamtically from bus off to active. Only when it's restarted by the kernel (or the user).
Agree, this is why it is an error message. CAN controllers are allowed to automatically recover under very specific conditions (c.f. ISO 11898-1, section 10.9.4 "Bus integration state": "Nodes that are in bus-off state shall re-enter the bus integration state immediately after detecting the idle condition if the bus-off recovery condition is not yet met"). Aside from the idle condition, I do not think that the nodes are allowed to self-recover. But because the can_restart() function does not use locks for the echo_skb[], I treat this scenario by printing an error message and ignoring it.
quoted
+ } + if (net_ratelimit()) + netdev_dbg(netdev, "Event CAN BUS ACTIVE\n"); + cf->data[1] |= CAN_ERR_CRTL_ACTIVE; + can->state = CAN_STATE_ERROR_ACTIVE; + break; + + case ES58X_EVENT_CRTL_PASSIVE: + if (net_ratelimit()) + netdev_dbg(netdev, "Event CAN BUS PASSIVE\n"); + /* Either TX or RX error count reached passive state + * but we do not know which. Setting both flags by + * default. + */ + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; + if (can->state < CAN_STATE_BUS_OFF) + can->state = CAN_STATE_ERROR_PASSIVE; + can_stats->error_passive++; + if (priv->err_passive_before_rtx_success < U8_MAX) + priv->err_passive_before_rtx_success++; + break; + + case ES58X_EVENT_CRTL_WARNING: + if (net_ratelimit()) + netdev_dbg(netdev, "Event CAN BUS WARNING\n"); + /* Either TX or RX error count reached warning state + * but we do not know which. Setting both flags by + * default. + */ + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; + if (can->state < CAN_STATE_BUS_OFF) + can->state = CAN_STATE_ERROR_WARNING; + can_stats->error_warning++; + break; + + case ES58X_EVENT_BUSOFF: + if (net_ratelimit()) + netdev_dbg(netdev, "Event CAN BUS OFF\n"); + netif_stop_queue(netdev); + if (can->state != CAN_STATE_BUS_OFF) { + can->state = CAN_STATE_BUS_OFF; + can_bus_off(netdev); + } + cf->can_id |= CAN_ERR_BUSOFF; + can_stats->bus_off++; + break; + + case ES58X_EVENT_SINGLE_WIRE: + if (net_ratelimit()) + netdev_warn(netdev, + "Lost connection on either CAN high or CAN low\n"); + /* Lost connection on either CAN high or CAN + * low. Setting both flags by default. + */ + cf->data[4] |= CAN_ERR_TRX_CANH_NO_WIRE; + cf->data[4] |= CAN_ERR_TRX_CANL_NO_WIRE; + break; + + default: + if (net_ratelimit()) + netdev_err(netdev, + "%s: Unspecified event code 0x%04X\n", + __func__, (int)event); + cf->can_id |= CAN_ERR_CRTL; + break; + } + + if (cf->data[1]) + cf->can_id |= CAN_ERR_CRTL; + if (cf->data[2] || cf->data[3]) { + cf->can_id |= CAN_ERR_PROT; + can_stats->bus_error++; + } + if (cf->data[4]) + cf->can_id |= CAN_ERR_TRX; + + /* driver/net/can/dev.c:can_restart() takes in account error + * messages in the RX stats. Doing same here for + * consistency. + */ + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += cf->can_dlc; + + es58x_set_skb_timestamp(netdev, skb, timestamp); + netif_rx(skb); + + if ((event & ES58X_EVENT_CRTL_PASSIVE) && + priv->err_passive_before_rtx_success == + ES58X_CONSECUTIVE_ERR_PASSIVE_MAX) { + netdev_info(netdev, + "Got %d consecutive warning events with no successful RX or TX. Forcing bus-off\n", + priv->err_passive_before_rtx_success); + return es58x_rx_err_msg(netdev, ES58X_ERR_OK, + ES58X_EVENT_BUSOFF, timestamp); + } + + return 0; +} + +/* Human readable description of the &enum es58x_cmd_ret_type. */ +static const char *const es58x_cmd_ret_desc[] = { + "Set bittiming", + "Enable channel", + "Disable channel", + "Transmit message", + "Reset RX", + "Reset TX", + "Device error" +}; + +/** + * es58x_rx_dev_ret_u8() - Handle return codes received from the + * ES58X device. + * @dev: Device, only used for the dev_XXX() print functions. + * @cmd_ret_type: Type of the command which triggered the return code. + * @rx_dev_ret_u8: error code as returned by the ES58X device. + * + * Handles the 8 bits return code. Those are specific to the ES581.4 + * device. The return value will eventually be used by + * es58x_handle_urb_cmd() function which will take proper actions in + * case of critical issues such and memory errors or bad CRC values. + * + * In contrast with es58x_rx_cmd_ret_u32(), the network device is + * unknown. + * + * Return: zero on success, return errno when any error occurs. + */ +int es58x_rx_dev_ret_u8(struct device *dev, + enum es58x_ret_type cmd_ret_type, + enum es58x_ret_u8 rx_dev_ret_u8) +{ + const char *ret_desc = es58x_cmd_ret_desc[cmd_ret_type];Can you guarantee not to access es58x_cmd_ret_desc[] out of bounds?
Yes, I guarantee it. The cmd_ret_type does not come directly from the device but from the glue code in es581_4 or es58x_fd. All the calls are made with an hardcoded value (which is in bound of course).
A safe way to do this is: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L94
I like it. I will rewrite it that way :)
quoted
+ + compiletime_assert(ARRAY_SIZE(es58x_cmd_ret_desc) == + ES58X_RET_TYPE_NUM_ENTRIES, + "Size mismatch between es58x_cmd_ret_desc and enum es58x_cmd_ret_type"); + + switch (rx_dev_ret_u8) { + case ES58X_RET_U8_OK: + dev_dbg_ratelimited(dev, "%s: OK\n", ret_desc); + break; + + case ES58X_RET_U8_ERR_UNSPECIFIED_FAILURE: + dev_err(dev, "%s: unspecified failure\n", ret_desc); + break; + + case ES58X_RET_U8_ERR_NO_MEM: + dev_err(dev, "%s: device ran out of memory\n", ret_desc); + return -ENOMEM; + + case ES58X_RET_U8_ERR_BAD_CRC: + dev_err(dev, "%s: CRC of previous command is incorrect\n", + ret_desc); + return -EIO; + + default: + dev_err(dev, "%s: returned unknown value: 0x%02X\n", + ret_desc, rx_dev_ret_u8); + break; + } + + return 0; +} + +/** + * es58x_rx_dev_ret_u32() - Handle return codes received from the + * ES58X device. + * @netdev: CAN network device. + * @cmd_ret_type: Type of the command which triggered the return code. + * @rx_cmd_ret_u32: error code as returned by the ES58X device. + * + * Handles the 32 bits return code. The return value will eventually + * be used by es58x_handle_urb_cmd() function which will take proper + * actions in case of critical issues such and memory errors or bad + * CRC values. + * + * Return: zero on success, errno when any error occurs. + */ +int es58x_rx_cmd_ret_u32(struct net_device *netdev, + enum es58x_ret_type cmd_ret_type, + enum es58x_ret_u32 rx_cmd_ret_u32) +{ + struct es58x_priv *priv = es58x_priv(netdev); + const char *ret_desc = es58x_cmd_ret_desc[cmd_ret_type];same here
ACK.
quoted
+ + switch (rx_cmd_ret_u32) { + case ES58X_RET_U32_OK: + switch (cmd_ret_type) { + case ES58X_RET_TYPE_ENABLE_CHANNEL: + es58x_flush_pending_tx_msg(netdev); + es58x_can_flush_echo_skb(netdev); + priv->err_passive_before_rtx_success = 0; + priv->can.state = CAN_STATE_ERROR_ACTIVE; + netif_wake_queue(netdev); + netdev_info(netdev, + "%s (Serial Number %s): CAN%d channel becomes ready\n", + priv->es58x_dev->udev->product, + priv->es58x_dev->udev->serial, + priv->channel_idx + 1); + break; + + case ES58X_RET_TYPE_TX_MSG: +#ifdef VERBOSE_DEBUGDoes this compile without the ifdef?
Yes, I will remove the #ifdef.
quoted
+ if (net_ratelimit()) + netdev_vdbg(netdev, "%s: OK\n", ret_desc); +#endif + break; + + default: + netdev_dbg(netdev, "%s: OK\n", ret_desc); + break; + } + break; + + case ES58X_RET_U32_ERR_UNSPECIFIED_FAILURE: + if (cmd_ret_type == ES58X_RET_TYPE_DISABLE_CHANNEL) + netdev_dbg(netdev, + "%s: channel is already closed\n", ret_desc); + else + netdev_err(netdev, + "%s: unspecified failure\n", ret_desc); + break; + + case ES58X_RET_U32_ERR_NO_MEM: + netdev_err(netdev, "%s: device ran out of memory\n", ret_desc); + return -ENOMEM; + + case ES58X_RET_U32_WARN_PARAM_ADJUSTED: + netdev_warn(netdev, + "%s: some incompatible parameters have been adjusted\n", + ret_desc); + break; + + case ES58X_RET_U32_WARN_TX_MAYBE_REORDER: + netdev_warn(netdev, + "%s: TX messages might have been reordered\n", + ret_desc); + break; + + case ES58X_RET_U32_ERR_TIMEOUT: + netdev_err(netdev, "%s: command timed out\n", ret_desc); + break; + + case ES58X_RET_U32_ERR_FIFO_FULL: + netdev_warn(netdev, "%s: fifo is full\n", ret_desc); + break; + + case ES58X_RET_U32_ERR_BAD_CONFIG: + netdev_err(netdev, "%s: bad configuration\n", ret_desc); + return -EINVAL; + + case ES58X_RET_U32_ERR_NO_RESOURCE: + netdev_err(netdev, "%s: no resource available\n", ret_desc); + break; + + default: + netdev_err(netdev, "%s returned unknown value: 0x%08X\n", + ret_desc, rx_cmd_ret_u32); + break; + } + + return 0; +}I'll review the rest later.
Thanks a lot! Yours sincerely, Vincent