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 <redacted>
---
drivers/net/can/flexcan.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
@@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)gotofailed_map;}-dev=alloc_candev(sizeof(structflexcan_priv),0);+dev=alloc_candev(sizeof(structflexcan_priv),1);if(!dev){err=-ENOMEM;gotofailed_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.+*Wesupportlocalechoinhardware,howeverthisisnotusedbecause+*hardwarelocalecholosesthesendingsocketreference+*(thusCAN_RAW_RECV_OWN_MSGSandCAN_RAW_LOOPBACKsocketoptions+*wouldnotwork)+*/+dev->flags|=IFF_ECHO;priv=netdev_priv(dev);priv->can.clock.freq=clock_freq;
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-10-31 22:31:02
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 <redacted>
Patch looks quite good. Can you please wrap the description to about 72
chars?
@@ -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 |
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 <redacted>
---
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,®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);flexcan_write(ctrl,®s->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),®s->iflag1);+can_get_echo_skb(dev,0);netif_wake_queue(dev);}
@@ -970,7 +974,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)gotofailed_map;}-dev=alloc_candev(sizeof(structflexcan_priv),0);+dev=alloc_candev(sizeof(structflexcan_priv),1);if(!dev){err=-ENOMEM;gotofailed_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.+*Wesupportlocalechoinhardware,howeverthisisnotusedbecause+*hardwarelocalecholosesthesendingsocketreference+*(thusCAN_RAW_RECV_OWN_MSGSandCAN_RAW_LOOPBACKsocketoptions+*wouldnotwork)+*/+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:
quoted
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.
quoted
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.
quoted
Signed-off-by: Reuben Dowle <redacted>
Patch looks quite good. Can you please wrap the description to about 72
chars?
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-10-31 22:49:48
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 <redacted>
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 |
Patch looks quite good. Can you please wrap the description to about 72
chars?
Will do.
quoted
}
@@ -670,6 +671,8 @@ static int flexcan_chip_start(struct net_device
*dev)
quoted
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?
quoted
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
quoted
+ * hardware local echo loses the sending socket reference
+ * (thus CAN_RAW_RECV_OWN_MSGS and CAN_RAW_LOOPBACK socket
options
quoted
+ * 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.
From: Kurt Van Dijck <hidden> Date: 2011-11-02 08:04:38
On Tue, Nov 01, 2011 at 11:18:03AM +1300, Reuben Dowle wrote:
quoted hunk
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 <redacted>
---
drivers/net/can/flexcan.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
@@ -302,7 +302,7 @@ static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)flexcan_write(can_id,®s->cantxfg[FLEXCAN_TX_BUF_ID].can_id);flexcan_write(ctrl,®s->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
flexcan_write(ctrl, ®s->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
From: Oliver Hartkopp <socketcan@hartkopp.net> Date: 2011-11-02 20:30:59
On 02.11.2011 20:46, Reuben Dowle wrote:
quoted
quoted
flexcan_write(ctrl, ®s->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
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
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-11-03 09:47:54
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(-)
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-11-03 09:47:55
From: Reuben Dowle <redacted>
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 <redacted>
[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(-)
@@ -970,7 +968,7 @@ static int __devinit flexcan_probe(struct platform_device *pdev)gotofailed_map;}-dev=alloc_candev(sizeof(structflexcan_priv),0);+dev=alloc_candev(sizeof(structflexcan_priv),1);if(!dev){err=-ENOMEM;gotofailed_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;
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-11-03 09:59:23
On 11/03/2011 10:47 AM, Marc Kleine-Budde wrote:
From: Reuben Dowle <redacted>
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 <redacted>
[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 |
From: Oliver Hartkopp <socketcan@hartkopp.net> Date: 2011-11-03 14:27:38
On 03.11.2011 11:42, Marc Kleine-Budde wrote:
On 11/03/2011 11:03 AM, Kurt Van Dijck wrote:
quoted
On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
quoted
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
From: Kurt Van Dijck <hidden> Date: 2011-11-03 14:37:52
On Thu, Nov 03, 2011 at 03:27:38PM +0100, Oliver Hartkopp wrote:
On 03.11.2011 11:42, Marc Kleine-Budde wrote:
quoted
On 11/03/2011 11:03 AM, Kurt Van Dijck wrote:
quoted
On Thu, Nov 03, 2011 at 10:47:53AM +0100, Marc Kleine-Budde wrote:
quoted
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.
From: Oliver Hartkopp <socketcan@hartkopp.net> Date: 2011-11-03 14:54:50
On 02.11.2011 22:31, Reuben Dowle wrote:
quoted
quoted
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
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2011-11-03 15:03:23
On 11/03/2011 03:37 PM, Kurt Van Dijck wrote:
On Thu, Nov 03, 2011 at 03:27:38PM +0100, Oliver Hartkopp wrote:
quoted
quoted
quoted
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 |