[PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Reuben Dowle]

Currently the flexcan driver uses hardware local echo. This blindly echos all transmitted frames to all receiving sockets, regardless what CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.

This patch now submits transmitted frames to be echoed in the transmit complete interrupt, preserving the reference to the sending socket. This allows the can protocol to correctly handle the local echo.

Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>

---
 drivers/net/can/flexcan.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e023379..542ada8 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
 	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
-	kfree_skb(skb);
+	can_put_echo_skb(skb, dev, 0);
 
 	/* tx_packets is incremented in flexcan_irq */
 	stats->tx_bytes += cf->can_dlc;
@@ -612,6 +612,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		/* tx_bytes is incremented in flexcan_start_xmit */
 		stats->tx_packets++;
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
+		can_get_echo_skb(dev, 0);
 		netif_wake_queue(dev);
 	}
 
@@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device *dev)
 	int err;
 	u32 reg_mcr, reg_ctrl;
 
+	can_free_echo_skb(dev, 0);
+
 	/* enable module */
 	flexcan_chip_enable(priv);
 
@@ -697,12 +700,13 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * only supervisor access
 	 * enable warning int
 	 * choose format C
+	 * disable local echo
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
-		FLEXCAN_MCR_IDAM_C;
+		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
 	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		goto failed_map;
 	}
 
-	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
+	dev = alloc_candev(sizeof(struct flexcan_priv), 1);
 	if (!dev) {
 		err = -ENOMEM;
 		goto failed_alloc;
@@ -978,7 +982,14 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 
 	dev->netdev_ops = &flexcan_netdev_ops;
 	dev->irq = irq;
-	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
+
+	/* Driver supports local echo.
+	 * We support local echo in hardware, however this is not used because
+	 * hardware local echo loses the sending socket reference
+	 * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket options
+	 *  would not work)
+	 */
+	dev->flags |= IFF_ECHO;
 
 	priv = netdev_priv(dev);
 	priv->can.clock.freq = clock_freq;

Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Marc Kleine-Budde]

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

On 10/31/2011 11:18 PM, Reuben Dowle wrote:
> Currently the flexcan driver uses hardware local echo. This blindly echos all transmitted frames to all receiving sockets, regardless what CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> 
> This patch now submits transmitted frames to be echoed in the transmit complete interrupt, preserving the reference to the sending socket. This allows the can protocol to correctly handle the local echo.
> 
> Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>

Patch looks quite good. Can you please wrap the description to about 72
chars?

> 
> ---
>  drivers/net/can/flexcan.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e023379..542ada8 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
>  	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
>  
> -	kfree_skb(skb);
> +	can_put_echo_skb(skb, dev, 0);
>  
>  	/* tx_packets is incremented in flexcan_irq */
>  	stats->tx_bytes += cf->can_dlc;
> @@ -612,6 +612,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  		/* tx_bytes is incremented in flexcan_start_xmit */
>  		stats->tx_packets++;
>  		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
> +		can_get_echo_skb(dev, 0);
>  		netif_wake_queue(dev);
>  	}
>  
> @@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device *dev)
>  	int err;
>  	u32 reg_mcr, reg_ctrl;
>  
> +	can_free_echo_skb(dev, 0);

what about putting this to flexcan_chip_stop? Otherwise you risk a
memleak if you do "ifconfig down; rmmod flexcan"

> +
>  	/* enable module */
>  	flexcan_chip_enable(priv);
>  
> @@ -697,12 +700,13 @@ static int flexcan_chip_start(struct net_device *dev)
>  	 * only supervisor access
>  	 * enable warning int
>  	 * choose format C
> +	 * disable local echo
>  	 *
>  	 */
>  	reg_mcr = flexcan_read(&regs->mcr);
>  	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
>  		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
> -		FLEXCAN_MCR_IDAM_C;
> +		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
>  	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	flexcan_write(reg_mcr, &regs->mcr);
>  
> @@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  		goto failed_map;
>  	}
>  
> -	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
> +	dev = alloc_candev(sizeof(struct flexcan_priv), 1);
>  	if (!dev) {
>  		err = -ENOMEM;
>  		goto failed_alloc;
> @@ -978,7 +982,14 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  
>  	dev->netdev_ops = &flexcan_netdev_ops;
>  	dev->irq = irq;
> -	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
> +
> +	/* Driver supports local echo.
> +	 * We support local echo in hardware, however this is not used because
> +	 * hardware local echo loses the sending socket reference
> +	 * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket options
> +	 *  would not work)
> +	 */

IMHO, you can skip this comment. The patch description is good enough.

> +	dev->flags |= IFF_ECHO;
>  
>  	priv = netdev_priv(dev);
>  	priv->can.clock.freq = clock_freq;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

RE: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Reuben Dowle]

Currently the flexcan driver uses hardware local echo. This blindly
echoes all transmitted frames to all receiving sockets, regardless what
CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.

This patch now submits transmitted frames to be echoed in the transmit
complete interrupt, preserving the reference to the sending socket.
This allows the can protocol to correctly handle the local echo.

Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>

---
 drivers/net/can/flexcan.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c index e023379..542ada8 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
 	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
-	kfree_skb(skb);
+	can_put_echo_skb(skb, dev, 0);
 
 	/* tx_packets is incremented in flexcan_irq */
 	stats->tx_bytes += cf->can_dlc;
@@ -612,6 +612,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		/* tx_bytes is incremented in flexcan_start_xmit */
 		stats->tx_packets++;
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
+		can_get_echo_skb(dev, 0);
 		netif_wake_queue(dev);
 	}
 
@@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device *dev)
 	int err;
 	u32 reg_mcr, reg_ctrl;
 
+	can_free_echo_skb(dev, 0);
+
 	/* enable module */
 	flexcan_chip_enable(priv);
 
@@ -697,12 +700,13 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * only supervisor access
 	 * enable warning int
 	 * choose format C
+	 * disable local echo
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
-		FLEXCAN_MCR_IDAM_C;
+		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
 	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		goto failed_map;
 	}
 
-	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
+	dev = alloc_candev(sizeof(struct flexcan_priv), 1);
 	if (!dev) {
 		err = -ENOMEM;
 		goto failed_alloc;
@@ -978,7 +982,14 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 
 	dev->netdev_ops = &flexcan_netdev_ops;
 	dev->irq = irq;
-	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
+
+	/* Driver supports local echo.
+	 * We support local echo in hardware, however this is not used because
+	 * hardware local echo loses the sending socket reference
+	 * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket options
+	 *  would not work)
+	 */
+	dev->flags |= IFF_ECHO;
 
 	priv = netdev_priv(dev);
 	priv->can.clock.freq = clock_freq;
---

> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Tuesday, November 01, 2011 11:31 AM
> To: Reuben Dowle
> Cc: netdev@vger.kernel.org; linux-can@vger.kernel.org
> Subject: Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and
> CAN_RAW_LOOPBACK
> 
> On 10/31/2011 11:18 PM, Reuben Dowle wrote:
> > Currently the flexcan driver uses hardware local echo. This blindly
> echos all transmitted frames to all receiving sockets, regardless what
> CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> >
> > This patch now submits transmitted frames to be echoed in the
> transmit complete interrupt, preserving the reference to the sending
> socket. This allows the can protocol to correctly handle the local
> echo.
> >
> > Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>
> 
> Patch looks quite good. Can you please wrap the description to about 72
> chars?

Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Marc Kleine-Budde]

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

On 10/31/2011 11:43 PM, Reuben Dowle wrote:
> Currently the flexcan driver uses hardware local echo. This blindly
> echoes all transmitted frames to all receiving sockets, regardless what
> CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> 
> This patch now submits transmitted frames to be echoed in the transmit
> complete interrupt, preserving the reference to the sending socket.
> This allows the can protocol to correctly handle the local echo.
> 
> Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>

there were some comments inline in my original mail.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

RE: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Reuben Dowle]

> 
> Patch looks quite good. Can you please wrap the description to about 72
> chars?

Will do.

> >  	}
> >
> > @@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device
> *dev)
> >  	int err;
> >  	u32 reg_mcr, reg_ctrl;
> >
> > +	can_free_echo_skb(dev, 0);
> 
> what about putting this to flexcan_chip_stop? Otherwise you risk a
> memleak if you do "ifconfig down; rmmod flexcan"

I originally thought this needs to be in flexcan_chip_start, so the bus off followed by restart sequence will drop the loopback packet. I investigated a bit deeper and can_flush_echo_skb is called in can_restart(). So my call of that function is not really needed. can_flush_echo_skb is also called in close_candev, which is called by the flexcan driver. So I actually think this function does not need to be called by the driver at all. What do you think?

> >
> >  	dev->netdev_ops = &flexcan_netdev_ops;
> >  	dev->irq = irq;
> > -	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
> > +	
> > +	/* Driver supports local echo.
> > +	 * We support local echo in hardware, however this is not used
> because
> > +	 * hardware local echo loses the sending socket reference
> > +	 * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket
> options
> > +	 *  would not work)
> > +	 */
> 
> IMHO, you can skip this comment. The patch description is good enough.

Ok, I will drop the extra comments from the patch.

Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Kurt Van Dijck]

On Tue, Nov 01, 2011 at 11:18:03AM +1300, Reuben Dowle wrote:
> Currently the flexcan driver uses hardware local echo. This blindly echos all transmitted frames to all receiving sockets, regardless what CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> 
> This patch now submits transmitted frames to be echoed in the transmit complete interrupt, preserving the reference to the sending socket. This allows the can protocol to correctly handle the local echo.
> 
> Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>
> 
> ---
>  drivers/net/can/flexcan.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index e023379..542ada8 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
>  	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
>  
> -	kfree_skb(skb);
> +	can_put_echo_skb(skb, dev, 0);
>  
>  	/* tx_packets is incremented in flexcan_irq */
>  	stats->tx_bytes += cf->can_dlc;
Why not move the statistics to the place where the echo_skb is popped?
When no frame gets out (due to whatever reason), the statistics may have
incremented.

Or maybe this needs a seperate patch?

The rest looks good.

Kurt

RE: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Reuben Dowle]

> >  	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
> >
> > -	kfree_skb(skb);
> > +	can_put_echo_skb(skb, dev, 0);
> >
> >  	/* tx_packets is incremented in flexcan_irq */
> >  	stats->tx_bytes += cf->can_dlc;
> Why not move the statistics to the place where the echo_skb is popped?
> When no frame gets out (due to whatever reason), the statistics may
> have
> incremented.
> 
> Or maybe this needs a seperate patch?
> 
> The rest looks good.
> 
> Kurt

I think you are right - incrementing the tx_bytes at this point does not seem right. It makes most sense to increment the bytes and packets at the same time when tx is confirmed by the interrupt. Looking through the various can drivers, there seems to be 2 approaches to this: at91, slcan and flexcan are incrementing tx_bytes when tx is started, and tx_packets in complete irq, whereas vcan, mcp251x, pch, hecc, softing, sja1000, c_can do it in tx complete irq.

So I think this is a slightly different issue to the one I was patching, which affects a couple of drivers. The majority of drivers seem to be doing it the way you suggest, but I am not familiar enough with the linux can layer to say one way or the other. I think if it is done how flexcan is doing it, then you would get tx_bytes always gets incremented, even when the tx is dropped due to bus off.

If we change the behaviour of flexcan, I think at91 and slcan should be changed too, because this should be handled consistently across all drivers I would think.

Reuben

Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Oliver Hartkopp]

On 02.11.2011 20:46, Reuben Dowle wrote:

>>>  	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
>>>
>>> -	kfree_skb(skb);
>>> +	can_put_echo_skb(skb, dev, 0);
>>>
>>>  	/* tx_packets is incremented in flexcan_irq */
>>>  	stats->tx_bytes += cf->can_dlc;
>> Why not move the statistics to the place where the echo_skb is popped?
>> When no frame gets out (due to whatever reason), the statistics may
>> have
>> incremented.
>>
>> Or maybe this needs a seperate patch?
>>
>> The rest looks good.
>>
>> Kurt
> 
> I think you are right - incrementing the tx_bytes at this point does not seem right. It makes most sense to increment the bytes and packets at the same time when tx is confirmed by the interrupt. Looking through the various can drivers, there seems to be 2 approaches to this: at91, slcan and flexcan are incrementing tx_bytes when tx is started, and tx_packets in complete irq, whereas vcan, mcp251x, pch, hecc, softing, sja1000, c_can do it in tx complete irq.
> 
> So I think this is a slightly different issue to the one I was patching, which affects a couple of drivers. The majority of drivers seem to be doing it the way you suggest, but I am not familiar enough with the linux can layer to say one way or the other. I think if it is done how flexcan is doing it, then you would get tx_bytes always gets incremented, even when the tx is dropped due to bus off.
> 
> If we change the behaviour of flexcan, I think at91 and slcan should be changed too, because this should be handled consistently across all drivers I would think.


At least the slcan driver does not support the correct echo of CAN frames on
driver level at all. The slcan driver adapts CAN frames to a serial line CAN
adapter where the feedback of successful transmission is not
guaranteed/checked within the serial data stream. Therefore the interface flag
IFF_ECHO is not set in the slcan driver and the local echo is performed in
af_can.c (as a workaround).

Regards,
Oliver

RE: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Reuben Dowle]

> >
> > If we change the behaviour of flexcan, I think at91 and slcan should
> be changed too, because this should be handled consistently across all
> drivers I would think.
> 
> 
> At least the slcan driver does not support the correct echo of CAN
> frames on
> driver level at all. The slcan driver adapts CAN frames to a serial
> line CAN
> adapter where the feedback of successful transmission is not
> guaranteed/checked within the serial data stream. Therefore the
> interface flag
> IFF_ECHO is not set in the slcan driver and the local echo is performed
> in
> af_can.c (as a workaround).
> 
> Regards,
> Oliver

Thats fine for the echo behaviour, but does not really answer why the slcan
(and flexcan) driver is setting the stats for tx_packets and tx_bytes at 
different points in the transmission logic. To my mind, either we think the
packet has transmitted (along with its bytes) so we should increment both,
or we should increment neither.

Reuben

[PATCH 0/2] clean up tx_bytes accounting

[Marc Kleine-Budde]

Hello,

what about turning can_get_echo_skb() into a helper function which returns the number
of tx'ed bytes.

Marc

[PATCH 1/2] can: dev: let can_get_echo_skb() return dlc of CAN frame

[Marc Kleine-Budde]

can_get_echo_skb() is usually called in the TX complete handler.
The stats->tx_packets and stats->tx_bytes should be updated there, too.
This patch simplifies to figure out the size of the sent CAN frame.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev.c   |   10 +++++++++-
 include/linux/can/dev.h |    2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 25695bd..b351632 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -327,16 +327,24 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb);
  * is handled in the device driver. The driver must protect
  * access to priv->echo_skb, if necessary.
  */
-void can_get_echo_skb(struct net_device *dev, unsigned int idx)
+unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
 	BUG_ON(idx >= priv->echo_skb_max);
 
 	if (priv->echo_skb[idx]) {
+		struct sk_buff *skb = priv->echo_skb[idx];
+		struct can_frame *cf = (struct can_frame *)skb->data;
+		u8 dlc = cf->can_dlc;
+
 		netif_rx(priv->echo_skb[idx]);
 		priv->echo_skb[idx] = NULL;
+
+		return dlc;
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(can_get_echo_skb);
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index a0969fc..5d2efe7 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -92,7 +92,7 @@ void can_bus_off(struct net_device *dev);
 
 void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		      unsigned int idx);
-void can_get_echo_skb(struct net_device *dev, unsigned int idx);
+unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
 void can_free_echo_skb(struct net_device *dev, unsigned int idx);
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
-- 
1.7.4.1

[PATCH 2/2] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Marc Kleine-Budde]

From: Reuben Dowle <Reuben.Dowle@navico.com>

Currently the flexcan driver uses hardware local echo. This blindly
echos all transmitted frames to all receiving sockets, regardless what
CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.

This patch now submits transmitted frames to be echoed in the transmit
complete interrupt, preserving the reference to the sending
socket. This allows the can protocol to correctly handle the local
echo.

Further this patch moves tx_bytes statistic accounting into the tx_complete
handler.

Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>
[mkl: move tx_bytes accounting into tx_complete handler; cleanups]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e023379..ea7b668 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -269,7 +269,6 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
 static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	const struct flexcan_priv *priv = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
 	struct flexcan_regs __iomem *regs = priv->base;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	u32 can_id;
@@ -299,14 +298,11 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		flexcan_write(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
 	}
 
+	can_put_echo_skb(skb, dev, 0);
+
 	flexcan_write(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
 	flexcan_write(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
 
-	kfree_skb(skb);
-
-	/* tx_packets is incremented in flexcan_irq */
-	stats->tx_bytes += cf->can_dlc;
-
 	return NETDEV_TX_OK;
 }
 
@@ -609,9 +605,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 
 	/* transmission complete interrupt */
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
-		/* tx_bytes is incremented in flexcan_start_xmit */
+		stats->tx_bytes += can_get_echo_skb(dev, 0);
 		stats->tx_packets++;
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
+		can_get_echo_skb(dev, 0);
 		netif_wake_queue(dev);
 	}
 
@@ -697,12 +694,13 @@ static int flexcan_chip_start(struct net_device *dev)
 	 * only supervisor access
 	 * enable warning int
 	 * choose format C
+	 * disable local echo
 	 *
 	 */
 	reg_mcr = flexcan_read(&regs->mcr);
 	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
 		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
-		FLEXCAN_MCR_IDAM_C;
+		FLEXCAN_MCR_IDAM_C | FLEXCAN_MCR_SRX_DIS;
 	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
 	flexcan_write(reg_mcr, &regs->mcr);
 
@@ -970,7 +968,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		goto failed_map;
 	}
 
-	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
+	dev = alloc_candev(sizeof(struct flexcan_priv), 1);
 	if (!dev) {
 		err = -ENOMEM;
 		goto failed_alloc;
@@ -978,7 +976,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 
 	dev->netdev_ops = &flexcan_netdev_ops;
 	dev->irq = irq;
-	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
+	dev->flags |= IFF_ECHO;
 
 	priv = netdev_priv(dev);
 	priv->can.clock.freq = clock_freq;
-- 
1.7.4.1

Re: [PATCH 2/2] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Marc Kleine-Budde]

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

On 11/03/2011 10:47 AM, Marc Kleine-Budde wrote:
> From: Reuben Dowle <Reuben.Dowle@navico.com>
> 
> Currently the flexcan driver uses hardware local echo. This blindly
> echos all transmitted frames to all receiving sockets, regardless what
> CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK are set to.
> 
> This patch now submits transmitted frames to be echoed in the transmit
> complete interrupt, preserving the reference to the sending
> socket. This allows the can protocol to correctly handle the local
> echo.
> 
> Further this patch moves tx_bytes statistic accounting into the tx_complete
> handler.
> 
> Signed-off-by: Reuben Dowle <reuben.dowle@navico.com>
> [mkl: move tx_bytes accounting into tx_complete handler; cleanups]
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---

A changelog would be nice :)

Since v1 (Reuben's original version):
- wrap patch description to 80 lines
- remove can_free_echo_skb() from flexcan_chip_start()
  not needed as Reuben pointed out
- remove comments about IFF_ECHO; see patch description for this
- closed potential race condition in flexcan_start_xmit()
  first call can_put_echo_skb(), then trigger xmit
- moved tx_bytes accounting to tx_complete handler;
  make use of new can_get_echo_skb()

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/2] clean up tx_bytes accounting

[Kurt Van Dijck]

On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
> Hello,
> 
> what about turning can_get_echo_skb() into a helper function which returns the number
> of tx'ed bytes.
That would work.

Next step would be to do the statistics inside can_get_echo_skb(), but that's
affecting all drivers using it ...
Kurt

> 
> Marc
> 
> 

Re: [PATCH 2/2] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Oliver Hartkopp]

On 03.11.2011 10:47, Marc Kleine-Budde wrote:

> @@ -609,9 +605,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  
>  	/* transmission complete interrupt */
>  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
> -		/* tx_bytes is incremented in flexcan_start_xmit */
> +		stats->tx_bytes += can_get_echo_skb(dev, 0);
>  		stats->tx_packets++;
>  		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
> +		can_get_echo_skb(dev, 0);
>  		netif_wake_queue(dev);
>  	}


What's the reason for the second can_get_echo_skb() here?

IIRC the skb is already netif_rx'ed and consumed by the first attempt.

Regards,
Oliver

Re: [PATCH 2/2] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Marc Kleine-Budde]

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

On 11/03/2011 11:03 AM, Oliver Hartkopp wrote:
> On 03.11.2011 10:47, Marc Kleine-Budde wrote:
> 
>> @@ -609,9 +605,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>  
>>  	/* transmission complete interrupt */
>>  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>> -		/* tx_bytes is incremented in flexcan_start_xmit */
>> +		stats->tx_bytes += can_get_echo_skb(dev, 0);
>>  		stats->tx_packets++;
>>  		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
>> +		can_get_echo_skb(dev, 0);
>>  		netif_wake_queue(dev);
>>  	}
> 
> 
> What's the reason for the second can_get_echo_skb() here?
doh!

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/2] clean up tx_bytes accounting

[Marc Kleine-Budde]

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

On 11/03/2011 11:03 AM, Kurt Van Dijck wrote:
> On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
>> Hello,
>>
>> what about turning can_get_echo_skb() into a helper function which returns the number
>> of tx'ed bytes.
> That would work.
> 
> Next step would be to do the statistics inside can_get_echo_skb(), but that's
> affecting all drivers using it ...
> Kurt

Interesting idea

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/2] clean up tx_bytes accounting

[Oliver Hartkopp]

On 03.11.2011 11:42, Marc Kleine-Budde wrote:

> On 11/03/2011 11:03 AM, Kurt Van Dijck wrote:
>> On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
>>> Hello,
>>>
>>> what about turning can_get_echo_skb() into a helper function which returns the number
>>> of tx'ed bytes.
>> That would work.
>>
>> Next step would be to do the statistics inside can_get_echo_skb(), but that's
>> affecting all drivers using it ...
>> Kurt
> 
> Interesting idea


Yes, but then the name of the function is not appropriate anymore.

The return value for can_get_echo_skb() gives a silent improvement that can be
adopted by CAN drivers by the time. But hiding more functionality inside this
function may lead to misunderstandings.

Regards,
Oliver

Re: [PATCH 0/2] clean up tx_bytes accounting

[Kurt Van Dijck]

On Thu, Nov 03, 2011 at 03:27:38PM +0100, Oliver Hartkopp wrote:
> On 03.11.2011 11:42, Marc Kleine-Budde wrote:
> 
> > On 11/03/2011 11:03 AM, Kurt Van Dijck wrote:
> >> On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
> >>> Hello,
> >>>
> >>> what about turning can_get_echo_skb() into a helper function which returns the number
> >>> of tx'ed bytes.
> >> That would work.
> >>
> >> Next step would be to do the statistics inside can_get_echo_skb(), but that's
> >> affecting all drivers using it ...
> >> Kurt
> > 
> > Interesting idea
> 
> 
> Yes, but then the name of the function is not appropriate anymore.
> 
> The return value for can_get_echo_skb() gives a silent improvement that can be
> adopted by CAN drivers by the time. But hiding more functionality inside this
> function may lead to misunderstandings.
That's what I described with the obscure phrase "..." :-)

Marc's proposed patch is indeed easier to digest at the time.
> 
Regards,
Kurt

Re: [PATCH] flexcan: Fix CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK

[Oliver Hartkopp]

On 02.11.2011 22:31, Reuben Dowle wrote:

>>>
>>> If we change the behaviour of flexcan, I think at91 and slcan should
>> be changed too, because this should be handled consistently across all
>> drivers I would think.
>>
>>
>> At least the slcan driver does not support the correct echo of CAN
>> frames on
>> driver level at all. The slcan driver adapts CAN frames to a serial
>> line CAN
>> adapter where the feedback of successful transmission is not
>> guaranteed/checked within the serial data stream. Therefore the
>> interface flag
>> IFF_ECHO is not set in the slcan driver and the local echo is performed
>> in
>> af_can.c (as a workaround).
>>
>> Regards,
>> Oliver
> 
> Thats fine for the echo behaviour, but does not really answer why the slcan
> (and flexcan) driver is setting the stats for tx_packets and tx_bytes at 
> different points in the transmission logic. To my mind, either we think the
> packet has transmitted (along with its bytes) so we should increment both,
> or we should increment neither.


In slcan.c the points of incrementing tx_bytes and tx_packets have been
adopted from slip.c where slcan.c heavily bases on.

Indeed one could argument that tx_packets could be incremented in slc_encaps
together with tx_bytes. This means that the packet is sent.

But the packet is surely sent in slcan_write_wakeup(), when tx_packets is
incremented. Unfortunately we do not have the can_dlc available at this point
anymore.

If you think it's worth the effort tx_packets++ could be integrated into
slc_encaps() to have a consistent statistic update (but you don't know whether
the packet hit the serial line entirely).

Additionally slip.c does it the same (inconsistent) way for the reasons above
and therefore i tend not to change it to stay on the current serial netdevices
behaviour (for slcan as only CAN netdevice).

Regards,
Oliver

Re: [PATCH 0/2] clean up tx_bytes accounting

[Marc Kleine-Budde]

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

On 11/03/2011 03:37 PM, Kurt Van Dijck wrote:
> On Thu, Nov 03, 2011 at 03:27:38PM +0100, Oliver Hartkopp wrote:

>>>> Next step would be to do the statistics inside can_get_echo_skb(), but that's
>>>> affecting all drivers using it ...
>>>> Kurt
>>>
>>> Interesting idea
>>
>>
>> Yes, but then the name of the function is not appropriate anymore.
>>
>> The return value for can_get_echo_skb() gives a silent improvement that can be
>> adopted by CAN drivers by the time. But hiding more functionality inside this
>> function may lead to misunderstandings.
> That's what I described with the obscure phrase "..." :-)
> 
> Marc's proposed patch is indeed easier to digest at the time.

+1, I'll repost my fixed branch. Let's first integrate that one.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]