lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Oct 2014 20:21:28 +0100 From: Oliver Hartkopp <socketcan@...tkopp.net> To: Dong Aisheng <b29396@...escale.com>, linux-can@...r.kernel.org CC: mkl@...gutronix.de, wg@...ndegger.com, varkabhadram@...il.com, netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH 6/7] can: m_can: update to support CAN FD features Hello Dong, thanks for your update to support CAN FD with the 3.0.x M_CAN IP core. AFAIK from the last CAN in Automation (CiA) Plugfest which took place in Nuremberg yesterday, there are two more IP cores on the way: v3.0.1 / v3.0.2 (the current spec from the Bosch website) v3.1.0 which will support per-frame CAN/CANFD switching in the tx path (the FDF/(former)EDL bit and the BRS bit appear in the TX buffer element at the bit position you know from reading the RX FIFO element) v3.2.0 which will support the final(?) ISO specification with a bitstuffing counter before the CRC in the frame on the wire. So first I would suggest to check the core release register (CREL) to be version 3.0.x and quit the driver initialization if it doesn't fit this version. I would suggest to provide a separate patch for that check. What about printing the core release version into the kernel log at driver initialization time? Regarding the CAN FD support in this patch I have some remarks in the text ... On 10/29/2014 11:45 AM, Dong Aisheng wrote: > /* Rx Buffer Element */ > +/* R0 */ > #define RX_BUF_ESI BIT(31) > #define RX_BUF_XTD BIT(30) > #define RX_BUF_RTR BIT(29) > +/* R1 */ > +#define RX_BUF_ANMF BIT(31) > +#define RX_BUF_EDL BIT(21) > +#define RX_BUF_BRS BIT(20) > > /* Tx Buffer Element */ > +/* R0 */ > #define TX_BUF_XTD BIT(30) > #define TX_BUF_RTR BIT(29) > > @@ -327,11 +357,12 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv) > m_can_write(priv, M_CAN_ILE, 0x0); > } > > -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, > +static void m_can_read_fifo(const struct net_device *dev, struct canfd_frame *cf, > u32 rxfs) > { > struct m_can_priv *priv = netdev_priv(dev); > u32 id, fgi; > + int i; > > /* calculate the fifo get index for where to read data */ > fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF; > @@ -341,15 +372,23 @@ static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, > else > cf->can_id = (id >> 18) & CAN_SFF_MASK; > > + if (id & RX_BUF_ESI) { > + cf->flags |= CANFD_ESI; > + netdev_dbg(dev, "ESI Error\n"); > + } > + > if (id & RX_BUF_RTR) { > cf->can_id |= CAN_RTR_FLAG; When RX_BUF_EDL is set you should not check for RX_BUF_RTR as RTR is not allowed for CAN FD. > } else { > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); > - cf->can_dlc = get_can_dlc((id >> 16) & 0x0F); > - *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi, > - M_CAN_FIFO_DATA(0)); > - *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi, > - M_CAN_FIFO_DATA(1)); > + cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F)); > + > + if (id & RX_BUF_BRS) > + cf->flags |= CANFD_BRS; > + > + for (i = 0; i < cf->len; i += 4) > + *(u32 *)(cf->data + i) = > + m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i / 4)); > } > > /* acknowledge rx fifo 0 */ > @@ -361,7 +400,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota) > struct m_can_priv *priv = netdev_priv(dev); > struct net_device_stats *stats = &dev->stats; > struct sk_buff *skb; > - struct can_frame *frame; > + struct canfd_frame *frame; > u32 pkts = 0; > u32 rxfs; > > @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota) > if (rxfs & RXFS_RFL) > netdev_warn(dev, "Rx FIFO 0 Message Lost\n"); > > - skb = alloc_can_skb(dev, &frame); > + skb = alloc_canfd_skb(dev, &frame); You are *always* allocating CAN FD frames now? Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN FD frame. Of course you can use the struct canfd_frame in m_can_read_fifo() as the layout of the struct can_frame is identical when filled with 'normal' CAN frame content. But you need to distinguish whether it is a CAN or CAN FD frame when allocating the skb based on the RX_BUF_EDL value. > if (!skb) { > stats->rx_dropped++; > return pkts; > @@ -384,7 +423,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota) > m_can_read_fifo(dev, frame, rxfs); > > stats->rx_packets++; > - stats->rx_bytes += frame->can_dlc; > + stats->rx_bytes += frame->len; > > netif_receive_skb(skb); > The rest of your entire patch set looks very good from my perspective. Best regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists