netdev - Re: [PATCH 6/7] can: m_can: update to support CAN FD features

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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