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

login
register
mail settings
Submitter Marc Kleine-Budde
Date Nov. 3, 2011, 9:47 a.m.
Message ID <1320313675-30749-3-git-send-email-mkl@pengutronix.de>
Download mbox | patch
Permalink /patch/123425/
State Deferred
Delegated to: David Miller
Headers show

Comments

Marc Kleine-Budde - Nov. 3, 2011, 9:47 a.m.
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(-)
Marc Kleine-Budde - Nov. 3, 2011, 9:59 a.m.
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
Oliver Hartkopp - Nov. 3, 2011, 10:03 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde - Nov. 3, 2011, 10:17 a.m.
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

Patch

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;