Re: [RFC PATCH 00/14] can: netlink: add CAN XL

[Oliver Hartkopp <socketcan@hartkopp.net>]

[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]

Hi Vincent,

I found some issues in the code and fixed up the problems below.

The funniest thing was this copy/paste problem in netlink.h ;-)
(see attached patch with my changes)

The patch descriptions are not finalized - but it becomes usable now.
I will add the CAN XL transceiver switch to the controlmode definitions.

For the PWM configuration we would need some more discussions.

https://lore.kernel.org/linux-can/20241201112333.6950-1-socketcan@hartkopp.net/T/#u
https://lore.kernel.org/linux-can/20241201112230.6917-1-socketcan@hartkopp.net/T/#t

Best regards,
Oliver

On 21.11.24 21:10, Oliver Hartkopp wrote:
> Hi Vincent,
> 
> On 11.11.24 16:32, Oliver Hartkopp wrote:
> 
>> No problem! I will give some feedback when I managed to integrate the 
>> extended netlink API to my driver.
> 
> I managed to set up my CAN XL dev board with the latest Linux kernel 
> (6.13 merge) and upgraded to Ubuntu 24.04 LTS.
> 
> Applying your RFC patches for the kernel and the iproute2 package 
> (including the sed hack) works great.
> 
> While the xl transceiver switch and the and the PWM configuration (with 
> 3 values with 6 bit each) are still missing I tried your current code 
> as-is.
> 
> # modprobe dummyxl
> (created can0)
> 
> # ip link set can0 type can bitrate 500000 xbitrate 8000000 xl on 
> dbitrate 4000000 fd on
> 
> # ip -det link show can0
> 7: can0: <NOARP> mtu 2060 qdisc noop state DOWN mode DEFAULT group 
> default qlen 10
>      link/can  promiscuity 0 allmulti 0 minmtu 0 maxmtu 0
>      can <FD,TDC-AUTO,XL> state STOPPED restart-ms 0
> 
> Should there be a XTDC-AUTO too?
> 
>        bitrate 500000 sample-point 0.875
>        tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 10 brp 1
>        dummyxl nominal: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 
> brp_inc 1
>        dbitrate 4000000 dsample-point 0.750
>        dtq 12 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 2 dbrp 1
>        tdco 15 tdcf 0
> 
> Should the tdc* values be placed behind dbrp 1 without a line break?
> The gso/gro stuff below also heavily exceeds the 80 columns. And it is 
> more in the same shape like with the CAN CC settings.
> 
>        dummyxl FD: dtseg1 2..256 dtseg2 2..128 dsjw 1..128 dbrp 1..512 
> dbrp_inc 1
>        tdco 0..127 tdcf 0..127
> 
> same here
> 
>        xbitrate 8000000 xsample-point 0.700
>        xtq 12 xprop-seg 3 xphase-seg1 3 xphase-seg2 3 xsjw 1 xbrp 1
> 
> xtdco/xtdcf missing here?
> 
>        dummyxl XL: xtseg1 2..256 xtseg2 2..128 xsjw 1..128 xbrp 1..512 
> xbrp_inc 1
> 
> xtdco/xtdcf missing here?
> 
>        clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 
> gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 
> 65536 gso_ipv4_max_size 65536 gro_ipv4_max_size 65536
> 
> Best regards,
> Oliver

[-- Attachment #2: fix-for-vincents-rfc.patch --]
[-- Type: text/x-patch, Size: 3542 bytes --]

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 0a6700194ab0..64f675e874f6 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -127,10 +127,19 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 		if (err) {
 			NL_SET_ERR_MSG_FMT(extack,
 					   "TDC parameters are incorrect");
 			return err;
 		}
+		err = can_validate_tdc(data[IFLA_CAN_XL_TDC],
+				       cm->flags & CAN_CTRLMODE_XL_TDC_AUTO,
+				       cm->flags & CAN_CTRLMODE_XL_TDC_MANUAL,
+				       extack);
+		if (err) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "XL TDC parameters are incorrect");
+			return err;
+		}
 	}
 
 	if (data[IFLA_CAN_BITTIMING]) {
 		struct can_bittiming bt;
 
@@ -570,20 +579,21 @@ static size_t can_get_size(const struct net_device *dev)
 	return size;
 }
 
 static int can_tdc_fill_info(struct sk_buff *skb,  const struct net_device *dev,
 			     struct data_bittiming_params *dbt_params,
-			     bool tdc_is_enabled, bool tdc_manual)
+			     bool tdc_is_enabled, bool tdc_manual,
+			     int ifla_can_tdc_type)
 {
 	struct nlattr *nest;
 	struct can_tdc *tdc = &dbt_params->tdc;
 	const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
 
 	if (!tdc_const)
 		return 0;
 
-	nest = nla_nest_start(skb, IFLA_CAN_TDC);
+	nest = nla_nest_start(skb, ifla_can_tdc_type);
 	if (!nest)
 		return -EMSGSIZE;
 
 	if (tdc_manual &&
 	    (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
@@ -702,11 +712,12 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),
 		     &priv->bitrate_max)) ||
 
 	    can_tdc_fill_info(skb, dev, &priv->fd, can_fd_tdc_is_enabled(priv),
-			      priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) ||
+			      priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL,
+			      IFLA_CAN_TDC) ||
 
 	    can_ctrlmode_ext_fill_info(skb, priv) ||
 
 	    (priv->xl.data_bittiming.bitrate &&
 	     nla_put(skb, IFLA_CAN_XL_DATA_BITTIMING,
@@ -722,11 +733,12 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		     sizeof(*priv->xl.data_bitrate_const) *
 		     priv->xl.data_bitrate_const_cnt,
 		     priv->xl.data_bitrate_const)) ||
 
 	    can_tdc_fill_info(skb, dev, &priv->xl, can_xl_tdc_is_enabled(priv),
-			      priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL)
+			      priv->ctrlmode & CAN_CTRLMODE_XL_TDC_MANUAL,
+			      IFLA_CAN_XL_TDC)
 	    )
 
 		return -EMSGSIZE;
 
 	return 0;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index c81fd153a07f..f4fb8eea8f35 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -102,12 +102,12 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO mode */
 #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
 #define CAN_CTRLMODE_TDC_AUTO		0x200	/* FD transceiver automatically calculates TDCV */
 #define CAN_CTRLMODE_TDC_MANUAL		0x400	/* FD TDCV is manually set up by user */
 #define CAN_CTRLMODE_XL			0x800	/* CAN XL mode */
-#define CAN_CTRLMODE_XL_TDC_AUTO	0x200	/* XL transceiver automatically calculates TDCV */
-#define CAN_CTRLMODE_XL_TDC_MANUAL	0x400	/* XL TDCV is manually set up by user */
+#define CAN_CTRLMODE_XL_TDC_AUTO	0x1000	/* XL transceiver automatically calculates TDCV */
+#define CAN_CTRLMODE_XL_TDC_MANUAL	0x2000	/* XL TDCV is manually set up by user */
 
 /*
  * CAN device statistics
  */
 struct can_device_stats {