linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* More flags for logging
@ 2021-05-03 10:02 Marc Kleine-Budde
  2021-05-03 10:08 ` Marc Kleine-Budde
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Kleine-Budde @ 2021-05-03 10:02 UTC (permalink / raw)
  To: linux-can; +Cc: kayoub5

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

Hi,

on github Ayoub Kaanich writes:

https://github.com/linux-can/can-utils/issues/291

-------->8-------->8-------->8-------->8-------->8-------->8--------

The SocketCAN API is a great initiative for standardizing the CAN
interface API. This request tries to extend this initiative for more use
cases.

Context:

The SocketCAN was adopted by libpcap and tcpdump as the standard format
for logging CAN Frames in PCAP and PCAP-NG. See:

https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-socketcan.c
https://www.wireshark.org/docs/dfref/c/can.html

Problem:
Applications that perform logging, usually need more details that normal
applications, for the sake of debugging later on. Flags needs to be
reserved/allocated in the SocketCAN API, so that logging applications
can make use of them in the PCAP format. The flags does not need
necessary need to be implemented by SocketCAN, they just need to be
marked as reserved for such use case.

Needed Flags:
FDF Flag

- Since CAN Frames and CAN-FD frames can co-exist in the same bus,
  logging application needs to know if a normal CAN Frame was
  transmitted on a CAN-FD bus, namely was the FDF bit set or not.
  
 ACK bit in data frame
- Some logging hardware can act as a "spy", meaning that it records CAN
  Frames, but does not set the ACK bit
- A way to know for a given frame (FD or not), was the ACK bit set or
  not.
- Current API allow detecting missing ACK, but it does not report what
  Frame had a missing ACK.

-------->8-------->8-------->8-------->8-------->8-------->8--------

regards,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 10:02 More flags for logging Marc Kleine-Budde
@ 2021-05-03 10:08 ` Marc Kleine-Budde
  2021-05-03 15:02   ` Vincent MAILHOL
  2021-05-04  8:49   ` Oliver Hartkopp
  0 siblings, 2 replies; 20+ messages in thread
From: Marc Kleine-Budde @ 2021-05-03 10:08 UTC (permalink / raw)
  To: linux-can; +Cc: kayoub5

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

On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
> The SocketCAN API is a great initiative for standardizing the CAN
> interface API. This request tries to extend this initiative for more use
> cases.
> 
> Context:
> 
> The SocketCAN was adopted by libpcap and tcpdump as the standard format
> for logging CAN Frames in PCAP and PCAP-NG. See:
> 
> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-socketcan.c
> https://www.wireshark.org/docs/dfref/c/can.html
> 
> Problem:
> Applications that perform logging, usually need more details that normal
> applications, for the sake of debugging later on. Flags needs to be
> reserved/allocated in the SocketCAN API, so that logging applications
> can make use of them in the PCAP format. The flags does not need
> necessary need to be implemented by SocketCAN, they just need to be
> marked as reserved for such use case.
> 
> Needed Flags:
> FDF Flag
> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>   logging application needs to know if a normal CAN Frame was
>   transmitted on a CAN-FD bus, namely was the FDF bit set or not.

I think someone asked for that some time ago. But that was never
mainlined. I'll look for that old mail.

> ACK bit in data frame
> - Some logging hardware can act as a "spy", meaning that it records CAN
>   Frames, but does not set the ACK bit
> - A way to know for a given frame (FD or not), was the ACK bit set or
>   not.
> - Current API allow detecting missing ACK, but it does not report what
>   Frame had a missing ACK.

This means the driver has to set a flag if it's configured in
listen-only mode. That should be possible.

I think we can make use of flags in the struct canfd_frame for this:

| struct canfd_frame {
| 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
| 	__u8    len;     /* frame payload length in byte */
| 	__u8    flags;   /* additional flags for CAN FD */
| 	__u8    __res0;  /* reserved / padding */
| 	__u8    __res1;  /* reserved / padding */
| 	__u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
| };

The struct can_frame doesn't have the flags member yet, but we can add
it there.

regards,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 10:08 ` Marc Kleine-Budde
@ 2021-05-03 15:02   ` Vincent MAILHOL
  2021-05-03 15:43     ` Marc Kleine-Budde
                       ` (2 more replies)
  2021-05-04  8:49   ` Oliver Hartkopp
  1 sibling, 3 replies; 20+ messages in thread
From: Vincent MAILHOL @ 2021-05-03 15:02 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: linux-can, kayoub5

On Mon. 3 Mai 2021 at 19:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
> > The SocketCAN API is a great initiative for standardizing the CAN
> > interface API. This request tries to extend this initiative for more use
> > cases.
> >
> > Context:
> >
> > The SocketCAN was adopted by libpcap and tcpdump as the standard format
> > for logging CAN Frames in PCAP and PCAP-NG. See:
> >
> > https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
> > https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-socketcan.c
> > https://www.wireshark.org/docs/dfref/c/can.html
> >
> > Problem:
> > Applications that perform logging, usually need more details that normal
> > applications, for the sake of debugging later on. Flags needs to be
> > reserved/allocated in the SocketCAN API, so that logging applications
> > can make use of them in the PCAP format. The flags does not need
> > necessary need to be implemented by SocketCAN, they just need to be
> > marked as reserved for such use case.
> >
> > Needed Flags:
> > FDF Flag
> > - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
> >   logging application needs to know if a normal CAN Frame was
> >   transmitted on a CAN-FD bus, namely was the FDF bit set or not.
>
> I think someone asked for that some time ago. But that was never
> mainlined. I'll look for that old mail.

The current API compares the size of the received structure with
the MTU to determine whether the message is classical CAN or CAN
FD.  If the struct can_frame and canfd_frame are stored as
is (including the padding at the end of the data array), then the
information is maintained.

Personally, I would have liked to also have a FDF flag in Socket
CAN userland API because I think it would be more intuitive than
the MTU solution. So I am open to this option and would also be
happy to see the flag implemented in Socket CAN (not only
reserved).

But, I also understand that it will create redundancy.

> > ACK bit in data frame
> > - Some logging hardware can act as a "spy", meaning that it records CAN
> >   Frames, but does not set the ACK bit
> > - A way to know for a given frame (FD or not), was the ACK bit set or
> >   not.
> > - Current API allow detecting missing ACK, but it does not report what
> >   Frame had a missing ACK.
>
> This means the driver has to set a flag if it's configured in
> listen-only mode. That should be possible.

For my understanding, when a controller does not see the ACK bit,
it stops the transmission and sends an error flag. For this
reason, the frame is not received and thus simply does not appear
in the log.

So this issue is not only about adding an ACK flag but is about
implementing a solution to report on which frame a given error
flag occured (not specific to ACK errors).

And to clarify the "spy mode", if all the receivers on the bus
are spies, then the ACK bit is never set and thus the
transmission should always result in an ACK error flag. If at
least one receiver is normal (not spy) it should send a dominant
ACK bit and thus all nodes (including the spies) see the flag as
dominant. In other terms, a received CAN frame has the ACK flag
set.

> I think we can make use of flags in the struct canfd_frame for this:
>
> | struct canfd_frame {
> |       canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
> |       __u8    len;     /* frame payload length in byte */
> |       __u8    flags;   /* additional flags for CAN FD */
> |       __u8    __res0;  /* reserved / padding */
> |       __u8    __res1;  /* reserved / padding */
> |       __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> | };
>
> The struct can_frame doesn't have the flags member yet, but we can add
> it there.
>
> regards,
> 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 |

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 15:02   ` Vincent MAILHOL
@ 2021-05-03 15:43     ` Marc Kleine-Budde
       [not found]     ` <DBBPR03MB70828377F51A1747B4E9E6529D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
  2021-05-04 10:07     ` Kurt Van Dijck
  2 siblings, 0 replies; 20+ messages in thread
From: Marc Kleine-Budde @ 2021-05-03 15:43 UTC (permalink / raw)
  To: Vincent MAILHOL, Thomas Kopp; +Cc: linux-can, kayoub5

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

On 04.05.2021 00:02:49, Vincent MAILHOL wrote:
> > > ACK bit in data frame
> > > - Some logging hardware can act as a "spy", meaning that it records CAN
> > >   Frames, but does not set the ACK bit
> > > - A way to know for a given frame (FD or not), was the ACK bit set or
> > >   not.
> > > - Current API allow detecting missing ACK, but it does not report what
> > >   Frame had a missing ACK.
> >
> > This means the driver has to set a flag if it's configured in
> > listen-only mode. That should be possible.
>
> For my understanding, when a controller does not see the ACK bit,
> it stops the transmission and sends an error flag. For this
> reason, the frame is not received and thus simply does not appear
> in the log.

Hmmm. That's not correct for the mcp251xfd in RX-listen only mode:

| (2021-05-03 17:11:28.493992)  mcp251xfd0  RX - -  222   [8]  05 00 00 00 00 00 00 00
| (2021-05-03 17:11:28.994109)  mcp251xfd0  RX - -  222   [8]  06 00 00 00 00 00 00 00
| (2021-05-03 17:11:29.494298)  mcp251xfd0  RX - -  222   [8]  07 00 00 00 00 00 00 00
| (2021-05-03 17:11:29.994510)  mcp251xfd0  RX - -  222   [8]  08 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.494635)  mcp251xfd0  RX - -  222   [8]  09 00 00 00 00 00 00 00

unplugged 3rd device here (that's used to send the ACK)

| (2021-05-03 17:11:30.998388)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.998662)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.998936)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.999210)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.999484)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:30.999758)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.000032)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.000306)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.000580)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.000854)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.001128)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.001402)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.001676)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.001950)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.002224)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.002498)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.002772)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.003046)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.003320)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00
| (2021-05-03 17:11:31.003594)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00


---

The flexcan (can0) does behave differently:

| (2021-05-03 17:16:43.004939)        can0  RX - -  222   [8]  08 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:43.005846)  mcp251xfd0  RX - -  222   [8]  08 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:43.505052)        can0  RX - -  222   [8]  09 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:43.505729)  mcp251xfd0  RX - -  222   [8]  09 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:44.005193)        can0  RX - -  222   [8]  0A 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:44.005856)  mcp251xfd0  RX - -  222   [8]  0A 00 00 00 00 00 00 00                                                                                                                                                                           

unplugged 3rd device here (that's used to send the ACK)

| (2021-05-03 17:16:44.507358)  mcp251xfd0  RX - -  222   [8]  0B 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:44.507399)  mcp251xfd0  RX - -  222   [8]  0B 00 00 00 00 00 00 00                                                                                                                                                                           
| (2021-05-03 17:16:44.508142)  mcp251xfd0  RX - -  222   [8]  0B 00 00 00 00 00 00 00                                                                                                                                                                           

Thomas, is that worth investigating in your side?

regards,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
       [not found]     ` <DBBPR03MB70828377F51A1747B4E9E6529D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
@ 2021-05-03 15:47       ` Marc Kleine-Budde
       [not found]         ` <DBBPR03MB7082F029173018680E5D869C9D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
  2021-05-04 10:10         ` More flags for logging Kurt Van Dijck
  0 siblings, 2 replies; 20+ messages in thread
From: Marc Kleine-Budde @ 2021-05-03 15:47 UTC (permalink / raw)
  To: Ayoub Kaanich; +Cc: Vincent MAILHOL, linux-can

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

On 03.05.2021 15:31:34, Ayoub Kaanich wrote:
> For the ack bit, I think we could go with the bit being a “NO_ACK”
> bit, so a spy listener will set it to 1, if it receives a CAN frame,
> but it does not see any node in the bus acknowledging it. This way we
> preserve backward compatibility.

As Vincent pointed out, the device should not receive the CAN frame that
has not been acked as I do on the flexcan.

And even on the mcp251xfd, where I receive the CAN frame, there's no way
to tell if this frame has been acked or not.

> For the FDF flag, check the MTU is not an option here, since this flag
> is needed to detect normal CAN frames being transmitted on a CAN-FD
> bus (The bus could be configured as CAN-FD, but the received frame
> could be a normal CAN frame)
> 
> If we are adding flags to can_frame, could we make sure it’s on the
> same byte offset as the flags of can_fd_frame, since PCAP format have
> no concept of MTU or struct size.

Sure - Do struct can_fd_frames have a different ARP header type?

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
       [not found]         ` <DBBPR03MB7082F029173018680E5D869C9D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
@ 2021-05-03 21:46           ` Vincent MAILHOL
  2021-05-04  5:40             ` Patrick Menschel
  2021-05-04  7:48             ` mcp251xfd receiving non ACKed frames (was: Re: More flags for logging) Marc Kleine-Budde
  0 siblings, 2 replies; 20+ messages in thread
From: Vincent MAILHOL @ 2021-05-03 21:46 UTC (permalink / raw)
  To: Ayoub Kaanich; +Cc: Marc Kleine-Budde, linux-can

On Tue. 4 Mai 2021 at 01:26, Ayoub Kaanich <kayoub5@live.com> wrote:
>
> Hi,
>
> > As Vincent pointed out, the device should not receive the CAN frame that
> > has not been acked.
>
> We are not talking about traditional off-the-shelf devices.
> The use case here is for specialized hardware in CAN logging, that dumps whatever happens in the network to a log file.
> Example: https://technica-engineering.de/en/produkt/capture-module-can-combo/

I am not used to those devices. By "whatever happens in the
network" do you mean that any cancelled transmission due to an
error flag (including but not limited to ACK error flags) is
logged?  How do you plan to use the struct can{,fd}_frame to
record those incomplete transmissions? For example, how do you
plan to use Socket CAN API to log a frame cancelled due to a bit
error in the middle of the data segment (i.e. sent a recessive
bit but monitored dominant one)?


> > Sure - Do struct can_fd_frames have a different ARP header type?
>
> I assume you mean “Pcap Linklayer” or “DLT”, and the answer is no.
> can_fd_frames and can_frames share the same type, and they only keep enough bytes stored in the file to cover used data.
> Unused data bytes are not store in the file to save disk space.
> So there is no way in PCAP format, to tell them apart.

This is my point. Here you took the *decision* to save disk space
instead of blindly following the Socket CAN "philosophy" of using
the MTU to differentiate between classical and FD frames. But you
are treating the struct can{,fd}_frame as if it had a flexible
array for the dada field which is not the case. So you are having
this issue because you are using a modified version of the Socket
CAN API to begin with.
I am not saying this is a bad choice. I do like this approach :)

Back to the Socket CAN API itself, one reason I would also like
the kernel to use such an FDF flag is as follows: on other
ethernet protocols, the developper does not need to allocate a
message buffer of the size of the MTU. It is fine to only
allocate a buffer able to contain the size of frames expected by
the application. In Socket CAN, even if I have an application
which only sends and receives FD frames with a data length of 8,
I am still forced to use an additional 56 bytes because of this
MTU design. This is the same as your PCAP issue: the current
Socket CAN API does not allow you to save the bytes.

I do not think this has currently a big overhead. But I am still
curious to see the figures: does anyone know how much ressources
are lost by memcpying those padding bytes? However, with the
upcoming CAN XL (which will have an MTU of 2KiB), I think we will
eventually need to use an approach in which the MTU and the type
of the frames get decorelated (i.e. make the data array a
flexible size array).


> And even on the mcp251xfd, where I receive the CAN frame, there's no way
> to tell if this frame has been acked or not.

The mcp251xfd behavior is interesting. Do you also receive the
ACK error flag? Does the controller retry to send the frame until
it gets acknowledged? Are you still able to send frames and
receive the echo if there is a single node on the network?


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 21:46           ` Vincent MAILHOL
@ 2021-05-04  5:40             ` Patrick Menschel
  2021-05-04  7:48             ` mcp251xfd receiving non ACKed frames (was: Re: More flags for logging) Marc Kleine-Budde
  1 sibling, 0 replies; 20+ messages in thread
From: Patrick Menschel @ 2021-05-04  5:40 UTC (permalink / raw)
  To: Vincent MAILHOL, Ayoub Kaanich; +Cc: Marc Kleine-Budde, linux-can

Hi,

I think this is an interesting topic and would like to point out that
only half of this discussion is reaching linux-can mailing list.
The parts that originate from Ayoub Kaanich <kayoub5@live.com> are
missing as far as I can tell. Maybe he did not register for linux-can
and his emails are dropped by vger.kernel.org.

If there are good ideas for a new can logging file format, I'd be
interested myself.

Thank you and Best Regards,
Patrick Menschel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* mcp251xfd receiving non ACKed frames (was: Re: More flags for logging)
  2021-05-03 21:46           ` Vincent MAILHOL
  2021-05-04  5:40             ` Patrick Menschel
@ 2021-05-04  7:48             ` Marc Kleine-Budde
  2021-05-04 12:22               ` Vincent MAILHOL
  1 sibling, 1 reply; 20+ messages in thread
From: Marc Kleine-Budde @ 2021-05-04  7:48 UTC (permalink / raw)
  To: Vincent MAILHOL; +Cc: Ayoub Kaanich, linux-can, Thomas Kopp

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

On 04.05.2021 06:46:17, Vincent MAILHOL wrote:
> > And even on the mcp251xfd, where I receive the CAN frame, there's no way
> > to tell if this frame has been acked or not.

The test setup is:

                    flexcan (listen only)
                             |
                             |
   PEAK PCAN-USB FD ---------+--------- mcp2518fd (listen only)
        (sender)             |
                             |
               candlelight (going to be unplugged)

pcan-usb: sending CAN frames
flexcan: receiving CAN frames - but controller in listen only mode
mcp2518fd: receiving CAN frames - but controller in listen only mode
candlelight: receiving CAN frames - first attached, then detached

> The mcp251xfd behavior is interesting. Do you also receive the ACK
> error flag?

In my tests from yesterday neither the flexcan nor the mcp2518fd had bus
error reporting enabled. So I haven't noticed any ACK errors on the
mcp2518fd nor the flexcan.

I just repeated the test with bus error reporting enabled:

On the flexcan I receive _only_ these errors (repeating) with
candlelight detached:

| (2021-05-04 09:00:30.407709)        can0  RX - -  20000088   [8]  00 00 08 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{tx-dominant-bit-error}{}}
|        bus-error


On the mcp2518fd I see these errors:

| (2021-05-04 09:05:00.594321)  mcp251xfd0  RX - -  222   [8]  4A 00 00 00 00 00 00 00
| (2021-05-04 09:05:01.094418)  mcp251xfd0  RX - -  222   [8]  4B 00 00 00 00 00 00 00
| (2021-05-04 09:05:01.594577)  mcp251xfd0  RX - -  222   [8]  4C 00 00 00 00 00 00 00
...unplug candlelight here...
| (2021-05-04 09:05:02.094878)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.095589)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.096263)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.096934)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.097596)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.098261)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{frame-format-error}{}}
|        bus-error
| (2021-05-04 09:05:02.099035)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.099054)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.099603)  mcp251xfd0  RX - -  20000088   [8]  00 00 00 00 00 00 00 00   ERRORFRAME
|        protocol-violation{{}{}}
|        bus-error

from here now only RX frames, no error frames

| (2021-05-04 09:05:02.100540)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.100570)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.100583)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.100593)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
| (2021-05-04 09:05:02.101326)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00

... and repeating.


Here a short dump of the mcp2518fd registers:

| INT: intf(0x01c)=0xbf1a0806
|                 IE      IF      IE & IF
|         IVMI    x                       Invalid Message Interrupt
|         WAKI                            Bus Wake Up Interrupt
|         CERRI   x                       CAN Bus Error Interrupt
|         SERRI   x                       System Error Interrupt
|         RXOVI   x       x       x       Receive FIFO Overflow Interrupt
|         TXATI   x                       Transmit Attempt Interrupt
|         SPICRCI x                       SPI CRC Error Interrupt
|         ECCI    x                       ECC Error Interrupt
|         TEFI    x                       Transmit Event FIFO Interrupt
|         MODI    x                       Mode Change Interrupt
|         TBCI            x               Time Base Counter Interrupt
|         RXI     x       x       x       Receive FIFO Interrupt
|         TXI                             Transmit FIFO Interrupt

Note: there is no invalid message interrupt pending

| TREC: trec(0x034)=0x00000000
|             TXBO                Transmitter in Bus Off State
|             TXBP                Transmitter in Error Passive State
|             RXBP                Receiver in Error Passive State
|           TXWARN                Transmitter in Error Warning State
|           RXWARN                Receiver in Error Warning State
|            EWARN                Transmitter or Receiver is in Error Warning State
|              TEC =   0          Transmit Error Counter
|              REC =   0          Receive Error Counter
| 
| BDIAG0: bdiag0(0x038)=0x00000010
|         DTERRCNT =   0          Data Bit Rate Transmit Error Counter
|         DRERRCNT =   0          Data Bit Rate Receive Error Counter
|         NTERRCNT =   0          Nominal Bit Rate Transmit Error Counter
|         NRERRCNT =  16          Nominal Bit Rate Receive Error Counter
| 
| BDIAG1: bdiag1(0x03c)=0x0000dd4b
|            DLCMM                DLC Mismatch
|              ESI                ESI flag of a received CAN FD message was set
|          DCRCERR                Data CRC Error
|         DSTUFERR                Data Bit Stuffing Error
|         DFORMERR                Data Format Error
|         DBIT1ERR                Data BIT1 Error
|         DBIT0ERR                Data BIT0 Error
|          TXBOERR                Device went to bus-off (and auto-recovered)
|          NCRCERR                CRC Error
|         NSTUFERR                Bit Stuffing Error
|         NFORMERR                Format Error
|          NACKERR                Transmitted message was not acknowledged
|         NBIT1ERR                Bit1 Error
|         NBIT0ERR                Bit0 Error
|         EFMSGCNT = 56651                Error Free Message Counter

> Does the controller retry to send the frame until it gets
> acknowledged?

Yes - as it should.

> Are you still able to send frames and receive the echo if there is a
> single node on the network?

No - But the peak driver/hw has some limitations:

The peak driver doesn't have TX complete signaling, it send the echo
after sending the TX CAN frame via USB. And the peak controller seems to
buffer quite a lot TX CAN frames, so it looks for the first ~72 frames
like the bus is still working.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 10:08 ` Marc Kleine-Budde
  2021-05-03 15:02   ` Vincent MAILHOL
@ 2021-05-04  8:49   ` Oliver Hartkopp
       [not found]     ` <DBBPR03MB7082C7E8FD22D0CA8C2DA99D9D5A9@DBBPR03MB7082.eurprd03.prod.outlook.com>
       [not found]     ` <DBBPR03MB70824BB82F7BB335928BE36A9D2F9@DBBPR03MB7082.eurprd03.prod.outlook.com>
  1 sibling, 2 replies; 20+ messages in thread
From: Oliver Hartkopp @ 2021-05-04  8:49 UTC (permalink / raw)
  To: Marc Kleine-Budde, linux-can; +Cc: kayoub5



On 03.05.21 12:08, Marc Kleine-Budde wrote:
> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>> The SocketCAN API is a great initiative for standardizing the CAN
>> interface API. This request tries to extend this initiative for more use
>> cases.
>>
>> Context:
>>
>> The SocketCAN was adopted by libpcap and tcpdump as the standard format
>> for logging CAN Frames in PCAP and PCAP-NG. See:
>>
>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-socketcan.c
>> https://www.wireshark.org/docs/dfref/c/can.html
>>
>> Problem:
>> Applications that perform logging, usually need more details that normal
>> applications, for the sake of debugging later on. Flags needs to be
>> reserved/allocated in the SocketCAN API, so that logging applications
>> can make use of them in the PCAP format. The flags does not need
>> necessary need to be implemented by SocketCAN, they just need to be
>> marked as reserved for such use case.
>>
>> Needed Flags:
>> FDF Flag
>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>    logging application needs to know if a normal CAN Frame was
>>    transmitted on a CAN-FD bus, namely was the FDF bit set or not.
> 
> I think someone asked for that some time ago. But that was never
> mainlined. I'll look for that old mail.
> 

When you display CAN and CAN FD frames in Wireshark they are displayed 
as different "protocols" - as they also have different ethtypes.

So the difference is provided by the 'protocol' field. Or did I miss 
something?

Regards,
Oliver

>> ACK bit in data frame
>> - Some logging hardware can act as a "spy", meaning that it records CAN
>>    Frames, but does not set the ACK bit
>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>    not.
>> - Current API allow detecting missing ACK, but it does not report what
>>    Frame had a missing ACK.
> 
> This means the driver has to set a flag if it's configured in
> listen-only mode. That should be possible.
> 
> I think we can make use of flags in the struct canfd_frame for this:
> 
> | struct canfd_frame {
> | 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
> | 	__u8    len;     /* frame payload length in byte */
> | 	__u8    flags;   /* additional flags for CAN FD */
> | 	__u8    __res0;  /* reserved / padding */
> | 	__u8    __res1;  /* reserved / padding */
> | 	__u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> | };
> 
> The struct can_frame doesn't have the flags member yet, but we can add
> it there.
> 
> regards,
> Marc
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: More flags for logging
       [not found]       ` <AM8PR08MB5698886555F8531B6CF65982B75A9@AM8PR08MB5698.eurprd08.prod.outlook.com>
@ 2021-05-04  9:49         ` Ayoub Kaanich
  2021-05-04 10:13           ` Oliver Hartkopp
  0 siblings, 1 reply; 20+ messages in thread
From: Ayoub Kaanich @ 2021-05-04  9:49 UTC (permalink / raw)
  To: socketcan, mkl, linux-can

Hi,

(I changed the email I use, since the old one did not support sending plain text emails.)

There seems to be a mis-conception about how WireShark/Libpcap handle SocketCAN:

There is no ethertype involved, the data of the frame will directly start with the CAN ID.

The socket CAN in PCAP format pre-date CAN-FD (Developed in 2011), so at the time, there was only "CAN" (Introduced in Oct, 2009).
See https://github.com/the-tcpdump-group/libpcap/commit/86ed15cc8b4bd7dc1458559108e66a07ec6721ec
The introduction of CAN-FD later on did not change that.

Libpcap library maps both LINUX_SLL_P_CAN and LINUX_SLL_P_CANFD (they are treated identically the same way) to DLT_CAN_SOCKETCAN and copy bytes as is (the only change is changing the endianness if ID if needed).

The MTU solution still not does provide a fix for the case of a normal CAN message being transported over a CAN-FD bus.
In this case, application cannot tell if frame was physically transported as CAN or CAN-FD.
See https://noelscher.com/blog/posts/compatibility-of-can-and-can-fd/

Best Regards.

Ayoub Kaanich.

-----Original Message-----

From: mailto:socketcan@hartkopp.net
Sent: Tuesday, May 4, 2021 10:49 AM
To: mailto:mkl@pengutronix.de; mailto:linux-can@vger.kernel.org
Cc: mailto:kayoub5@live.com
Subject: Re: More flags for logging



On 03.05.21 12:08, Marc Kleine-Budde wrote:
> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>> The SocketCAN API is a great initiative for standardizing the CAN 
>> interface API. This request tries to extend this initiative for more 
>> use cases.
>>
>> Context:
>>
>> The SocketCAN was adopted by libpcap and tcpdump as the standard 
>> format for logging CAN Frames in PCAP and PCAP-NG. See:
>>
>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/pa
>> cket-socketcan.c https://www.wireshark.org/docs/dfref/c/can.html
>>
>> Problem:
>> Applications that perform logging, usually need more details that 
>> normal applications, for the sake of debugging later on. Flags needs 
>> to be reserved/allocated in the SocketCAN API, so that logging 
>> applications can make use of them in the PCAP format. The flags does 
>> not need necessary need to be implemented by SocketCAN, they just 
>> need to be marked as reserved for such use case.
>>
>> Needed Flags:
>> FDF Flag
>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>    logging application needs to know if a normal CAN Frame was
>>    transmitted on a CAN-FD bus, namely was the FDF bit set or not.
> 
> I think someone asked for that some time ago. But that was never 
> mainlined. I'll look for that old mail.
> 

When you display CAN and CAN FD frames in Wireshark they are displayed as different "protocols" - as they also have different ethtypes.

So the difference is provided by the 'protocol' field. Or did I miss something?

Regards,
Oliver

>> ACK bit in data frame
>> - Some logging hardware can act as a "spy", meaning that it records 
>>CAN
>>    Frames, but does not set the ACK bit
>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>    not.
>> - Current API allow detecting missing ACK, but it does not report 
>>what
>>    Frame had a missing ACK.
> 
> This means the driver has to set a flag if it's configured in 
> listen-only mode. That should be possible.
> 
> I think we can make use of flags in the struct canfd_frame for this:
> 
> | struct canfd_frame {
> |      canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
> |      __u8    len;     /* frame payload length in byte */
> |      __u8    flags;   /* additional flags for CAN FD */
> |      __u8    __res0;  /* reserved / padding */
> |      __u8    __res1;  /* reserved / padding */
> |      __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));  };
> 
> The struct can_frame doesn't have the flags member yet, but we can add 
> it there.
> 
> regards,
> Marc
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 15:02   ` Vincent MAILHOL
  2021-05-03 15:43     ` Marc Kleine-Budde
       [not found]     ` <DBBPR03MB70828377F51A1747B4E9E6529D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
@ 2021-05-04 10:07     ` Kurt Van Dijck
  2 siblings, 0 replies; 20+ messages in thread
From: Kurt Van Dijck @ 2021-05-04 10:07 UTC (permalink / raw)
  To: Vincent MAILHOL; +Cc: Marc Kleine-Budde, linux-can, kayoub5

On Tue, 04 May 2021 00:02:49 +0900, Vincent MAILHOL wrote:
> On Mon. 3 Mai 2021 at 19:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
> > > ACK bit in data frame
> > > - Some logging hardware can act as a "spy", meaning that it records CAN
> > >   Frames, but does not set the ACK bit
> > > - A way to know for a given frame (FD or not), was the ACK bit set or
> > >   not.
> > > - Current API allow detecting missing ACK, but it does not report what
> > >   Frame had a missing ACK.
> >
> > This means the driver has to set a flag if it's configured in
> > listen-only mode. That should be possible.
> 
> For my understanding, when a controller does not see the ACK bit,
> it stops the transmission and sends an error flag. For this
> reason, the frame is not received and thus simply does not appear
> in the log.

I should correct you here.
When the controller does not see the ACK bit, the (transmitting)
controller will just repeat the frame until ...

Kind regards,
Kurt

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-03 15:47       ` Marc Kleine-Budde
       [not found]         ` <DBBPR03MB7082F029173018680E5D869C9D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
@ 2021-05-04 10:10         ` Kurt Van Dijck
  1 sibling, 0 replies; 20+ messages in thread
From: Kurt Van Dijck @ 2021-05-04 10:10 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Ayoub Kaanich, Vincent MAILHOL, linux-can

On Mon, 03 May 2021 17:47:49 +0200, Marc Kleine-Budde wrote:
> On 03.05.2021 15:31:34, Ayoub Kaanich wrote:
> > For the ack bit, I think we could go with the bit being a “NO_ACK”
> > bit, so a spy listener will set it to 1, if it receives a CAN frame,
> > but it does not see any node in the bus acknowledging it. This way we
> > preserve backward compatibility.
> 
> As Vincent pointed out, the device should not receive the CAN frame that
> has not been acked as I do on the flexcan.
> 
> And even on the mcp251xfd, where I receive the CAN frame, there's no way
> to tell if this frame has been acked or not.

in listen-only mode, it's debatable if you should see an ack.
If there's only 1 active node on the bus, you would not be able to probe
the bitrate using listen-only mode, unless you accept CAN frames that
are only miss the ACK bit.

Kurt

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-04  9:49         ` Ayoub Kaanich
@ 2021-05-04 10:13           ` Oliver Hartkopp
  2021-05-04 13:58             ` Ayoub Kaanich
  0 siblings, 1 reply; 20+ messages in thread
From: Oliver Hartkopp @ 2021-05-04 10:13 UTC (permalink / raw)
  To: Ayoub Kaanich, mkl, linux-can

Hi Ayoub,

On 04.05.21 11:49, Ayoub Kaanich wrote:

> There seems to be a mis-conception about how WireShark/Libpcap handle SocketCAN:
> 
> There is no ethertype involved, the data of the frame will directly start with the CAN ID.
> 
> The socket CAN in PCAP format pre-date CAN-FD (Developed in 2011), so at the time, there was only "CAN" (Introduced in Oct, 2009).
> See https://github.com/the-tcpdump-group/libpcap/commit/86ed15cc8b4bd7dc1458559108e66a07ec6721ec
> The introduction of CAN-FD later on did not change that.

Ah, now I got the problem.

I was heavily involved in introducing CAN FD into Wireshark here:

https://github.com/wireshark/wireshark/commit/7fad354a3e379382368cd1ef67b841315c29e050#diff-f0fd5f515da65110cd5c1231b43180693e1d46779765d65c997e32da11ae70d1

But this obviously had NO impact on libpcap :-/

> Libpcap library maps both LINUX_SLL_P_CAN and LINUX_SLL_P_CANFD (they are treated identically the same way) to DLT_CAN_SOCKETCAN and copy bytes as is (the only change is changing the endianness if ID if needed).

So this would mean, we need a separate handler for LINUX_SLL_P_CANFD ??

Best regards,
Oliver

> 
> The MTU solution still not does provide a fix for the case of a normal CAN message being transported over a CAN-FD bus.
> In this case, application cannot tell if frame was physically transported as CAN or CAN-FD.
> See https://noelscher.com/blog/posts/compatibility-of-can-and-can-fd/
> 
> Best Regards.
> 
> Ayoub Kaanich.
> 
> -----Original Message-----
> 
> From: mailto:socketcan@hartkopp.net
> Sent: Tuesday, May 4, 2021 10:49 AM
> To: mailto:mkl@pengutronix.de; mailto:linux-can@vger.kernel.org
> Cc: mailto:kayoub5@live.com
> Subject: Re: More flags for logging
> 
> 
> 
> On 03.05.21 12:08, Marc Kleine-Budde wrote:
>> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>>> The SocketCAN API is a great initiative for standardizing the CAN
>>> interface API. This request tries to extend this initiative for more
>>> use cases.
>>>
>>> Context:
>>>
>>> The SocketCAN was adopted by libpcap and tcpdump as the standard
>>> format for logging CAN Frames in PCAP and PCAP-NG. See:
>>>
>>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/pa
>>> cket-socketcan.c https://www.wireshark.org/docs/dfref/c/can.html
>>>
>>> Problem:
>>> Applications that perform logging, usually need more details that
>>> normal applications, for the sake of debugging later on. Flags needs
>>> to be reserved/allocated in the SocketCAN API, so that logging
>>> applications can make use of them in the PCAP format. The flags does
>>> not need necessary need to be implemented by SocketCAN, they just
>>> need to be marked as reserved for such use case.
>>>
>>> Needed Flags:
>>> FDF Flag
>>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>>      logging application needs to know if a normal CAN Frame was
>>>      transmitted on a CAN-FD bus, namely was the FDF bit set or not.
>>
>> I think someone asked for that some time ago. But that was never
>> mainlined. I'll look for that old mail.
>>
> 
> When you display CAN and CAN FD frames in Wireshark they are displayed as different "protocols" - as they also have different ethtypes.
> 
> So the difference is provided by the 'protocol' field. Or did I miss something?
> 
> Regards,
> Oliver
> 
>>> ACK bit in data frame
>>> - Some logging hardware can act as a "spy", meaning that it records
>>> CAN
>>>      Frames, but does not set the ACK bit
>>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>>      not.
>>> - Current API allow detecting missing ACK, but it does not report
>>> what
>>>      Frame had a missing ACK.
>>
>> This means the driver has to set a flag if it's configured in
>> listen-only mode. That should be possible.
>>
>> I think we can make use of flags in the struct canfd_frame for this:
>>
>> | struct canfd_frame {
>> |      canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>> |      __u8    len;     /* frame payload length in byte */
>> |      __u8    flags;   /* additional flags for CAN FD */
>> |      __u8    __res0;  /* reserved / padding */
>> |      __u8    __res1;  /* reserved / padding */
>> |      __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));  };
>>
>> The struct can_frame doesn't have the flags member yet, but we can add
>> it there.
>>
>> regards,
>> Marc
>>
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: mcp251xfd receiving non ACKed frames (was: Re: More flags for logging)
  2021-05-04  7:48             ` mcp251xfd receiving non ACKed frames (was: Re: More flags for logging) Marc Kleine-Budde
@ 2021-05-04 12:22               ` Vincent MAILHOL
  2021-05-04 12:53                 ` Thomas.Kopp
  2021-05-04 17:44                 ` Vincent MAILHOL
  0 siblings, 2 replies; 20+ messages in thread
From: Vincent MAILHOL @ 2021-05-04 12:22 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Ayoub Kaanich, linux-can, Thomas Kopp

Hi Marc,

Thanks for the detailed answer!

On Mar. 4 Mai 2021 at 16:48, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.05.2021 06:46:17, Vincent MAILHOL wrote:
> > > And even on the mcp251xfd, where I receive the CAN frame, there's no way
> > > to tell if this frame has been acked or not.
>
> The test setup is:
>
>                     flexcan (listen only)
>                              |
>                              |
>    PEAK PCAN-USB FD ---------+--------- mcp2518fd (listen only)
>         (sender)             |
>                              |
>                candlelight (going to be unplugged)
>
> pcan-usb: sending CAN frames
> flexcan: receiving CAN frames - but controller in listen only mode
> mcp2518fd: receiving CAN frames - but controller in listen only mode
> candlelight: receiving CAN frames - first attached, then detached
>
> > The mcp251xfd behavior is interesting. Do you also receive the ACK
> > error flag?
>
> In my tests from yesterday neither the flexcan nor the mcp2518fd had bus
> error reporting enabled. So I haven't noticed any ACK errors on the
> mcp2518fd nor the flexcan.
>
> I just repeated the test with bus error reporting enabled:
>
> On the flexcan I receive _only_ these errors (repeating) with
> candlelight detached:
>
> | (2021-05-04 09:00:30.407709)        can0  RX - -  20000088   [8]  00 00 08 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{tx-dominant-bit-error}{}}
> |        bus-error
>
>
> On the mcp2518fd I see these errors:
>
> | (2021-05-04 09:05:00.594321)  mcp251xfd0  RX - -  222   [8]  4A 00 00 00 00 00 00 00
> | (2021-05-04 09:05:01.094418)  mcp251xfd0  RX - -  222   [8]  4B 00 00 00 00 00 00 00
> | (2021-05-04 09:05:01.594577)  mcp251xfd0  RX - -  222   [8]  4C 00 00 00 00 00 00 00
> ...unplug candlelight here...
> | (2021-05-04 09:05:02.094878)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.095589)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.096263)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.096934)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.097596)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.098261)  mcp251xfd0  RX - -  20000088   [8]  00 00 02 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{frame-format-error}{}}
> |        bus-error
> | (2021-05-04 09:05:02.099035)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.099054)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.099603)  mcp251xfd0  RX - -  20000088   [8]  00 00 00 00 00 00 00 00   ERRORFRAME
> |        protocol-violation{{}{}}
> |        bus-error
>
> from here now only RX frames, no error frames

I guess that above error flags are the consequence of the
interferences on the bus while unplugging the candlelight. Those
are probably not relevant to our specific topic.

> | (2021-05-04 09:05:02.100540)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.100570)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.100583)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.100593)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
> | (2021-05-04 09:05:02.101326)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00 00 00 00
>
> ... and repeating.
>
>
> Here a short dump of the mcp2518fd registers:
>
> | INT: intf(0x01c)=0xbf1a0806
> |                 IE      IF      IE & IF
> |         IVMI    x                       Invalid Message Interrupt
> |         WAKI                            Bus Wake Up Interrupt
> |         CERRI   x                       CAN Bus Error Interrupt
> |         SERRI   x                       System Error Interrupt
> |         RXOVI   x       x       x       Receive FIFO Overflow Interrupt
> |         TXATI   x                       Transmit Attempt Interrupt
> |         SPICRCI x                       SPI CRC Error Interrupt
> |         ECCI    x                       ECC Error Interrupt
> |         TEFI    x                       Transmit Event FIFO Interrupt
> |         MODI    x                       Mode Change Interrupt
> |         TBCI            x               Time Base Counter Interrupt
> |         RXI     x       x       x       Receive FIFO Interrupt
> |         TXI                             Transmit FIFO Interrupt
>
> Note: there is no invalid message interrupt pending
>
> | TREC: trec(0x034)=0x00000000
> |             TXBO                Transmitter in Bus Off State
> |             TXBP                Transmitter in Error Passive State
> |             RXBP                Receiver in Error Passive State
> |           TXWARN                Transmitter in Error Warning State
> |           RXWARN                Receiver in Error Warning State
> |            EWARN                Transmitter or Receiver is in Error Warning State
> |              TEC =   0          Transmit Error Counter
> |              REC =   0          Receive Error Counter
> |
> | BDIAG0: bdiag0(0x038)=0x00000010
> |         DTERRCNT =   0          Data Bit Rate Transmit Error Counter
> |         DRERRCNT =   0          Data Bit Rate Receive Error Counter
> |         NTERRCNT =   0          Nominal Bit Rate Transmit Error Counter
> |         NRERRCNT =  16          Nominal Bit Rate Receive Error Counter
> |
> | BDIAG1: bdiag1(0x03c)=0x0000dd4b
> |            DLCMM                DLC Mismatch
> |              ESI                ESI flag of a received CAN FD message was set
> |          DCRCERR                Data CRC Error
> |         DSTUFERR                Data Bit Stuffing Error
> |         DFORMERR                Data Format Error
> |         DBIT1ERR                Data BIT1 Error
> |         DBIT0ERR                Data BIT0 Error
> |          TXBOERR                Device went to bus-off (and auto-recovered)
> |          NCRCERR                CRC Error
> |         NSTUFERR                Bit Stuffing Error
> |         NFORMERR                Format Error
> |          NACKERR                Transmitted message was not acknowledged
> |         NBIT1ERR                Bit1 Error
> |         NBIT0ERR                Bit0 Error
> |         EFMSGCNT = 56651                Error Free Message Counter
>
> > Does the controller retry to send the frame until it gets
> > acknowledged?
>
> Yes - as it should.

I should have been more careful when reading your previous
message. I could have seen that you sent the message with an
increasing payload and that as soon as the acknowledging node was
removed, the same payload kept repeating again and again.

In light of above information I have two remarks:

First, the Peak does not generate the ACK error flag as it is
expected to do. I do not know if this is a side effect of setting
it to listen only. I would expect the listen only mode to only
impact the reception, but maybe it has the side effect of also
allowing to not generate an error if not receiving the ACK bit?
Does the Peak correctly send the ACK error flag when sending in
normal mode (not listen only)?

Second, the receiver behaviour when receiving an non-ACKed frame
is actually unspecified. As mentioned before, non-ACKed frames
should be immediately followed by an ACK error flag. Here, the
receiving nodes are facing a situation which should never
occur. The mcp2518fd decides to register the frame as received
and the flexcan decides to not register the frame. I think that
both behaviors are actually fine: with the lack of specification,
the implementation is free to decide how to handle this side
case.

In short, the real question is the first point: why didn't the
Peak send the ACK error flag?

> > Are you still able to send frames and receive the echo if there is a
> > single node on the network?
>
> No - But the peak driver/hw has some limitations:
>
> The peak driver doesn't have TX complete signaling, it send the echo
> after sending the TX CAN frame via USB. And the peak controller seems to
> buffer quite a lot TX CAN frames, so it looks for the first ~72 frames
> like the bus is still working.

Yes, I also noticed that when I had peak devices in my test
lab. The peak driver call can_put_echo_skb() inside
peak_usb_ndo_start_xmit() and thus, the echo frames do not
reflect whether the actual completion occured or not. I guess
fixing that should not be too hard but I do not have access to
that hardware anymore to do it myself.

I am just surprised by the value of 72 frames. My understanding
is that peak_usb_ndo_start_xmit() should stop the network queue
whenever the number of active tx urbs reaches 10.
Ref:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/peak_usb/pcan_usb_core.c#L399
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/peak_usb/pcan_usb_core.h#L29


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: mcp251xfd receiving non ACKed frames (was: Re: More flags for logging)
  2021-05-04 12:22               ` Vincent MAILHOL
@ 2021-05-04 12:53                 ` Thomas.Kopp
  2021-05-04 14:26                   ` Vincent MAILHOL
  2021-05-04 17:44                 ` Vincent MAILHOL
  1 sibling, 1 reply; 20+ messages in thread
From: Thomas.Kopp @ 2021-05-04 12:53 UTC (permalink / raw)
  To: mailhol.vincent, mkl; +Cc: kayoub5, linux-can

Hi All,

> I guess that above error flags are the consequence of the
> interferences on the bus while unplugging the candlelight. Those
> are probably not relevant to our specific topic.
> 
> > | (2021-05-04 09:05:02.100540)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> 00 00 00
> > | (2021-05-04 09:05:02.100570)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> 00 00 00
> > | (2021-05-04 09:05:02.100583)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> 00 00 00
> > | (2021-05-04 09:05:02.100593)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> 00 00 00
> > | (2021-05-04 09:05:02.101326)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> 00 00 00
> >
Could be, would be worth to check that one a scope. From what I've seen so far the Bus looks clean without disconnecting nodes.

 
> Second, the receiver behaviour when receiving an non-ACKed frame
> is actually unspecified. As mentioned before, non-ACKed frames
> should be immediately followed by an ACK error flag. Here, the
> receiving nodes are facing a situation which should never
> occur. The mcp2518fd decides to register the frame as received
> and the flexcan decides to not register the frame. I think that
> both behaviors are actually fine: with the lack of specification,
> the implementation is free to decide how to handle this side
> case.

I'd say it is actually defined. ISO 11989-1:2015 states the following in chapter 10.4 Bus monitoring
"Optionally, CAN implementations may provide the bus monitoring mode, where they shall be able to 
receive valid DFs and valid RFs, but it sends only recessive bits on the CAN network and does not start 
a transmission. If the MAC sub-layer is required to send a dominant bit (ACK bit, overload flag, active 
error flag), the bit shall be rerouted internally so that the MAC sub-layer monitors this dominant bit, 
although the CAN network may remain in recessive state."

Looking at the MCAN manual here: https://github.com/linux-can/can-doc/blob/master/m_can/mcan_users_manual_v330.pdf

There's a similar description including a picture showing the TX signal being "connected" internally to the RX path to generate the Ack internally.

Thomas

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: More flags for logging
  2021-05-04 10:13           ` Oliver Hartkopp
@ 2021-05-04 13:58             ` Ayoub Kaanich
  2021-05-04 14:38               ` Oliver Hartkopp
  0 siblings, 1 reply; 20+ messages in thread
From: Ayoub Kaanich @ 2021-05-04 13:58 UTC (permalink / raw)
  To: Oliver Hartkopp, mkl, linux-can

Hi,

One solution is to mark a flag bit as 1/0 depending whether the frame was received using LINUX_SLL_P_CAN or LINUX_SLL_P_CANFD.

This may solve 90% of the cases, but I don't think it's the solution we want to go with, since it does not cover classical CAN over CAN-FD transmission.
See 
https://www.can-cia.org/fileadmin/resources/documents/proceedings/2015_esparza.pdf
https://can-newsletter.org/uploads/media/raw/35a48f1d88ddf2a198638c0a5be51761.pdf

I believe it would be better to have an FDF bit in the SocketCAN flags, that maps to (for example) MCP251XFD_OBJ_FLAGS_FDF

Best Regards.

-----Original Message-----
From: Oliver Hartkopp <socketcan@hartkopp.net> 
Sent: Tuesday, May 4, 2021 12:14 PM
To: Ayoub Kaanich <ayoub.kaanich@technica-engineering.de>; mkl@pengutronix.de; linux-can@vger.kernel.org
Subject: Re: More flags for logging

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

Hi Ayoub,

On 04.05.21 11:49, Ayoub Kaanich wrote:

> There seems to be a mis-conception about how WireShark/Libpcap handle SocketCAN:
>
> There is no ethertype involved, the data of the frame will directly start with the CAN ID.
>
> The socket CAN in PCAP format pre-date CAN-FD (Developed in 2011), so at the time, there was only "CAN" (Introduced in Oct, 2009).
> See 
> https://github.com/the-tcpdump-group/libpcap/commit/86ed15cc8b4bd7dc14
> 58559108e66a07ec6721ec The introduction of CAN-FD later on did not 
> change that.

Ah, now I got the problem.

I was heavily involved in introducing CAN FD into Wireshark here:

https://github.com/wireshark/wireshark/commit/7fad354a3e379382368cd1ef67b841315c29e050#diff-f0fd5f515da65110cd5c1231b43180693e1d46779765d65c997e32da11ae70d1

But this obviously had NO impact on libpcap :-/

> Libpcap library maps both LINUX_SLL_P_CAN and LINUX_SLL_P_CANFD (they are treated identically the same way) to DLT_CAN_SOCKETCAN and copy bytes as is (the only change is changing the endianness if ID if needed).

So this would mean, we need a separate handler for LINUX_SLL_P_CANFD ??

Best regards,
Oliver

>
> The MTU solution still not does provide a fix for the case of a normal CAN message being transported over a CAN-FD bus.
> In this case, application cannot tell if frame was physically transported as CAN or CAN-FD.
> See https://noelscher.com/blog/posts/compatibility-of-can-and-can-fd/
>
> Best Regards.
>
> Ayoub Kaanich.
>
> -----Original Message-----
>
> From: mailto:socketcan@hartkopp.net
> Sent: Tuesday, May 4, 2021 10:49 AM
> To: mailto:mkl@pengutronix.de; mailto:linux-can@vger.kernel.org
> Cc: mailto:kayoub5@live.com
> Subject: Re: More flags for logging
>
>
>
> On 03.05.21 12:08, Marc Kleine-Budde wrote:
>> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>>> The SocketCAN API is a great initiative for standardizing the CAN 
>>> interface API. This request tries to extend this initiative for more 
>>> use cases.
>>>
>>> Context:
>>>
>>> The SocketCAN was adopted by libpcap and tcpdump as the standard 
>>> format for logging CAN Frames in PCAP and PCAP-NG. See:
>>>
>>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/p
>>> a cket-socketcan.c https://www.wireshark.org/docs/dfref/c/can.html
>>>
>>> Problem:
>>> Applications that perform logging, usually need more details that 
>>> normal applications, for the sake of debugging later on. Flags needs 
>>> to be reserved/allocated in the SocketCAN API, so that logging 
>>> applications can make use of them in the PCAP format. The flags does 
>>> not need necessary need to be implemented by SocketCAN, they just 
>>> need to be marked as reserved for such use case.
>>>
>>> Needed Flags:
>>> FDF Flag
>>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>>      logging application needs to know if a normal CAN Frame was
>>>      transmitted on a CAN-FD bus, namely was the FDF bit set or not.
>>
>> I think someone asked for that some time ago. But that was never 
>> mainlined. I'll look for that old mail.
>>
>
> When you display CAN and CAN FD frames in Wireshark they are displayed as different "protocols" - as they also have different ethtypes.
>
> So the difference is provided by the 'protocol' field. Or did I miss something?
>
> Regards,
> Oliver
>
>>> ACK bit in data frame
>>> - Some logging hardware can act as a "spy", meaning that it records 
>>> CAN
>>>      Frames, but does not set the ACK bit
>>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>>      not.
>>> - Current API allow detecting missing ACK, but it does not report 
>>> what
>>>      Frame had a missing ACK.
>>
>> This means the driver has to set a flag if it's configured in 
>> listen-only mode. That should be possible.
>>
>> I think we can make use of flags in the struct canfd_frame for this:
>>
>> | struct canfd_frame {
>> |      canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>> |      __u8    len;     /* frame payload length in byte */
>> |      __u8    flags;   /* additional flags for CAN FD */
>> |      __u8    __res0;  /* reserved / padding */
>> |      __u8    __res1;  /* reserved / padding */
>> |      __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));  };
>>
>> The struct can_frame doesn't have the flags member yet, but we can 
>> add it there.
>>
>> regards,
>> Marc
>>
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: mcp251xfd receiving non ACKed frames (was: Re: More flags for logging)
  2021-05-04 12:53                 ` Thomas.Kopp
@ 2021-05-04 14:26                   ` Vincent MAILHOL
  0 siblings, 0 replies; 20+ messages in thread
From: Vincent MAILHOL @ 2021-05-04 14:26 UTC (permalink / raw)
  To: Thomas.Kopp; +Cc: Marc Kleine-Budde, Ayoub Kaanich, linux-can

On Tue. 4 Mai 2021 à 21:53, <Thomas.Kopp@microchip.com> wrote:
> Hi All,
>
> > I guess that above error flags are the consequence of the
> > interferences on the bus while unplugging the candlelight. Those
> > are probably not relevant to our specific topic.
> >
> > > | (2021-05-04 09:05:02.100540)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> > 00 00 00
> > > | (2021-05-04 09:05:02.100570)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> > 00 00 00
> > > | (2021-05-04 09:05:02.100583)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> > 00 00 00
> > > | (2021-05-04 09:05:02.100593)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> > 00 00 00
> > > | (2021-05-04 09:05:02.101326)  mcp251xfd0  RX - -  222   [8]  4D 00 00 00 00
> > 00 00 00
> > >
> Could be, would be worth to check that one a scope. From what I've seen so far the Bus looks clean without disconnecting nodes.
>
>
> > Second, the receiver behaviour when receiving an non-ACKed frame
> > is actually unspecified. As mentioned before, non-ACKed frames
> > should be immediately followed by an ACK error flag. Here, the
> > receiving nodes are facing a situation which should never
> > occur. The mcp2518fd decides to register the frame as received
> > and the flexcan decides to not register the frame. I think that
> > both behaviors are actually fine: with the lack of specification,
> > the implementation is free to decide how to handle this side
> > case.
>
> I'd say it is actually defined. ISO 11989-1:2015 states the following in chapter 10.4 Bus monitoring
> "Optionally, CAN implementations may provide the bus monitoring mode, where they shall be able to
> receive valid DFs and valid RFs, but it sends only recessive bits on the CAN network and does not start
> a transmission. If the MAC sub-layer is required to send a dominant bit (ACK bit, overload flag, active
> error flag), the bit shall be rerouted internally so that the MAC sub-layer monitors this dominant bit,
> although the CAN network may remain in recessive state."
>
> Looking at the MCAN manual here: https://github.com/linux-can/can-doc/blob/master/m_can/mcan_users_manual_v330.pdf
>
> There's a similar description including a picture showing the TX signal being "connected" internally to the RX path to generate the Ack internally.

Thanks for pointing this out. You are right and I completely
missed that paragraph of the ISO.

So this perfectly explains the behaviour of the mcp251xfd0. It
also means that the ACK flag is set by the transmitter so there
is indeed no way to differentiate if the frame was indeed ACKed
or not by the other nodes.

Concerning the flexcan it is not following the standard. Even if
the monitor mode is optional, once it is implemented, it should
behave as stated in chapter 10.4.

However, this still does not explain why the Peak does not
generate the ACK error flag. Also, the ISO says that in
monitoring mode, the controller "does not start a
transmission". So it also seems like a violation of the standard
that Peak devices are able to transmit frames while being in
monitor mode.


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
  2021-05-04 13:58             ` Ayoub Kaanich
@ 2021-05-04 14:38               ` Oliver Hartkopp
  0 siblings, 0 replies; 20+ messages in thread
From: Oliver Hartkopp @ 2021-05-04 14:38 UTC (permalink / raw)
  To: Ayoub Kaanich, mkl, linux-can



On 04.05.21 15:58, Ayoub Kaanich wrote:

> One solution is to mark a flag bit as 1/0 depending whether the frame was received using LINUX_SLL_P_CAN or LINUX_SLL_P_CANFD.
> 
> This may solve 90% of the cases, but I don't think it's the solution we want to go with, since it does not cover classical CAN over CAN-FD transmission.
> See
> https://www.can-cia.org/fileadmin/resources/documents/proceedings/2015_esparza.pdf
> https://can-newsletter.org/uploads/media/raw/35a48f1d88ddf2a198638c0a5be51761.pdf

A CAN segment can transfer CAN and CAN FD frames when all the nodes are 
capable to cope with CAN FD frames (having the former reserved FDF bit a 
recessive '1' value).

I don't see any problem here ... CAN frames and CAN FD can co-exist an 
use LINUX_SLL_P_CAN or LINUX_SLL_P_CANFD.

> I believe it would be better to have an FDF bit in the SocketCAN flags, that maps to (for example) MCP251XFD_OBJ_FLAGS_FDF

In fact I already posted a patch for that priblem in 2017:

https://lore.kernel.org/linux-can/20170411134343.3089-1-socketcan@hartkopp.net/

I had a similar idea behind it ;-)

Best regards,
Oliver

> 
> -----Original Message-----
> From: Oliver Hartkopp <socketcan@hartkopp.net>
> Sent: Tuesday, May 4, 2021 12:14 PM
> To: Ayoub Kaanich <ayoub.kaanich@technica-engineering.de>; mkl@pengutronix.de; linux-can@vger.kernel.org
> Subject: Re: More flags for logging
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> Hi Ayoub,
> 
> On 04.05.21 11:49, Ayoub Kaanich wrote:
> 
>> There seems to be a mis-conception about how WireShark/Libpcap handle SocketCAN:
>>
>> There is no ethertype involved, the data of the frame will directly start with the CAN ID.
>>
>> The socket CAN in PCAP format pre-date CAN-FD (Developed in 2011), so at the time, there was only "CAN" (Introduced in Oct, 2009).
>> See
>> https://github.com/the-tcpdump-group/libpcap/commit/86ed15cc8b4bd7dc14
>> 58559108e66a07ec6721ec The introduction of CAN-FD later on did not
>> change that.
> 
> Ah, now I got the problem.
> 
> I was heavily involved in introducing CAN FD into Wireshark here:
> 
> https://github.com/wireshark/wireshark/commit/7fad354a3e379382368cd1ef67b841315c29e050#diff-f0fd5f515da65110cd5c1231b43180693e1d46779765d65c997e32da11ae70d1
> 
> But this obviously had NO impact on libpcap :-/
> 
>> Libpcap library maps both LINUX_SLL_P_CAN and LINUX_SLL_P_CANFD (they are treated identically the same way) to DLT_CAN_SOCKETCAN and copy bytes as is (the only change is changing the endianness if ID if needed).
> 
> So this would mean, we need a separate handler for LINUX_SLL_P_CANFD ??
> 
> Best regards,
> Oliver
> 
>>
>> The MTU solution still not does provide a fix for the case of a normal CAN message being transported over a CAN-FD bus.
>> In this case, application cannot tell if frame was physically transported as CAN or CAN-FD.
>> See https://noelscher.com/blog/posts/compatibility-of-can-and-can-fd/
>>
>> Best Regards.
>>
>> Ayoub Kaanich.
>>
>> -----Original Message-----
>>
>> From: mailto:socketcan@hartkopp.net
>> Sent: Tuesday, May 4, 2021 10:49 AM
>> To: mailto:mkl@pengutronix.de; mailto:linux-can@vger.kernel.org
>> Cc: mailto:kayoub5@live.com
>> Subject: Re: More flags for logging
>>
>>
>>
>> On 03.05.21 12:08, Marc Kleine-Budde wrote:
>>> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>>>> The SocketCAN API is a great initiative for standardizing the CAN
>>>> interface API. This request tries to extend this initiative for more
>>>> use cases.
>>>>
>>>> Context:
>>>>
>>>> The SocketCAN was adopted by libpcap and tcpdump as the standard
>>>> format for logging CAN Frames in PCAP and PCAP-NG. See:
>>>>
>>>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>>>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/p
>>>> a cket-socketcan.c https://www.wireshark.org/docs/dfref/c/can.html
>>>>
>>>> Problem:
>>>> Applications that perform logging, usually need more details that
>>>> normal applications, for the sake of debugging later on. Flags needs
>>>> to be reserved/allocated in the SocketCAN API, so that logging
>>>> applications can make use of them in the PCAP format. The flags does
>>>> not need necessary need to be implemented by SocketCAN, they just
>>>> need to be marked as reserved for such use case.
>>>>
>>>> Needed Flags:
>>>> FDF Flag
>>>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>>>       logging application needs to know if a normal CAN Frame was
>>>>       transmitted on a CAN-FD bus, namely was the FDF bit set or not.
>>>
>>> I think someone asked for that some time ago. But that was never
>>> mainlined. I'll look for that old mail.
>>>
>>
>> When you display CAN and CAN FD frames in Wireshark they are displayed as different "protocols" - as they also have different ethtypes.
>>
>> So the difference is provided by the 'protocol' field. Or did I miss something?
>>
>> Regards,
>> Oliver
>>
>>>> ACK bit in data frame
>>>> - Some logging hardware can act as a "spy", meaning that it records
>>>> CAN
>>>>       Frames, but does not set the ACK bit
>>>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>>>       not.
>>>> - Current API allow detecting missing ACK, but it does not report
>>>> what
>>>>       Frame had a missing ACK.
>>>
>>> This means the driver has to set a flag if it's configured in
>>> listen-only mode. That should be possible.
>>>
>>> I think we can make use of flags in the struct canfd_frame for this:
>>>
>>> | struct canfd_frame {
>>> |      canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>> |      __u8    len;     /* frame payload length in byte */
>>> |      __u8    flags;   /* additional flags for CAN FD */
>>> |      __u8    __res0;  /* reserved / padding */
>>> |      __u8    __res1;  /* reserved / padding */
>>> |      __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));  };
>>>
>>> The struct can_frame doesn't have the flags member yet, but we can
>>> add it there.
>>>
>>> regards,
>>> Marc
>>>
>>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: mcp251xfd receiving non ACKed frames (was: Re: More flags for logging)
  2021-05-04 12:22               ` Vincent MAILHOL
  2021-05-04 12:53                 ` Thomas.Kopp
@ 2021-05-04 17:44                 ` Vincent MAILHOL
  1 sibling, 0 replies; 20+ messages in thread
From: Vincent MAILHOL @ 2021-05-04 17:44 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Ayoub Kaanich, linux-can, Thomas Kopp

On Tue. 4 Mai 2021 at 21:22, Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> On Mar. 4 Mai 2021 at 16:48, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 04.05.2021 06:46:17, Vincent MAILHOL wrote:
[...]
> > > Are you still able to send frames and receive the echo if there is a
> > > single node on the network?
> >
> > No - But the peak driver/hw has some limitations:
> >
> > The peak driver doesn't have TX complete signaling, it send the echo
> > after sending the TX CAN frame via USB. And the peak controller seems to
> > buffer quite a lot TX CAN frames, so it looks for the first ~72 frames
> > like the bus is still working.
>
> Yes, I also noticed that when I had peak devices in my test
> lab. The peak driver call can_put_echo_skb() inside
> peak_usb_ndo_start_xmit() and thus, the echo frames do not
> reflect whether the actual completion occured or not. I guess
> fixing that should not be too hard but I do not have access to
> that hardware anymore to do it myself.
>
> I am just surprised by the value of 72 frames. My understanding
> is that peak_usb_ndo_start_xmit() should stop the network queue
> whenever the number of active tx urbs reaches 10.
> Ref:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/peak_usb/pcan_usb_core.c#L399
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/peak_usb/pcan_usb_core.h#L29

Hi Marc, please ignore above paragraphs. I mixed up the tx buffer
with the TX frame buffer. Also, implementing a full TX completion
might actually not be possible due to the Peak device
limitations.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: More flags for logging
       [not found]     ` <DBBPR03MB70824BB82F7BB335928BE36A9D2F9@DBBPR03MB7082.eurprd03.prod.outlook.com>
@ 2021-05-17  7:29       ` Oliver Hartkopp
  0 siblings, 0 replies; 20+ messages in thread
From: Oliver Hartkopp @ 2021-05-17  7:29 UTC (permalink / raw)
  To: Ayoub Kaanich, Marc Kleine-Budde, linux-can



On 16.05.21 01:46, Ayoub Kaanich wrote:
> Hi,
> 
> Sorry for taking so long to reply, I wanted to check the FDF topic 
> thoroughly first.
> 

No problem ;-)

> 
> The documentation of the canfd_frame states that "The use of struct 
> canfd_frame implies the Extended Data Length (EDL) bit to be set in the 
> CAN frame bitstream on the wire."

Thanks for the hint. This comment in include/uapi/linux/can.h has been 
created before the renaming that bit to FDF and needs an update.

> There have been editorial changes when the FD Frame Format was 
> introduced into ISO 11898-1,the bit EDL is now named FDF (FD Format).
> 
> See 
> https://www.can-cia.org/fileadmin/resources/documents/proceedings/2013_hartwich_v2.pdf
> 
> I believe that the fact that "EDL" and "FDF" are the same thing should 
> be clarified to avoid future confusion.

I was personally involved in CAN FD standardization and suggested the 
renaming, as "extended data length" (EDL) did not completely catch the 
CAN FD features. This renaming EDL -> FDF is a replacement for 
clarification. No confusion here :-)

> According to 
> https://www.kernel.org/doc/html/latest/networking/can.html#raw-socket-option-can-raw-fd-frames
> 
> When performing a read operation on a socket that had CAN_RAW_FD_FRAMES 
> enabled, both CAN_MTU and CANFD_MTU are allowed.
> 
> I originally though that only one of them is allowed at a time, that was 
> a mistake from my side.

You might want to take a look into this slide deck (slide 48ff):

https://wiki.automotivelinux.org/_media/agl-distro/agl2017-socketcan-print.pdf

> So using MTU will work for FD, but will definitely be a problem in the 
> future with CAN XL due to its large size (more than 2KB)

Yes. But we should then thing about CAN_RAW_XL_FRAMES in the same manner.

> The last question would be, would libpcap keep the extra padding bytes 
> in, or remove them.
> 
> Since the document in 
> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html is not 
> conclusive about the topic (the diagram makes it look like it's truncated)
> 
> I will have to check on an actual system during the work days, and push 
> a PR to tcpdump if it turns out it's just the diagram that was misleading.

In fact this documentation is outdated as we updated the struct 
can_frame for the 'Classical CAN' (not CAN FD) to contain a raw data 
length code (DLC) value in the formerly reserved space:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=ea7800565a128c1adafa1791ce80afd6016fe21c

Btw. I would still be interested to get the CANFD_FDF reservation for 
user space application programmers into Mainline:

https://lore.kernel.org/linux-can/20170411134343.3089-1-socketcan@hartkopp.net/

So let's see whether Marc can be excited on this patch ;-)

Best regards,
Oliver


> 
> 
> Best Regards.
> 
> 
> On 2021-05-04 10:49 AM, Oliver Hartkopp wrote:
>>
>>
>> On 03.05.21 12:08, Marc Kleine-Budde wrote:
>>> On 03.05.2021 12:02:46, Marc Kleine-Budde wrote:
>>>> The SocketCAN API is a great initiative for standardizing the CAN
>>>> interface API. This request tries to extend this initiative for more 
>>>> use
>>>> cases.
>>>>
>>>> Context:
>>>>
>>>> The SocketCAN was adopted by libpcap and tcpdump as the standard format
>>>> for logging CAN Frames in PCAP and PCAP-NG. See:
>>>>
>>>> https://www.tcpdump.org/linktypes/LINKTYPE_CAN_SOCKETCAN.html
>>>> https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-socketcan.c 
>>>>
>>>> https://www.wireshark.org/docs/dfref/c/can.html
>>>>
>>>> Problem:
>>>> Applications that perform logging, usually need more details that 
>>>> normal
>>>> applications, for the sake of debugging later on. Flags needs to be
>>>> reserved/allocated in the SocketCAN API, so that logging applications
>>>> can make use of them in the PCAP format. The flags does not need
>>>> necessary need to be implemented by SocketCAN, they just need to be
>>>> marked as reserved for such use case.
>>>>
>>>> Needed Flags:
>>>> FDF Flag
>>>> - Since CAN Frames and CAN-FD frames can co-exist in the same bus,
>>>>    logging application needs to know if a normal CAN Frame was
>>>>    transmitted on a CAN-FD bus, namely was the FDF bit set or not.
>>>
>>> I think someone asked for that some time ago. But that was never
>>> mainlined. I'll look for that old mail.
>>>
>>
>> When you display CAN and CAN FD frames in Wireshark they are displayed 
>> as different "protocols" - as they also have different ethtypes.
>>
>> So the difference is provided by the 'protocol' field. Or did I miss 
>> something?
>>
>> Regards,
>> Oliver
>>
>>>> ACK bit in data frame
>>>> - Some logging hardware can act as a "spy", meaning that it records CAN
>>>>    Frames, but does not set the ACK bit
>>>> - A way to know for a given frame (FD or not), was the ACK bit set or
>>>>    not.
>>>> - Current API allow detecting missing ACK, but it does not report what
>>>>    Frame had a missing ACK.
>>>
>>> This means the driver has to set a flag if it's configured in
>>> listen-only mode. That should be possible.
>>>
>>> I think we can make use of flags in the struct canfd_frame for this:
>>>
>>> | struct canfd_frame {
>>> |     canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
>>> |     __u8    len;     /* frame payload length in byte */
>>> |     __u8    flags;   /* additional flags for CAN FD */
>>> |     __u8    __res0;  /* reserved / padding */
>>> |     __u8    __res1;  /* reserved / padding */
>>> |     __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>>> | };
>>>
>>> The struct can_frame doesn't have the flags member yet, but we can add
>>> it there.
>>>
>>> regards,
>>> Marc
>>>

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-05-17  7:29 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-03 10:02 More flags for logging Marc Kleine-Budde
2021-05-03 10:08 ` Marc Kleine-Budde
2021-05-03 15:02   ` Vincent MAILHOL
2021-05-03 15:43     ` Marc Kleine-Budde
     [not found]     ` <DBBPR03MB70828377F51A1747B4E9E6529D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
2021-05-03 15:47       ` Marc Kleine-Budde
     [not found]         ` <DBBPR03MB7082F029173018680E5D869C9D5B9@DBBPR03MB7082.eurprd03.prod.outlook.com>
2021-05-03 21:46           ` Vincent MAILHOL
2021-05-04  5:40             ` Patrick Menschel
2021-05-04  7:48             ` mcp251xfd receiving non ACKed frames (was: Re: More flags for logging) Marc Kleine-Budde
2021-05-04 12:22               ` Vincent MAILHOL
2021-05-04 12:53                 ` Thomas.Kopp
2021-05-04 14:26                   ` Vincent MAILHOL
2021-05-04 17:44                 ` Vincent MAILHOL
2021-05-04 10:10         ` More flags for logging Kurt Van Dijck
2021-05-04 10:07     ` Kurt Van Dijck
2021-05-04  8:49   ` Oliver Hartkopp
     [not found]     ` <DBBPR03MB7082C7E8FD22D0CA8C2DA99D9D5A9@DBBPR03MB7082.eurprd03.prod.outlook.com>
     [not found]       ` <AM8PR08MB5698886555F8531B6CF65982B75A9@AM8PR08MB5698.eurprd08.prod.outlook.com>
2021-05-04  9:49         ` Ayoub Kaanich
2021-05-04 10:13           ` Oliver Hartkopp
2021-05-04 13:58             ` Ayoub Kaanich
2021-05-04 14:38               ` Oliver Hartkopp
     [not found]     ` <DBBPR03MB70824BB82F7BB335928BE36A9D2F9@DBBPR03MB7082.eurprd03.prod.outlook.com>
2021-05-17  7:29       ` Oliver Hartkopp

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).