On 10.10.20 02:57, Jakub Kicinski wrote:
On Wed, 7 Oct 2020 23:31:50 +0200 Marc Kleine-Budde wrote:
From: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
CAN Transport Protocols offer support for segmented Point-to-Point
communication between CAN nodes via two defined CAN Identifiers.
As CAN frames can only transport a small amount of data bytes
(max. 8 bytes for 'classic' CAN and max. 64 bytes for CAN FD) this
segmentation is needed to transport longer PDUs as needed e.g. for
vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
This protocol driver implements data transfers according to
ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
A few random things jump out here at a quick scan. Most of them are
not important enough to have to be addressed, but please follow up on
the 'default y' thing ASAP.
+/*
+ * Remark on CAN_ISOTP_DEFAULT_RECV_* values:
+ *
+ * We can strongly assume, that the Linux Kernel implementation of
+ * CAN_ISOTP is capable to run with BS=0, STmin=0 and WFTmax=0.
+ * But as we like to be able to behave as a commonly available ECU,
+ * these default settings can be changed via sockopts.
+ * For that reason the STmin value is intentionally _not_ checked for
+ * consistency and copied directly into the flow control (FC) frame.
+ *
spurious empty comment line
Oh, yes - at the front and at the end. I will fix that.
+ */
+
+#endif /* !_UAPI_CAN_ISOTP_H */
diff --git a/net/can/Kconfig b/net/can/Kconfig
index 25436a715db3..021fe03a8ed6 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -55,6 +55,19 @@ config CAN_GW
source "net/can/j1939/Kconfig"
+config CAN_ISOTP
+ tristate "ISO 15765-2:2016 CAN transport protocol"
+ default y
default should not be y unless there is a very good reason.
I don't see such reason here. This is new functionality, users
can enable it if they need it.
Yes. I agree. But there is a good reason for it.
The ISO 15765-2 protocol is used for vehicle diagnosis and is a *very*
common CAN bus use case.
The config item only shows up when CONFIG_CAN is selected and then ISO
15765-2 should be enabled too. I have implemented and maintained the
out-of-tree driver for ~12 years now and the people have real problems
using e.g. Ubuntu with signed kernel modules when they need this protocol.
Therefore the option should default to 'y' to make sure the common
distros (that enable CONFIG_CAN) enable ISO-TP too.
+ help
+ CAN Transport Protocols offer support for segmented Point-to-Point
+ communication between CAN nodes via two defined CAN Identifiers.
+ As CAN frames can only transport a small amount of data bytes
+ (max. 8 bytes for 'classic' CAN and max. 64 bytes for CAN FD) this
+ segmentation is needed to transport longer PDUs as needed e.g. for
+ vehicle diagnosis (UDS, ISO 14229) or IP-over-CAN traffic.
+ This protocol driver implements data transfers according to
+ ISO 15765-2:2016 for 'classic' CAN and CAN FD frame types.
+
source "drivers/net/can/Kconfig"
endif
+#define CAN_ISOTP_VERSION "20200928"
We've been removing such version strings throughout the drivers.
Kernel version should be sufficient for in-tree modules.
Yes. Good point.
I will send a separate patch which removes all the VERSION information
from the entire CAN bus subsystem (core, raw, bcm, gw).
+static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
+{
+ struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
+ txtimer);
+ struct sock *sk = &so->sk;
+ struct sk_buff *skb;
+ struct net_device *dev;
+ struct canfd_frame *cf;
+ enum hrtimer_restart restart = HRTIMER_NORESTART;
+ int can_send_ret;
+ int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
+
+ switch (so->tx.state) {
+ case ISOTP_WAIT_FC:
+ case ISOTP_WAIT_FIRST_FC:
+
+ /* we did not get any flow control frame in time */
+
+ /* report 'communication error on send' */
+ sk->sk_err = ECOMM;
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk->sk_error_report(sk);
+
+ /* reset tx state */
+ so->tx.state = ISOTP_IDLE;
+ wake_up_interruptible(&so->wait);
+ break;
+
+ case ISOTP_SENDING:
+
+ /* push out the next segmented pdu */
+ dev = dev_get_by_index(sock_net(sk), so->ifindex);
+ if (!dev)
+ break;
+
+isotp_tx_burst:
+ skb = alloc_skb(so->ll.mtu + sizeof(struct can_skb_priv),
+ gfp_any());
This is always in a timer context, so no need for gfp_any(), right?
Some code from the time where hrtimer was only in hard-irq context. Will
fix that too.
+ if (!skb) {
+ dev_put(dev);
+ break;
+ }
+
+ can_skb_reserve(skb);
+ can_skb_prv(skb)->ifindex = dev->ifindex;
+ can_skb_prv(skb)->skbcnt = 0;
+
+ cf = (struct canfd_frame *)skb->data;
+ skb_put(skb, so->ll.mtu);
+
+ /* create consecutive frame */
+ isotp_fill_dataframe(cf, so, ae, 0);
+
+ /* place consecutive frame N_PCI in appropriate index */
+ cf->data[ae] = N_PCI_CF | so->tx.sn++;
+ so->tx.sn %= 16;
+ so->tx.bs++;
+
+ if (so->ll.mtu == CANFD_MTU)
+ cf->flags = so->ll.tx_flags;
+
+ skb->dev = dev;
+ can_skb_set_owner(skb, sk);
+
+ can_send_ret = can_send(skb, 1);
+ if (can_send_ret)
+ printk_once(KERN_NOTICE "can-isotp: %s: can_send_ret %d\n",
+ __func__, can_send_ret);
pr_notice_once()
Ok. Will change that at the several occurrences.
+
+ if (so->tx.idx >= so->tx.len) {
+ /* we are done */
+ so->tx.state = ISOTP_IDLE;
+ dev_put(dev);
+ wake_up_interruptible(&so->wait);
+ break;
+ }
+
+ if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {
+ /* stop and wait for FC */
+ so->tx.state = ISOTP_WAIT_FC;
+ dev_put(dev);
+ hrtimer_set_expires(&so->txtimer,
+ ktime_add(ktime_get(),
+ ktime_set(1, 0)));
+ restart = HRTIMER_RESTART;
+ break;
+ }
+
+ /* no gap between data frames needed => use burst mode */
+ if (!so->tx_gap)
+ goto isotp_tx_burst;
+
+ /* start timer to send next data frame with correct delay */
+ dev_put(dev);
+ hrtimer_set_expires(&so->txtimer,
+ ktime_add(ktime_get(), so->tx_gap));
+ restart = HRTIMER_RESTART;
+ break;
+
+ default:
+ WARN_ON_ONCE(1);
+ }
+
+ return restart;
+}
Thanks for the review!
Will send the patches for the net-next tree later this day.
Best regards,
Oliver
