On Tue 15 Dec. 2020 at 17:08, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > > On 12/14/20 10:14 AM, Marc Kleine-Budde wrote: > > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > This patch adds the function can_skb_get_dll_len() which returns the length of > > a CAN frame on the data link layer, including Start-of-frame, Identifier, > > various other bits, the CRC, the End-of-frame, the Inter frame spacing and the > > actual data. > > > > Co-developed-by: Arunachalam Santhanam <arunachalam.santhanam@xxxxxxxxxxxx> > > Not-Signed-off-by: Arunachalam Santhanam <arunachalam.santhanam@xxxxxxxxxxxx> > > Co-developed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > Not-Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > Co-developed-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > > --- > > Hello, > > > > while reviewing the etas_es58X driver and hacking on BQL support for the > > mcp251xfd driver, it turned out the a function to calculate the length of the > > CAN frame that's send over the wire should go into the generic CAN dev code. > > > > Looking at the CAN and OSI layering I think the first layer where we have all > > the bits that we see on the wire is the data link layer (DLL). > > > > https://www.can-cia.org/can-knowledge/can/can-data-link-layers > > > > This is why I named the function can_skb_get_dll_len(). > > > > I'm planing to add a (better) calculation of the CAN-FD dll_len, depending on > > the data and including different CRC lengths. > > > > As this code is copied from the etas_es58X driver, I've added the authors as > > Co-developed-by and I'd like to add their S-o-b to this patch. > > > > Please review and commnt on the naming on the functions. > > When implementing BQL we need the CAN frame's DLL len twice: > 1. When sending the package to the hardware > 2. After the TX-complete IRQ > > We can calculate this information twice, but I thought we might put it into the > struct can_skb_priv. > > https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L34 > > Thoughts? I am not knowledgeable enough on this part to guarantee if there will be no side effects but regardless, I like the idea. Also, an u8 is enough to hold the value. I wonder if it would be fine to change ifindex to, for example, u16, so that we do not lose any memory. I would look like that: struct can_skb_priv { u16 ifindex; u8 frame_len; u8 res; int skbcnt; struct can_frame cf[]; }; And the final result in the driver would look as below: /* Tx branch */ can_skb_prv(skb)->frame_len = can_skb_get_frame_len(skb); netdev_sent_queue(can_skb_prv(skb)->frame_len); /* Rx branch */ netdev_completed_queue(can_skb_prv(skb)->frame_len); This is quite neat.
