From 63dfe0709643528290c8a6825f278eda0e3f3c2e Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Sat, 18 Sep 2021 18:56:32 +0900 Subject: can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min ISO 11898-1 specifies in section 11.3.3 "Transmitter delay compensation" that "the configuration range for [the] SSP position shall be at least 0 to 63 minimum time quanta." Because SSP = TDCV + TDCO, it means that we should allow both TDCV and TDCO to hold zero value in order to honor SSP's minimum possible value. However, current implementation assigned special meaning to TDCV and TDCO's zero values: * TDCV = 0 -> TDCV is automatically measured by the transceiver. * TDCO = 0 -> TDC is off. In order to allow for those values to really be zero and to maintain current features, we introduce two new flags: * CAN_CTRLMODE_TDC_AUTO indicates that the controller support automatic measurement of TDCV. * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support manual configuration of TDCV. N.B.: current implementation failed to provide an option for the driver to indicate that only manual mode was supported. TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function can_tdc_is_enabled() which is also introduced in this patch. Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to struct can_tdc_const. While we are not convinced that those three fields could be anything else than zero, we can imagine that some controllers might specify a lower bound on these. Thus, those minimums are really added "just in case". Comments of struct can_tdc and can_tdc_const are updated accordingly. Finally, the changes are applied to the etas_es58x driver. Link: https://lore.kernel.org/all/20210918095637.20108-2-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/bittiming.h | 64 +++++++++++++++++++++++++++++++------------ include/linux/can/dev.h | 4 +++ 2 files changed, 51 insertions(+), 17 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h index 9de6e9053e34..9e20260611cc 100644 --- a/include/linux/can/bittiming.h +++ b/include/linux/can/bittiming.h @@ -19,6 +19,9 @@ /* Megahertz */ #define CAN_MHZ 1000000UL +#define CAN_CTRLMODE_TDC_MASK \ + (CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL) + /* * struct can_tdc - CAN FD Transmission Delay Compensation parameters * @@ -33,29 +36,43 @@ * * This structure contains the parameters to calculate that SSP. * - * @tdcv: Transmitter Delay Compensation Value. Distance, in time - * quanta, from when the bit is sent on the TX pin to when it is - * received on the RX pin of the transmitter. Possible options: + * -+----------- one bit ----------+-- TX pin + * |<--- Sample Point --->| + * + * --+----------- one bit ----------+-- RX pin + * |<-------- TDCV -------->| + * |<------- TDCO ------->| + * |<----------- Secondary Sample Point ---------->| + * + * @tdcv: Transmitter Delay Compensation Value. The time needed for + * the signal to propagate, i.e. the distance, in time quanta, + * from the start of the bit on the TX pin to when it is received + * on the RX pin. @tdcv depends on the controller modes: + * + * CAN_CTRLMODE_TDC_AUTO is set: The transceiver dynamically + * measures @tdcv for each transmitted CAN FD frame and the + * value provided here should be ignored. * - * 0: automatic mode. The controller dynamically measures @tdcv - * for each transmitted CAN FD frame. + * CAN_CTRLMODE_TDC_MANUAL is set: use the fixed provided @tdcv + * value. * - * Other values: manual mode. Use the fixed provided value. + * N.B. CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL are + * mutually exclusive. Only one can be set at a time. If both + * CAN_TDC_CTRLMODE_AUTO and CAN_TDC_CTRLMODE_MANUAL are unset, + * TDC is disabled and all the values of this structure should be + * ignored. * * @tdco: Transmitter Delay Compensation Offset. Offset value, in time * quanta, defining the distance between the start of the bit * reception on the RX pin of the transceiver and the SSP * position such that SSP = @tdcv + @tdco. * - * If @tdco is zero, then TDC is disabled and both @tdcv and - * @tdcf should be ignored. - * * @tdcf: Transmitter Delay Compensation Filter window. Defines the - * minimum value for the SSP position in time quanta. If SSP is - * less than @tdcf, then no delay compensations occur and the - * normal sampling point is used instead. The feature is enabled - * if and only if @tdcv is set to zero (automatic mode) and @tdcf - * is configured to a value greater than @tdco. + * minimum value for the SSP position in time quanta. If the SSP + * position is less than @tdcf, then no delay compensations occur + * and the normal sampling point is used instead. The feature is + * enabled if and only if @tdcv is set to zero (automatic mode) + * and @tdcf is configured to a value greater than @tdco. */ struct can_tdc { u32 tdcv; @@ -67,19 +84,32 @@ struct can_tdc { * struct can_tdc_const - CAN hardware-dependent constant for * Transmission Delay Compensation * - * @tdcv_max: Transmitter Delay Compensation Value maximum value. - * Should be set to zero if the controller does not support - * manual mode for tdcv. + * @tdcv_min: Transmitter Delay Compensation Value minimum value. If + * the controller does not support manual mode for tdcv + * (c.f. flag CAN_CTRLMODE_TDC_MANUAL) then this value is + * ignored. + * @tdcv_max: Transmitter Delay Compensation Value maximum value. If + * the controller does not support manual mode for tdcv + * (c.f. flag CAN_CTRLMODE_TDC_MANUAL) then this value is + * ignored. + * + * @tdco_min: Transmitter Delay Compensation Offset minimum value. * @tdco_max: Transmitter Delay Compensation Offset maximum value. * Should not be zero. If the controller does not support TDC, * then the pointer to this structure should be NULL. + * + * @tdcf_min: Transmitter Delay Compensation Filter window minimum + * value. If @tdcf_max is zero, this value is ignored. * @tdcf_max: Transmitter Delay Compensation Filter window maximum * value. Should be set to zero if the controller does not * support this feature. */ struct can_tdc_const { + u32 tdcv_min; u32 tdcv_max; + u32 tdco_min; u32 tdco_max; + u32 tdcf_min; u32 tdcf_max; }; diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 2413253e54c7..6dacbbb41e68 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -96,6 +96,10 @@ struct can_priv { #endif }; +static inline bool can_tdc_is_enabled(const struct can_priv *priv) +{ + return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK); +} /* helper to define static CAN controller features at device creation time */ static inline void can_set_static_ctrlmode(struct net_device *dev, -- cgit v1.2.3 From 39f66c9e229797a58a12ea78388cbbad1f81aec9 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Sat, 18 Sep 2021 18:56:33 +0900 Subject: can: bittiming: change unit of TDC parameters to clock periods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the current implementation, all Transmission Delay Compensation (TDC) parameters are expressed in time quantum. However, ISO 11898-1 actually specifies that these should be expressed in *minimum* time quantum. Furthermore, the minimum time quantum is specified to be "one node clock period long" (c.f. paragraph 11.3.1.1 "Bit time"). For sake of simplicity, we prefer to use the "clock period" term instead of "minimum time quantum" because we believe that it is more broadly understood. This patch fixes that discrepancy by updating the documentation and the formula for TDCO calculation. N.B. In can_calc_tdco(), the sample point (in time quantum) was calculated using a division, thus introducing a risk of rounding and truncation errors. On top of changing the unit to clock period, we also modified the formula to use only additions. Link: https://lore.kernel.org/all/20210918095637.20108-3-mailhol.vincent@wanadoo.fr Suggested-by: Stefan Mätje Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/bittiming.h | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h index 9e20260611cc..aebbe65dab7e 100644 --- a/include/linux/can/bittiming.h +++ b/include/linux/can/bittiming.h @@ -31,8 +31,8 @@ * * To solve this issue, ISO 11898-1 introduces in section 11.3.3 * "Transmitter delay compensation" a SSP (Secondary Sample Point) - * equal to the distance, in time quanta, from the start of the bit - * time on the TX pin to the actual measurement on the RX pin. + * equal to the distance from the start of the bit time on the TX pin + * to the actual measurement on the RX pin. * * This structure contains the parameters to calculate that SSP. * @@ -44,8 +44,13 @@ * |<------- TDCO ------->| * |<----------- Secondary Sample Point ---------->| * + * To increase precision, contrary to the other bittiming parameters + * which are measured in time quanta, the TDC parameters are measured + * in clock periods (also referred as "minimum time quantum" in ISO + * 11898-1). + * * @tdcv: Transmitter Delay Compensation Value. The time needed for - * the signal to propagate, i.e. the distance, in time quanta, + * the signal to propagate, i.e. the distance, in clock periods, * from the start of the bit on the TX pin to when it is received * on the RX pin. @tdcv depends on the controller modes: * @@ -62,17 +67,18 @@ * TDC is disabled and all the values of this structure should be * ignored. * - * @tdco: Transmitter Delay Compensation Offset. Offset value, in time - * quanta, defining the distance between the start of the bit - * reception on the RX pin of the transceiver and the SSP + * @tdco: Transmitter Delay Compensation Offset. Offset value, in + * clock periods, defining the distance between the start of the + * bit reception on the RX pin of the transceiver and the SSP * position such that SSP = @tdcv + @tdco. * * @tdcf: Transmitter Delay Compensation Filter window. Defines the - * minimum value for the SSP position in time quanta. If the SSP - * position is less than @tdcf, then no delay compensations occur - * and the normal sampling point is used instead. The feature is - * enabled if and only if @tdcv is set to zero (automatic mode) - * and @tdcf is configured to a value greater than @tdco. + * minimum value for the SSP position in clock periods. If the + * SSP position is less than @tdcf, then no delay compensations + * occur and the normal sampling point is used instead. The + * feature is enabled if and only if @tdcv is set to zero + * (automatic mode) and @tdcf is configured to a value greater + * than @tdco. */ struct can_tdc { u32 tdcv; -- cgit v1.2.3 From da45a1e4d7b9d6b5a8231acb812df719fe3228b4 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Sat, 18 Sep 2021 18:56:34 +0900 Subject: can: bittiming: change can_calc_tdco()'s prototype to not directly modify priv The function can_calc_tdco() directly retrieves can_priv from the net_device and directly modifies it. This is annoying for the upcoming patch. In drivers/net/can/dev/netlink.c:can_changelink(), the data bittiming are written to a temporary structure and memcpyed to can_priv only after everything succeeded. In the next patch, where we will introduce the netlink interface for TDC parameters, we will add a new TDC block which can potentially fail. For this reason, the data bittiming temporary structure has to be copied after that to-be-introduced TDC block. However, TDC also needs to access data bittiming information. We change the prototype so that the data bittiming structure is passed to can_calc_tdco() as an argument instead of retrieving it from priv. This way can_calc_tdco() can access the data bittiming before it gets memcpyed to priv. Link: https://lore.kernel.org/all/20210918095637.20108-4-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/bittiming.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h index aebbe65dab7e..20b50baf3a02 100644 --- a/include/linux/can/bittiming.h +++ b/include/linux/can/bittiming.h @@ -123,7 +123,9 @@ struct can_tdc_const { int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt, const struct can_bittiming_const *btc); -void can_calc_tdco(struct net_device *dev); +void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const, + const struct can_bittiming *dbt, + u32 *ctrlmode, u32 ctrlmode_supported); #else /* !CONFIG_CAN_CALC_BITTIMING */ static inline int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt, @@ -133,7 +135,10 @@ can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt, return -EINVAL; } -static inline void can_calc_tdco(struct net_device *dev) +static inline void +can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const, + const struct can_bittiming *dbt, + u32 *ctrlmode, u32 ctrlmode_supported) { } #endif /* CONFIG_CAN_CALC_BITTIMING */ -- cgit v1.2.3 From e8060f08cd69d1d692cfb9f0a2808477a501f35a Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Sat, 18 Sep 2021 18:56:36 +0900 Subject: can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some CAN device can measure the TDCV (Transmission Delay Compensation Value) automatically for each transmitted CAN frames. A callback function do_get_auto_tdcv() is added to retrieve that value. This function is used only if CAN_CTRLMODE_TDC_AUTO is enabled (if CAN_CTRLMODE_TDC_MANUAL is selected, the TDCV value is provided by the user). If the device does not support reporting of TDCV, do_get_auto_tdcv() should be set to NULL and TDCV will not be reported by the netlink interface. On success, do_get_auto_tdcv() shall return 0. If the value can not be measured by the device, for example because network is down or because no frames were transmitted yet, can_priv::do_get_auto_tdcv() shall return a negative error code (e.g. -EINVAL) to signify that the value is not yet available. In such cases, TDCV is not reported by the netlink interface. Link: https://lore.kernel.org/all/20210918095637.20108-6-mailhol.vincent@wanadoo.fr CC: Stefan Mätje Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/dev.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/linux/can') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 6dacbbb41e68..b4aa0f048cab 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -82,6 +82,7 @@ struct can_priv { enum can_state *state); int (*do_get_berr_counter)(const struct net_device *dev, struct can_berr_counter *bec); + int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv); unsigned int echo_skb_max; struct sk_buff **echo_skb; -- cgit v1.2.3 From fa759a9395ea81c17db613dde43c46f0607df7e7 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Sat, 18 Sep 2021 18:56:37 +0900 Subject: can: dev: add can_tdc_get_relative_tdco() helper function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit struct can_tdc::tdco represents the absolute offset from TDCV. Some controllers use instead an offset relative to the Sample Point (SP) such that: | SSP = TDCV + absolute TDCO | = TDCV + SP + relative TDCO Consequently: | relative TDCO = absolute TDCO - SP The function can_tdc_get_relative_tdco() allow to retrieve this relative TDCO value. Link: https://lore.kernel.org/all/20210918095637.20108-7-mailhol.vincent@wanadoo.fr CC: Stefan Mätje Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/dev.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'include/linux/can') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index b4aa0f048cab..45f19d9db5ca 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -102,6 +102,35 @@ static inline bool can_tdc_is_enabled(const struct can_priv *priv) return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK); } +/* + * can_get_relative_tdco() - TDCO relative to the sample point + * + * struct can_tdc::tdco represents the absolute offset from TDCV. Some + * controllers use instead an offset relative to the Sample Point (SP) + * such that: + * + * SSP = TDCV + absolute TDCO + * = TDCV + SP + relative TDCO + * + * -+----------- one bit ----------+-- TX pin + * |<--- Sample Point --->| + * + * --+----------- one bit ----------+-- RX pin + * |<-------- TDCV -------->| + * |<------------------------>| absolute TDCO + * |<--- Sample Point --->| + * | |<->| relative TDCO + * |<------------- Secondary Sample Point ------------>| + */ +static inline s32 can_get_relative_tdco(const struct can_priv *priv) +{ + const struct can_bittiming *dbt = &priv->data_bittiming; + s32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg + + dbt->phase_seg1) * dbt->brp; + + return (s32)priv->tdc.tdco - sample_point_in_tc; +} + /* helper to define static CAN controller features at device creation time */ static inline void can_set_static_ctrlmode(struct net_device *dev, u32 static_mode) -- cgit v1.2.3 From 330c6d3bfa268794bf692165d0f781f1c2d4d83e Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Wed, 24 Nov 2021 10:45:36 +0900 Subject: can: bittiming: replace CAN units with the generic ones from linux/units.h In [1], we introduced a set of units in linux/can/bittiming.h. Since then, generic SI prefixes were added to linux/units.h in [2]. Those new prefixes can perfectly replace CAN specific ones. This patch replaces all occurrences of the CAN units with their corresponding prefix (from linux/units) and the unit (as a comment) according to below table. CAN units SI metric prefix (from linux/units) + unit (as a comment) ------------------------------------------------------------------------ CAN_KBPS KILO /* BPS */ CAN_MBPS MEGA /* BPS */ CAM_MHZ MEGA /* Hz */ The definition are then removed from linux/can/bittiming.h [1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and CAN_MHZ macros") [2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions") Link: https://lore.kernel.org/all/20211124014536.782550-1-mailhol.vincent@wanadoo.fr Suggested-by: Jimmy Assarsson Suggested-by: Oliver Hartkopp Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/bittiming.h | 7 ------- 1 file changed, 7 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h index 20b50baf3a02..a81652d1c6f3 100644 --- a/include/linux/can/bittiming.h +++ b/include/linux/can/bittiming.h @@ -12,13 +12,6 @@ #define CAN_SYNC_SEG 1 -/* Kilobits and Megabits per second */ -#define CAN_KBPS 1000UL -#define CAN_MBPS 1000000UL - -/* Megahertz */ -#define CAN_MHZ 1000000UL - #define CAN_CTRLMODE_TDC_MASK \ (CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL) -- cgit v1.2.3 From cc4b08c31b5c51352f258032cc65e884b3e61e6a Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 7 Dec 2021 21:15:31 +0900 Subject: can: do not increase tx_bytes statistics for RTR frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The actual payload length of the CAN Remote Transmission Request (RTR) frames is always 0, i.e. no payload is transmitted on the wire. However, those RTR frames still use the DLC to indicate the length of the requested frame. As such, net_device_stats::tx_bytes should not be increased when sending RTR frames. The function can_get_echo_skb() already returns the correct length, even for RTR frames (c.f. [1]). However, for historical reasons, the drivers do not use can_get_echo_skb()'s return value and instead, most of them store a temporary length (or dlc) in some local structure or array. Using the return value of can_get_echo_skb() solves the issue. After doing this, such length/dlc fields become unused and so this patch does the adequate cleaning when needed. This patch fixes all the CAN drivers. Finally, can_get_echo_skb() is decorated with the __must_check attribute in order to force future drivers to correctly use its return value (else the compiler would emit a warning). [1] commit ed3320cec279 ("can: dev: __can_get_echo_skb(): fix real payload length return value for RTR frames") Link: https://lore.kernel.org/all/20211207121531.42941-6-mailhol.vincent@wanadoo.fr Cc: Nicolas Ferre Cc: Alexandre Belloni Cc: Ludovic Desroches Cc: Maxime Ripard Cc: Chen-Yu Tsai Cc: Jernej Skrabec Cc: Yasushi SHOJI Cc: Oliver Hartkopp Cc: Stephane Grosjean Cc: Andreas Larsson Tested-by: Jimmy Assarsson # kvaser Signed-off-by: Vincent Mailhol Acked-by: Stefan Mätje # esd_usb2 Tested-by: Stefan Mätje # esd_usb2 [mkl: add conversion for grcan] Signed-off-by: Marc Kleine-Budde --- include/linux/can/skb.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h index d311bc369a39..fdb22b00674a 100644 --- a/include/linux/can/skb.h +++ b/include/linux/can/skb.h @@ -21,8 +21,9 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, unsigned int idx, unsigned int frame_len); struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr, unsigned int *frame_len_ptr); -unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx, - unsigned int *frame_len_ptr); +unsigned int __must_check can_get_echo_skb(struct net_device *dev, + unsigned int idx, + unsigned int *frame_len_ptr); void can_free_echo_skb(struct net_device *dev, unsigned int idx, unsigned int *frame_len_ptr); struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf); -- cgit v1.2.3 From c9e1d8ed304cc6106c3241add170193995953325 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 14 Dec 2021 01:02:23 +0900 Subject: can: dev: replace can_priv::ctrlmode_static by can_get_static_ctrlmode() The statically enabled features of a CAN controller can be retrieved using below formula: | u32 ctrlmode_static = priv->ctrlmode & ~priv->ctrlmode_supported; As such, there is no need to store this information. This patch remove the field ctrlmode_static of struct can_priv and provides, in replacement, the inline function can_get_static_ctrlmode() which returns the same value. Link: https://lore.kernel.org/all/20211213160226.56219-2-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/dev.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 45f19d9db5ca..92e2d69462f0 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -69,7 +69,6 @@ struct can_priv { /* CAN controller features - see include/uapi/linux/can/netlink.h */ u32 ctrlmode; /* current options setting */ u32 ctrlmode_supported; /* options that can be modified by netlink */ - u32 ctrlmode_static; /* static enabled options for driver/hardware */ int restart_ms; struct delayed_work restart_work; @@ -139,13 +138,17 @@ static inline void can_set_static_ctrlmode(struct net_device *dev, /* alloc_candev() succeeded => netdev_priv() is valid at this point */ priv->ctrlmode = static_mode; - priv->ctrlmode_static = static_mode; /* override MTU which was set by default in can_setup()? */ if (static_mode & CAN_CTRLMODE_FD) dev->mtu = CANFD_MTU; } +static inline u32 can_get_static_ctrlmode(struct can_priv *priv) +{ + return priv->ctrlmode & ~priv->ctrlmode_supported; +} + void can_setup(struct net_device *dev); struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, -- cgit v1.2.3 From 7d4a101c0bd3c6e5c6e45c705a54f7bc8f6c128d Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 14 Dec 2021 01:02:24 +0900 Subject: can: dev: add sanity check in can_set_static_ctrlmode() Previous patch removed can_priv::ctrlmode_static to replace it with can_get_static_ctrlmode(). A condition sine qua non for this to work is that the controller static modes should never be set in can_priv::ctrlmode_supported (c.f. the comment on can_priv::ctrlmode_supported which states that it is for "options that can be *modified* by netlink"). Also, this condition is already correctly fulfilled by all existing drivers which rely on the ctrlmode_static feature. Nonetheless, we added an extra safeguard in can_set_static_ctrlmode() to return an error value and to warn the developer who would be adventurous enough to set to static a given feature that is already set to supported. The drivers which rely on the static controller mode are then updated to check the return value of can_set_static_ctrlmode(). Link: https://lore.kernel.org/all/20211213160226.56219-3-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/dev.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 92e2d69462f0..fff3f70df697 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -131,17 +131,24 @@ static inline s32 can_get_relative_tdco(const struct can_priv *priv) } /* helper to define static CAN controller features at device creation time */ -static inline void can_set_static_ctrlmode(struct net_device *dev, - u32 static_mode) +static inline int __must_check can_set_static_ctrlmode(struct net_device *dev, + u32 static_mode) { struct can_priv *priv = netdev_priv(dev); /* alloc_candev() succeeded => netdev_priv() is valid at this point */ + if (priv->ctrlmode_supported & static_mode) { + netdev_warn(dev, + "Controller features can not be supported and static at the same time\n"); + return -EINVAL; + } priv->ctrlmode = static_mode; /* override MTU which was set by default in can_setup()? */ if (static_mode & CAN_CTRLMODE_FD) dev->mtu = CANFD_MTU; + + return 0; } static inline u32 can_get_static_ctrlmode(struct can_priv *priv) -- cgit v1.2.3 From 5fe1be81efd28bf2afcac218f23118dca6b1b648 Mon Sep 17 00:00:00 2001 From: Vincent Mailhol Date: Tue, 14 Dec 2021 01:02:25 +0900 Subject: can: dev: reorder struct can_priv members for better packing Save eight bytes of holes on x86-64 architectures by reordering the members of struct can_priv. Before: | $ pahole -C can_priv drivers/net/can/dev/dev.o | struct can_priv { | struct net_device * dev; /* 0 8 */ | struct can_device_stats can_stats; /* 8 24 */ | const struct can_bittiming_const * bittiming_const; /* 32 8 */ | const struct can_bittiming_const * data_bittiming_const; /* 40 8 */ | struct can_bittiming bittiming; /* 48 32 */ | /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ | struct can_bittiming data_bittiming; /* 80 32 */ | const struct can_tdc_const * tdc_const; /* 112 8 */ | struct can_tdc tdc; /* 120 12 */ | /* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */ | unsigned int bitrate_const_cnt; /* 132 4 */ | const u32 * bitrate_const; /* 136 8 */ | const u32 * data_bitrate_const; /* 144 8 */ | unsigned int data_bitrate_const_cnt; /* 152 4 */ | u32 bitrate_max; /* 156 4 */ | struct can_clock clock; /* 160 4 */ | unsigned int termination_const_cnt; /* 164 4 */ | const u16 * termination_const; /* 168 8 */ | u16 termination; /* 176 2 */ | | /* XXX 6 bytes hole, try to pack */ | | struct gpio_desc * termination_gpio; /* 184 8 */ | /* --- cacheline 3 boundary (192 bytes) --- */ | u16 termination_gpio_ohms[2]; /* 192 4 */ | enum can_state state; /* 196 4 */ | u32 ctrlmode; /* 200 4 */ | u32 ctrlmode_supported; /* 204 4 */ | int restart_ms; /* 208 4 */ | | /* XXX 4 bytes hole, try to pack */ | | struct delayed_work restart_work; /* 216 88 */ | | /* XXX last struct has 4 bytes of padding */ | | /* --- cacheline 4 boundary (256 bytes) was 48 bytes ago --- */ | int (*do_set_bittiming)(struct net_device *); /* 304 8 */ | int (*do_set_data_bittiming)(struct net_device *); /* 312 8 */ | /* --- cacheline 5 boundary (320 bytes) --- */ | int (*do_set_mode)(struct net_device *, enum can_mode); /* 320 8 */ | int (*do_set_termination)(struct net_device *, u16); /* 328 8 */ | int (*do_get_state)(const struct net_device *, enum can_state *); /* 336 8 */ | int (*do_get_berr_counter)(const struct net_device *, struct can_berr_counter *); /* 344 8 */ | unsigned int echo_skb_max; /* 352 4 */ | | /* XXX 4 bytes hole, try to pack */ | | struct sk_buff * * echo_skb; /* 360 8 */ | | /* size: 368, cachelines: 6, members: 32 */ | /* sum members: 354, holes: 3, sum holes: 14 */ | /* paddings: 1, sum paddings: 4 */ | /* last cacheline: 48 bytes */ | }; After: | $ pahole -C can_priv drivers/net/can/dev/dev.o | struct can_priv { | struct net_device * dev; /* 0 8 */ | struct can_device_stats can_stats; /* 8 24 */ | const struct can_bittiming_const * bittiming_const; /* 32 8 */ | const struct can_bittiming_const * data_bittiming_const; /* 40 8 */ | struct can_bittiming bittiming; /* 48 32 */ | /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ | struct can_bittiming data_bittiming; /* 80 32 */ | const struct can_tdc_const * tdc_const; /* 112 8 */ | struct can_tdc tdc; /* 120 12 */ | /* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */ | unsigned int bitrate_const_cnt; /* 132 4 */ | const u32 * bitrate_const; /* 136 8 */ | const u32 * data_bitrate_const; /* 144 8 */ | unsigned int data_bitrate_const_cnt; /* 152 4 */ | u32 bitrate_max; /* 156 4 */ | struct can_clock clock; /* 160 4 */ | unsigned int termination_const_cnt; /* 164 4 */ | const u16 * termination_const; /* 168 8 */ | u16 termination; /* 176 2 */ | | /* XXX 6 bytes hole, try to pack */ | | struct gpio_desc * termination_gpio; /* 184 8 */ | /* --- cacheline 3 boundary (192 bytes) --- */ | u16 termination_gpio_ohms[2]; /* 192 4 */ | unsigned int echo_skb_max; /* 196 4 */ | struct sk_buff * * echo_skb; /* 200 8 */ | enum can_state state; /* 208 4 */ | u32 ctrlmode; /* 212 4 */ | u32 ctrlmode_supported; /* 216 4 */ | int restart_ms; /* 220 4 */ | struct delayed_work restart_work; /* 224 88 */ | | /* XXX last struct has 4 bytes of padding */ | | /* --- cacheline 4 boundary (256 bytes) was 56 bytes ago --- */ | int (*do_set_bittiming)(struct net_device *); /* 312 8 */ | /* --- cacheline 5 boundary (320 bytes) --- */ | int (*do_set_data_bittiming)(struct net_device *); /* 320 8 */ | int (*do_set_mode)(struct net_device *, enum can_mode); /* 328 8 */ | int (*do_set_termination)(struct net_device *, u16); /* 336 8 */ | int (*do_get_state)(const struct net_device *, enum can_state *); /* 344 8 */ | int (*do_get_berr_counter)(const struct net_device *, struct can_berr_counter *); /* 352 8 */ | | /* size: 360, cachelines: 6, members: 32 */ | /* sum members: 354, holes: 1, sum holes: 6 */ | /* paddings: 1, sum paddings: 4 */ | /* last cacheline: 40 bytes */ | }; Link: https://lore.kernel.org/all/20211213160226.56219-4-mailhol.vincent@wanadoo.fr Signed-off-by: Vincent Mailhol Signed-off-by: Marc Kleine-Budde --- include/linux/can/dev.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'include/linux/can') diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index fff3f70df697..c2ea47f30046 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -64,6 +64,9 @@ struct can_priv { struct gpio_desc *termination_gpio; u16 termination_gpio_ohms[CAN_TERMINATION_GPIO_MAX]; + unsigned int echo_skb_max; + struct sk_buff **echo_skb; + enum can_state state; /* CAN controller features - see include/uapi/linux/can/netlink.h */ @@ -83,9 +86,6 @@ struct can_priv { struct can_berr_counter *bec); int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv); - unsigned int echo_skb_max; - struct sk_buff **echo_skb; - #ifdef CONFIG_CAN_LEDS struct led_trigger *tx_led_trig; char tx_led_trig_name[CAN_LED_NAME_SZ]; -- cgit v1.2.3