[permalink] [raw]

On 9/30/20 4:45 PM, Vincent Mailhol wrote:
> In classical CAN, the length of the data (i.e. CAN payload) is not
> always equal to the DLC! If the frame is a Remote Transmission Request
> (RTR), data length is always zero regardless of DLC value and else, if
> the DLC is greater than 8, the length is 8. Contrary to common belief,
> ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
> for Classical Frames and specifies that those DLCs shall indicate that
> the data field is 8 bytes long.
>
> Above facts are widely unknown and so many developpers uses the "len"
> field of "struct canfd_frame" to get the length of classical CAN
> frames: this is incorrect!
>
> This patch introduces function get_can_len() which can be used in
> remediation. The function takes the SKB as an input in order to be
> able to determine if the frame is classical or FD.
>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
> include/linux/can/dev.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 5e3d45525bd3..72a8a60c0094 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -177,6 +177,29 @@ u8 can_dlc2len(u8 can_dlc);
> /* map the sanitized data length to an appropriate data length code */
> u8 can_len2dlc(u8 len);
>
> +/*
> + * get_can_len(skb) - get the length of the CAN payload.
> + *
> + * In classical CAN, the length of the data (i.e. CAN payload) is not
> + * always equal to the DLC! If the frame is a Remote Transmission
> + * Request (RTR), data length is always zero regardless of DLC value
> + * and else, if the DLC is greater than 8, the length is 8. Contrary
> + * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
> + * DLCs greater than 8 for Classical Frames and specifies that those
> + * DLCs shall indicate that the data field is 8 bytes long.
> + */
> +static inline int get_can_len(struct sk_buff *skb)

make this return an u8
make the skb const

> +{
> + struct canfd_frame *cf = (struct canfd_frame *)skb->data;

const

> +
> + if (can_is_canfd_skb(skb))
> + return min_t(__u8, cf->len, CANFD_MAX_DLEN);

u8

> + else if (cf->can_id & CAN_RTR_FLAG)
> + return 0;
> + else
> + return min_t(__u8, cf->len, CAN_MAX_DLEN);

u8

> +}
> +
> struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
> unsigned int txqs, unsigned int rxqs);
> #define alloc_candev(sizeof_priv, echo_skb_max) \
>

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 |

Attachments:

signature.asc (499.00 B)
OpenPGP digital signature

2020-09-30 16:22:51

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2 5/6] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

On Wed, Sep 30, 2020 at 11:45:32PM +0900, Vincent Mailhol wrote:
> + num_element =
> + es58x_msg_num_element(es58x_dev->dev,
> + bulk_rx_loopback_msg->rx_loopback_msg,
> + msg_len);
> + if (unlikely(num_element <= 0))
> + return num_element;

Meta-comment on your use of 'unlikely' everywhere. Please drop it, it's
only to be used if you can actually measure the difference in a
benchmark. You are dealing with USB devices, which are really really
slow here. Also, humans make horrible guessers for this type of thing,
the compiler and CPU can get this right much more often than we can, and
we had the numbers for it (someone measured that 80-90% of our usages of
these markings are actually wrong on modern cpus).

So just drop them all, it makes the code simpler to read and understand,
and the cpu can actually go faster.

thanks,

greg k-h

2020-10-01 15:47:41

by Vincent MAILHOL

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] can: dev: add a helper function

> > +static inline int get_can_len(struct sk_buff *skb)
>
> make this return an u8
> make the skb const
>
> > +{
> > + struct canfd_frame *cf =3D (struct canfd_frame *)skb->data;
>
> const
>
> > +
> > + if (can_is_canfd_skb(skb))
> > + return min_t(__u8, cf->len, CANFD_MAX_DLEN);
>
> u8
>
> > + else if (cf->can_id & CAN_RTR_FLAG)
> > + return 0;
> > + else
> > + return min_t(__u8, cf->len, CAN_MAX_DLEN);
>
> u8

Noted. All those changes will be addressed in v3 series.
Thank you.

As a side note, macros get_can_dlc() and get_canfd_dlc of the same
file (include/linux/can/dev.h) also use __u8 instead of u8. Do you
want me to add a patch to change these as below?

/*
* get_can_dlc(value) - helper macro to cast a given data length code (dlc)
- * to __u8 and ensure the dlc value to be max. 8 bytes.
+ * to u8 and ensure the dlc value to be max. 8 bytes.
*
* To be used in the CAN netdriver receive path to ensure conformance with
* ISO 11898-1 Chapter 8.4.2.3 (DLC field)
*/
-#define get_can_dlc(i) (min_t(__u8, (i), CAN_MAX_DLC))
-#define get_canfd_dlc(i) (min_t(__u8, (i), CANFD_MAX_DLC))
+#define get_can_dlc(i) (min_t(u8, (i), CAN_MAX_DLC))
+#define get_canfd_dlc(i) (min_t(u8, (i), CANFD_MAX_DLC))

/* Check for outgoing skbs that have not been created by the CAN subsystem */
static inline bool can_skb_headroom_valid(struct net_device *dev,

2020-10-01 15:54:34

On 10/2/20 5:41 PM, Vincent Mailhol wrote:
> Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
> through include/linux/can/dev.h
>
> Add an helper function can_bit_time() which returns the duration (in
> time quanta) of one CAN bit.
>
> Rationale for this patch: the sync segment and the bit time are two
> concepts which are defined in the CAN ISO standard. Device drivers for
> CAN might need those.
>
> Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
> additional information.
>
> Signed-off-by: Vincent Mailhol <[email protected]>
> ---
>
> Changes in v3: None
>
> Changes in v2: None
> ---
> drivers/net/can/dev.c | 13 ++++++-------
> include/linux/can/dev.h | 15 +++++++++++++++
> 2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 8c3e11820e03..6070b4ab3bd8 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -60,7 +60,6 @@ EXPORT_SYMBOL_GPL(can_len2dlc);
>
> #ifdef CONFIG_CAN_CALC_BITTIMING
> #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
> -#define CAN_CALC_SYNC_SEG 1
>
> /* Bit-timing calculation derived from:
> *
> @@ -86,8 +85,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
> int i;
>
> for (i = 0; i <= 1; i++) {
> - tseg2 = tseg + CAN_CALC_SYNC_SEG -
> - (sample_point_nominal * (tseg + CAN_CALC_SYNC_SEG)) /
> + tseg2 = tseg + CAN_SYNC_SEG -
> + (sample_point_nominal * (tseg + CAN_SYNC_SEG)) /
> 1000 - i;
> tseg2 = clamp(tseg2, btc->tseg2_min, btc->tseg2_max);
> tseg1 = tseg - tseg2;
> @@ -96,8 +95,8 @@ can_update_sample_point(const struct can_bittiming_const *btc,
> tseg2 = tseg - tseg1;
> }
>
> - sample_point = 1000 * (tseg + CAN_CALC_SYNC_SEG - tseg2) /
> - (tseg + CAN_CALC_SYNC_SEG);
> + sample_point = 1000 * (tseg + CAN_SYNC_SEG - tseg2) /
> + (tseg + CAN_SYNC_SEG);
> sample_point_error = abs(sample_point_nominal - sample_point);
>
> if (sample_point <= sample_point_nominal &&
> @@ -145,7 +144,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> /* tseg even = round down, odd = round up */
> for (tseg = (btc->tseg1_max + btc->tseg2_max) * 2 + 1;
> tseg >= (btc->tseg1_min + btc->tseg2_min) * 2; tseg--) {
> - tsegall = CAN_CALC_SYNC_SEG + tseg / 2;
> + tsegall = CAN_SYNC_SEG + tseg / 2;
>
> /* Compute all possible tseg choices (tseg=tseg1+tseg2) */
> brp = priv->clock.freq / (tsegall * bt->bitrate) + tseg % 2;
> @@ -223,7 +222,7 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
>
> /* real bitrate */
> bt->bitrate = priv->clock.freq /
> - (bt->brp * (CAN_CALC_SYNC_SEG + tseg1 + tseg2));
> + (bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
>
> return 0;
> }
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 791c452d98e1..77c3ea49b8fb 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -82,6 +82,21 @@ struct can_priv {
> #endif
> };
>
> +#define CAN_SYNC_SEG 1
> +
> +/*
> + * can_bit_time() - Duration of one bit
> + *
> + * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
> + * additional information.
> + *
> + * Return: the number of time quanta in one bit.
> + */
> +static inline int can_bit_time(struct can_bittiming *bt)

make it return an unsigned int
make the bt pointer const

> +{
> + return CAN_SYNC_SEG + bt->prop_seg + bt->phase_seg1 + bt->phase_seg2;
> +}
> +
> /*
> * get_can_dlc(value) - helper macro to cast a given data length code (dlc)
> * to u8 and ensure the dlc value to be max. 8 bytes.
>

tnx,
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 |

Attachments:

signature.asc (499.00 B)
OpenPGP digital signature

2020-10-04 11:12:29

by Marc Kleine-Budde

[permalink] [raw]

Subject: Re: [PATCH v3 5/7] can: dev: add a helper function to calculate the duration of one bit

On 10/4/20 1:06 PM, Marc Kleine-Budde wrote:
> On 10/2/20 5:41 PM, Vincent Mailhol wrote:
>> Rename macro CAN_CALC_SYNC_SEG to CAN_SYNC_SEG and make it available
>> through include/linux/can/dev.h
>>
>> Add an helper function can_bit_time() which returns the duration (in
>> time quanta) of one CAN bit.
>>
>> Rationale for this patch: the sync segment and the bit time are two
>> concepts which are defined in the CAN ISO standard. Device drivers for
>> CAN might need those.
>>
>> Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
>> additional information.
>>
>> Signed-off-by: Vincent Mailhol <[email protected]>

[...]

>> index 791c452d98e1..77c3ea49b8fb 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -82,6 +82,21 @@ struct can_priv {
>> #endif
>> };
>>
>> +#define CAN_SYNC_SEG 1
>> +
>> +/*
>> + * can_bit_time() - Duration of one bit
>> + *
>> + * Please refer to ISO 11898-1:2015, section 11.3.1.1 "Bit time" for
>> + * additional information.
>> + *
>> + * Return: the number of time quanta in one bit.
>> + */
>> +static inline int can_bit_time(struct can_bittiming *bt)
>
> make it return an unsigned int
> make the bt pointer const

I'll change this while applying the patch.

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 |

Attachments:

signature.asc (499.00 B)
OpenPGP digital signature

2020-10-04 12:10:05

> Just one header file for now :)

Thanks for the review, very constructive comments :)

I acknowledge all the trivial fixes (space, new line, // comments,
naming...), those will be fixed in v4 (will also review other files
for similar mistakes). In this reply, I will only focus on the points
which need explanations.

v4 will come a bit later.

> > +/* Threshold on consecutive CAN_STATE_ERROR_PASSIVE. If we receive
> > + * ES58X_CONSECUTIVE_ERR_PASSIVE_MAX times the event
> > + * ES58X_ERR_CRTL_PASSIVE in a row without any successful Rx or Tx,
> > + * we force the device to switch to CAN_STATE_BUS_OFF state.
> > + */
> > +#define ES58X_CONSECUTIVE_ERR_PASSIVE_MAX 254
>
> Does the device recover from bus off automatically or why is this needed?
>

Will be answered below together with your other question on
@err_passive_before_rtx_success of struct es58x_priv.

> > +
> > +enum es58x_physical_media {
> > + ES58X_MEDIA_HIGH_SPEED = 1,
> > + ES58X_MEDIA_FAULT_TOLERANT = 2
>
> You mean with FAULT_TOLERANT you mean ISO 11898-3? According to [1] they should
> be named low speed.

Two comments:
1/ Yes, this is "low speed". I did not know about the ISO 11898-3,
thanks for the hint.
2/ After double checking, this option is not supported by the devices
in scope of this driver (other devices of the ESxxx portfolio
might support it).
This option will be removed in v4.

> > +};
> > +
> > +enum es58x_samples_per_bit {
> > + ES58X_ONE_SAMPLE_PER_BIT = 1,
> > +
> > + /* Some CAN controllers do not support three samples per
> > + * bit. In this case the default value of one sample per bit
> > + * is used, even if the configuration is set to
> > + * ES58X_THREE_SAMPLES_PER_BIT.
> > + */
>
> Can you autodetect the controller and avoid announcing tripple sample mode to
> the driver framework?

Will be addressed in v4. Your remarks made me realized that some of
the controller modes might not have been announced correctly. Will
double check the other CAN_CTRLMODE_* as well.

> > + ES58X_THREE_SAMPLES_PER_BIT = 2
> > +};
> > +
> > +enum es58x_sync_edge {
> > + /* ISO CAN specification defines the use of a single edge
> > + * synchronization. The synchronization should be done on
> > + * recessive to dominant level change.
> > + */
> > + ES58X_SINGLE_SYNC_EDGE = 1,
> > +
> > + /* In addition to the ISO CAN specification, a double
> > + * synchronization is also supported: recessive to dominant
> > + * level change and dominant to recessive level change.
> > + */
> > + ES58X_DUAL_SYNC_EDGE = 2
>
> >We don't have a setting in the CAN framework for this....

The idea here was just to let know people that the option exists so
that if someone needs the feature one day, he or she can hack the
driver for his or her own use.

Is it OK to keep it (maybe with a comment such as "not implemented in
this driver") or should it be simply removed?

There are other similar references in other files. Will change these
accordingly to your answer on above question.

> > +/**
> > + * struct es58x_abstracted_can_frame - Common structure to hold can
> > + * frame information.
>
> why do you have an itermediate can frame format? We have the struct can_frame
> and the skb for this.

The goal of this structure was to factorize code when calculating the
CAN flags. I will try to rethink this part in v4.

> > +union es58x_urb_cmd {
> > + u8 raw_cmd[0];
>
> I have to polish my C, what's an empty array in the beginning of a struct?

This is not a struct but a union (it would indeed make no sense at the
beginning of a struct).

Because it is in a union, the order of the fields does not make a
difference (if you prefer this to be at the end, I can fix it).

This field is used to cast the union to an u8 array. Because the
length is unknown it is declared as empty.

For reference, I could at least find a few other references of union
starting with an empty array in the kernel. One example here:
https://elixir.bootlin.com/linux/latest/source/include/linux/bpf.h#L821

>
> > +/**
> > + * struct es58x_priv - All information specific to a can channel.
> > + * @can: struct can_priv must be the first member (Socket CAN relies
> > + * on the fact that function netdev_priv() returns a pointer to
> > + * a struct can_priv).
> > + * @es58x_dev: pointer to the corresponding ES58X device.
> > + * @echo_skb_spinlock: Spinlock to protect the access to the echo skb
> > + * FIFO.
> > + * @current_packet_idx: Keeps track of the packet indexes.
> > + * @echo_skb_tail_idx: beginning of the echo skb FIFO, i.e. index of
> > + * the first element.
> > + * @echo_skb_head_idx: end of the echo skb FIFO plus one, i.e. first
> > + * free index.
> > + * @num_echo_skb: actual number of elements in the FIFO. Thus, the end
> > + * of the FIFO is echo_skb_head = (echo_skb_tail_idx +
> > + * num_echo_skb) % can.echo_skb_max.
> > + * @tx_urb: Used as a buffer to concatenate the TX messages and to do
> > + * a bulk send. Please refer to es58x_start_xmit() for more
> > + * details.
> > + * @tx_can_msg_is_fd: false: all messages in @tx_urb are non-FD CAN,
> > + * true: all messages in @tx_urb are CAN-FD. Rationale: ES58X FD
> > + * devices do not allow to mix standard and FD CAN in one single
> > + * bulk transmission.
> > + * @tx_can_msg_cnt: Number of messages in @tx_urb.
> > + * @err_passive_before_rtx_success: The ES58X device might enter in a
> > + * state in which it keeps alternating between error passive
> > + * and active state. This counter keeps track of the number of
> > + * error passive and if it gets bigger than
> > + * ES58X_CONSECUTIVE_ERR_PASSIVE_MAX, es58x_rx_err_msg() will
> > + * force the status to bus-off.
>
> Is this a bug or a feature?

This is a bug of the device.

Rationale: According to ISO 11898-1, paragraph 12.1.4.2 "Error
counting", the two only possible scenarios to decrements the error
counter are "After the successful transmission of a frame" (paragraph
g) and "After the successful reception of a frame" (paragraph h).

Here, the device switch from error passive state to error active state
without any successful Tx or Rx of a frame. This means that the error
counter does not behave as stipulated in the ISO.

When the issue occurs, the only solution would be to set down the
network. Forcing the bus off allows at least the user to recover (with
restart or restart-ms).

For information, this issue was only witnessed when the device is
trying to send frames on a bus which is already at 100% load.

Example to reproduce: have can0 and can1 on the same bus and do:
cangen -g0 can0
cangen -g0 can1

Note: development team of the device's firmware was informed of this
issue and will consider how to fix it.

> > +#define ES58X_SIZEOF_ES58X_DEVICE(es58x_dev_param) \
> > + (offsetof(struct es58x_device, rx_cmd_buf) + \
> > + (es58x_dev_param)->rx_urb_cmd_max_len)
>
> can this be made a static inline?

Yes. Will be fixed in v4.

> > + * Must be a macro in order to retrieve the actual size using
> > + * sizeof(). Can be use with any of the messages which have a fixed
> > + * length. Check for an exact match of the size.
>
> You can provide an outer macro that does the sizeof() and then calls the a
> normal (static inline) function to do the actual work. Applied to the next 3 macros.

OK. Will be fixed in v4.

> > +#define ES58X_SIZEOF_URB_CMD(es58x_urb_cmd_type, msg_field) \
> > + (offsetof(es58x_urb_cmd_type, raw_msg) \
> > + + sizeof_field(es58x_urb_cmd_type, msg_field) \
> > + + sizeof_field(es58x_urb_cmd_type, \
> > + reserved_for_crc16_do_not_use))
>
> static inline?

Sorry but this one can not be converted into a static inline: the
first argument is a type (that will become the first argument of
offsetof() and sizeof_field()).

One more time, thank you for your time and your review!

Yours sincerely,
Vincent Mailhol