On 3/19/21 1:41 PM, Marc Kleine-Budde wrote: > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > 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 <arunachalam.santhanam@xxxxxxxxxxxx> > Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@xxxxxxxxxxxx> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > --- [...] > 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 [...] > +/** > + * 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. 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@xxxxxxxxxxxxxx/ As your device already provides a u64 timestamp you don't need the worker. > + */ > +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..... > + * > + * Return: zero on success. > + */ > +int es58x_rx_timestamp(struct es58x_device *es58x_dev, u64 timestamp) void > +{ > + 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. > + } > + > + 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 > + * 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. > + > + 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? > +{ > + *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@xxxxxxxxxxxxxx > + > + 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. > + 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. > + 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 > + > + 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. > + > + 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. > + 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). > + } > + 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? A safe way to do this is: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L94 > + > + 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 > + > + 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_DEBUG Does this compile without the ifdef? > + 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. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: OpenPGP digital signature
