Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

Hello everyone,

I'm finally implementing SocketCAN support in our internal codebase, and
have run across some issues. We previously used a character driver provided
by the vendor.

I myself implemented the driver for the Janz ICAN3 card. It has been in
mainline Linux since 2.6.35.

The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
messages go through the microcontroller firmware.

This firmware has the following features:
- it does have hardware-supported local loopback
- it does NOT have any sort of "tx-complete" notification or interrupt
- it does NOT have any indication that a frame went through the
  hardware-supported local loopback

To work around the lack of "tx-complete" interrupts, I used the hardware
local loopback feature. Every frame has hardware loopback set, which causes
the ican3_napi() routine to be called for each frame sent. This works fine,
except that sockets created with the default options (loopback on,
can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.

QUESTION 1:
The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?

QUESTION 2:
The socketcan test tst-rcv-own-msgs says:
Starting PF_CAN frame flow test.
checking socket default settings ... failure!

If I understand the code correctly, this means that the sending socket
received an echo frame when it should not have received one.

This matches what I expect the code to do.

How can I fix this?

OBSERVATION 1:
The following patch drops the relevant calls to netif_stop_queue() and
netif_wake_queue(), as well as the hardware-supported local loopback.

The tst-rcv-own-msgs test passes.

Is dropping these API calls a reasonable thing to do?

Thanks,
Ira



diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..b8bf972 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -845,7 +845,9 @@ static void can_frame_to_ican3(struct ican3_dev *mod,
 	/* we always use the extended format, with the ECHO flag set */
 	desc->command = ICAN3_CAN_TYPE_EFF;
 	desc->data[0] |= cf->can_dlc;
+#if 0
 	desc->data[1] |= ICAN3_ECHO;
+#endif
 
 	if (cf->can_id & CAN_RTR_FLAG)
 		desc->data[0] |= ICAN3_EFF_RTR;
@@ -1206,9 +1208,11 @@ static int ican3_napi(struct napi_struct *napi, int budget)
 
 	spin_lock_irqsave(&mod->lock, flags);
 
+#if 0
 	/* Wake up the transmit queue if necessary */
 	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
 		netif_wake_queue(mod->ndev);
+#endif
 
 	spin_unlock_irqrestore(&mod->lock, flags);
 
@@ -1426,8 +1430,10 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* check that we can actually transmit */
 	if (!ican3_txok(mod)) {
+#if 0
 		dev_err(mod->dev, "no free descriptors, stopping queue\n");
 		netif_stop_queue(ndev);
+#endif
 		spin_unlock_irqrestore(&mod->lock, flags);
 		return NETDEV_TX_BUSY;
 	}
@@ -1471,9 +1477,11 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	 * It will be woken when the ECHO skb for the current packet is recv'd.
 	 */
 
+#if 0
 	/* copy the control bits of the descriptor */
 	if (!ican3_txok(mod))
 		netif_stop_queue(ndev);
+#endif
 
 	spin_unlock_irqrestore(&mod->lock, flags);
 	return NETDEV_TX_OK;
@@ -1678,7 +1686,9 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	mod->free_page = DPM_FREE_START;
 
 	ndev->netdev_ops = &ican3_netdev_ops;
+#if 0
 	ndev->flags |= IFF_ECHO;
+#endif
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	mod->can.clock.freq = ICAN3_CAN_CLOCK;

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Wolfgang Grandegger]

Hi Ira,

On 07/04/2012 01:23 AM, Ira W. Snyder wrote:
> Hello everyone,
> 
> I'm finally implementing SocketCAN support in our internal codebase, and
> have run across some issues. We previously used a character driver provided
> by the vendor.
> 
> I myself implemented the driver for the Janz ICAN3 card. It has been in
> mainline Linux since 2.6.35.
> 
> The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
> messages go through the microcontroller firmware.
> 
> This firmware has the following features:
> - it does have hardware-supported local loopback
> - it does NOT have any sort of "tx-complete" notification or interrupt
> - it does NOT have any indication that a frame went through the
>   hardware-supported local loopback
> 
> To work around the lack of "tx-complete" interrupts, I used the hardware
> local loopback feature. Every frame has hardware loopback set, which causes
> the ican3_napi() routine to be called for each frame sent. This works fine,
> except that sockets created with the default options (loopback on,
> can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.
> 
> QUESTION 1:
> The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?

Not necessarily. IIRC, the early versions of the Flexcan driver used the
hardware loopback as well but switched to classical echo-skb handling
because of the problem with recv-own-msgs.

> QUESTION 2:
> The socketcan test tst-rcv-own-msgs says:
> Starting PF_CAN frame flow test.
> checking socket default settings ... failure!
> 
> If I understand the code correctly, this means that the sending socket
> received an echo frame when it should not have received one.
> 
> This matches what I expect the code to do.
> 
> How can I fix this?
> 
> OBSERVATION 1:
> The following patch drops the relevant calls to netif_stop_queue() and
> netif_wake_queue(), as well as the hardware-supported local loopback.
> 
> The tst-rcv-own-msgs test passes.
> 
> Is dropping these API calls a reasonable thing to do?

Well, that controller will not allow to support all features and options
and we need to make a choice:

 recv-own-msgs support versus proper tx-done handling (correct timing)

As the recv-own-msgs is not used frequently, it would not be really
dramatic if the feature is not available. Just my 0.02 cent.

Wolfgang.

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Kurt Van Dijck]

On Wed, Jul 04, 2012 at 11:18:38AM +0200, Wolfgang Grandegger wrote:
> Hi Ira,
> 
> On 07/04/2012 01:23 AM, Ira W. Snyder wrote:
> > Hello everyone,
> > 
> > I'm finally implementing SocketCAN support in our internal codebase, and
> > have run across some issues. We previously used a character driver provided
> > by the vendor.
> > 
> > I myself implemented the driver for the Janz ICAN3 card. It has been in
> > mainline Linux since 2.6.35.
> > 
> > The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
> > messages go through the microcontroller firmware.
> > 
> > This firmware has the following features:
> > - it does have hardware-supported local loopback
> > - it does NOT have any sort of "tx-complete" notification or interrupt
> > - it does NOT have any indication that a frame went through the
> >   hardware-supported local loopback
> > 
> > To work around the lack of "tx-complete" interrupts, I used the hardware
> > local loopback feature. Every frame has hardware loopback set, which causes
> > the ican3_napi() routine to be called for each frame sent. This works fine,
> > except that sockets created with the default options (loopback on,
> > can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.
> > 
> > QUESTION 1:
> > The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?
> 
> Not necessarily. IIRC, the early versions of the Flexcan driver used the
> hardware loopback as well but switched to classical echo-skb handling
> because of the problem with recv-own-msgs.
> 
> > QUESTION 2:
> > The socketcan test tst-rcv-own-msgs says:
> > Starting PF_CAN frame flow test.
> > checking socket default settings ... failure!
> > 
> > If I understand the code correctly, this means that the sending socket
> > received an echo frame when it should not have received one.
> > 
> > This matches what I expect the code to do.
> > 
> > How can I fix this?
> > 
> > OBSERVATION 1:
> > The following patch drops the relevant calls to netif_stop_queue() and
> > netif_wake_queue(), as well as the hardware-supported local loopback.
> > 
> > The tst-rcv-own-msgs test passes.
> > 
> > Is dropping these API calls a reasonable thing to do?

I'm not sure.
> 
> Well, that controller will not allow to support all features and options
> and we need to make a choice:
> 
>  recv-own-msgs support versus proper tx-done handling (correct timing)
> 
> As the recv-own-msgs is not used frequently, it would not be really
> dramatic if the feature is not available.

My opinion:

1) is the correct timing of tx-done more used than recv-own-msgs?
   I think such choices are really hard to make.
   Avoid choosing if possible.

2) To avoid choosing, you could:
   a. use can_put_echo_skb()
   b. when an rx interrupt && the frame matches the next echo_skb,
      drop the rx frame and deliver echo_skb.
   I realise that b. needs some code in generic can_dev (something like
   can_peek_echo_skb()), and costs you some performance.

> Just my 0.02 cent.
0.02 EUR or 2 cents, but 0.02 cents :-) ?
   
-- 
Kurt Van Dijck

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp]

On 04.07.2012 14:07, Kurt Van Dijck wrote:

> On Wed, Jul 04, 2012 at 11:18:38AM +0200, Wolfgang Grandegger wrote:
>> Hi Ira,
>>
>> On 07/04/2012 01:23 AM, Ira W. Snyder wrote:
>>> Hello everyone,
>>>
>>> I'm finally implementing SocketCAN support in our internal codebase, and
>>> have run across some issues. We previously used a character driver provided
>>> by the vendor.
>>>
>>> I myself implemented the driver for the Janz ICAN3 card. It has been in
>>> mainline Linux since 2.6.35.
>>>
>>> The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
>>> messages go through the microcontroller firmware.
>>>
>>> This firmware has the following features:
>>> - it does have hardware-supported local loopback
>>> - it does NOT have any sort of "tx-complete" notification or interrupt
>>> - it does NOT have any indication that a frame went through the
>>>   hardware-supported local loopback
>>>
>>> To work around the lack of "tx-complete" interrupts, I used the hardware
>>> local loopback feature. Every frame has hardware loopback set, which causes
>>> the ican3_napi() routine to be called for each frame sent. This works fine,
>>> except that sockets created with the default options (loopback on,
>>> can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.
>>>
>>> QUESTION 1:
>>> The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?
>>
>> Not necessarily. IIRC, the early versions of the Flexcan driver used the
>> hardware loopback as well but switched to classical echo-skb handling
>> because of the problem with recv-own-msgs.
>>
>>> QUESTION 2:
>>> The socketcan test tst-rcv-own-msgs says:
>>> Starting PF_CAN frame flow test.
>>> checking socket default settings ... failure!
>>>
>>> If I understand the code correctly, this means that the sending socket
>>> received an echo frame when it should not have received one.
>>>
>>> This matches what I expect the code to do.
>>>
>>> How can I fix this?
>>>
>>> OBSERVATION 1:
>>> The following patch drops the relevant calls to netif_stop_queue() and
>>> netif_wake_queue(), as well as the hardware-supported local loopback.
>>>
>>> The tst-rcv-own-msgs test passes.
>>>
>>> Is dropping these API calls a reasonable thing to do?
> 
> I'm not sure.
>>
>> Well, that controller will not allow to support all features and options
>> and we need to make a choice:
>>
>>  recv-own-msgs support versus proper tx-done handling (correct timing)
>>
>> As the recv-own-msgs is not used frequently, it would not be really
>> dramatic if the feature is not available.
> 
> My opinion:
> 
> 1) is the correct timing of tx-done more used than recv-own-msgs?
>    I think such choices are really hard to make.
>    Avoid choosing if possible.
> 
> 2) To avoid choosing, you could:
>    a. use can_put_echo_skb()
>    b. when an rx interrupt && the frame matches the next echo_skb,
>       drop the rx frame and deliver echo_skb.
>    I realise that b. needs some code in generic can_dev (something like
>    can_peek_echo_skb()), and costs you some performance.
> 


Hello all,

i also thought about your case 'b' and it could be a good choice indeed.

First it is generally an unusual case that you receive a CAN-ID from the
'outside' that is sent from the local host. But of course we can not assume
this 'good CAN network design rule' is to be implemented always ...

Anyway we currently have the flexcan and the ican3 hardware that generally
supports a loopback on hardware level but looses the reference to the tx-skb
and therefore to the sk pointer which is used to identify the originating
socket for recv-own-msgs support.

If we receive a new CAN frame and there are any CAN frames stored in the
echo_skb cache, we can compare the received frame with the cached frames.

If we find an identical CAN frame, we get the echo_skb and push it into the
receive path. For that reason the flexcan and ican3 driver should only
allocate a new rx skb when there's no matching echo_skb available.

What could happen, if we (unlikely) *really* receive an identical CAN frame
content from the 'outside' we already have in the echo_skb cache?

Well the only thing that comes in mind is that we would mix these two
*identical* frames in their order and therefore in their timestamp. Additional
it can be assumed that the second 'identical' frame is following instantaneous.

To me this looks like a acceptable risk - especially as it is opposed to the
'good CAN network design rule' to have identical CAN-IDs.

@Ira: Do you want to propose a patch which adds the echo_skb cache check for
received CAN frames for the ican3 driver?

Regards,
Oliver

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

On Wed, Jul 04, 2012 at 06:23:46PM +0200, Oliver Hartkopp wrote:
> On 04.07.2012 14:07, Kurt Van Dijck wrote:
> 
> > On Wed, Jul 04, 2012 at 11:18:38AM +0200, Wolfgang Grandegger wrote:
> >> Hi Ira,
> >>
> >> On 07/04/2012 01:23 AM, Ira W. Snyder wrote:
> >>> Hello everyone,
> >>>
> >>> I'm finally implementing SocketCAN support in our internal codebase, and
> >>> have run across some issues. We previously used a character driver provided
> >>> by the vendor.
> >>>
> >>> I myself implemented the driver for the Janz ICAN3 card. It has been in
> >>> mainline Linux since 2.6.35.
> >>>
> >>> The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
> >>> messages go through the microcontroller firmware.
> >>>
> >>> This firmware has the following features:
> >>> - it does have hardware-supported local loopback
> >>> - it does NOT have any sort of "tx-complete" notification or interrupt
> >>> - it does NOT have any indication that a frame went through the
> >>>   hardware-supported local loopback
> >>>
> >>> To work around the lack of "tx-complete" interrupts, I used the hardware
> >>> local loopback feature. Every frame has hardware loopback set, which causes
> >>> the ican3_napi() routine to be called for each frame sent. This works fine,
> >>> except that sockets created with the default options (loopback on,
> >>> can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.
> >>>
> >>> QUESTION 1:
> >>> The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?
> >>
> >> Not necessarily. IIRC, the early versions of the Flexcan driver used the
> >> hardware loopback as well but switched to classical echo-skb handling
> >> because of the problem with recv-own-msgs.
> >>
> >>> QUESTION 2:
> >>> The socketcan test tst-rcv-own-msgs says:
> >>> Starting PF_CAN frame flow test.
> >>> checking socket default settings ... failure!
> >>>
> >>> If I understand the code correctly, this means that the sending socket
> >>> received an echo frame when it should not have received one.
> >>>
> >>> This matches what I expect the code to do.
> >>>
> >>> How can I fix this?
> >>>
> >>> OBSERVATION 1:
> >>> The following patch drops the relevant calls to netif_stop_queue() and
> >>> netif_wake_queue(), as well as the hardware-supported local loopback.
> >>>
> >>> The tst-rcv-own-msgs test passes.
> >>>
> >>> Is dropping these API calls a reasonable thing to do?
> > 
> > I'm not sure.
> >>
> >> Well, that controller will not allow to support all features and options
> >> and we need to make a choice:
> >>
> >>  recv-own-msgs support versus proper tx-done handling (correct timing)
> >>
> >> As the recv-own-msgs is not used frequently, it would not be really
> >> dramatic if the feature is not available.
> > 
> > My opinion:
> > 
> > 1) is the correct timing of tx-done more used than recv-own-msgs?
> >    I think such choices are really hard to make.
> >    Avoid choosing if possible.
> > 
> > 2) To avoid choosing, you could:
> >    a. use can_put_echo_skb()
> >    b. when an rx interrupt && the frame matches the next echo_skb,
> >       drop the rx frame and deliver echo_skb.
> >    I realise that b. needs some code in generic can_dev (something like
> >    can_peek_echo_skb()), and costs you some performance.
> > 
> 
> 
> Hello all,
> 
> i also thought about your case 'b' and it could be a good choice indeed.
> 
> First it is generally an unusual case that you receive a CAN-ID from the
> 'outside' that is sent from the local host. But of course we can not assume
> this 'good CAN network design rule' is to be implemented always ...
> 
> Anyway we currently have the flexcan and the ican3 hardware that generally
> supports a loopback on hardware level but looses the reference to the tx-skb
> and therefore to the sk pointer which is used to identify the originating
> socket for recv-own-msgs support.
> 
> If we receive a new CAN frame and there are any CAN frames stored in the
> echo_skb cache, we can compare the received frame with the cached frames.
> 
> If we find an identical CAN frame, we get the echo_skb and push it into the
> receive path. For that reason the flexcan and ican3 driver should only
> allocate a new rx skb when there's no matching echo_skb available.
> 
> What could happen, if we (unlikely) *really* receive an identical CAN frame
> content from the 'outside' we already have in the echo_skb cache?
> 
> Well the only thing that comes in mind is that we would mix these two
> *identical* frames in their order and therefore in their timestamp. Additional
> it can be assumed that the second 'identical' frame is following instantaneous.
> 
> To me this looks like a acceptable risk - especially as it is opposed to the
> 'good CAN network design rule' to have identical CAN-IDs.
> 
> @Ira: Do you want to propose a patch which adds the echo_skb cache check for
> received CAN frames for the ican3 driver?
> 
> Regards,
> Oliver

Hello Oliver,

Is the following patch what you have in mind? I tested it on my board,
and it crashed after a few seconds, so there must be something wrong.
Any ideas?

I won't have access to the machine to do more testing until tomorrow.

Ira


diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c5fe3a3..f112f77 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -348,6 +348,39 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 EXPORT_SYMBOL_GPL(can_get_echo_skb);
 
 /*
+ * Compare an skb with an existing echo skb
+ *
+ * This function will be used on devices which have a hardware loopback.
+ * On these devices, this function can be used to compare a received skb
+ * with the saved echo skbs so that the hardware echo skb can be dropped.
+ *
+ * Returns true if the skb's are identical, false otherwise.
+ */
+bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev,
+		      unsigned int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_frame *cf = (struct can_frame *)skb->data;
+
+	BUG_ON(idx >= priv->echo_skb_max);
+
+	if (priv->echo_skb[idx]) {
+		struct sk_buff *echo_skb = priv->echo_skb[idx];
+		struct can_frame *echo_cf = (struct can_frame *)echo_skb->data;
+		if (cf->can_id != echo_cf->can_id)
+			return false;
+
+		if (cf->can_dlc != echo_cf->can_dlc)
+			return false;
+
+		return memcmp(cf->data, echo_cf->data, cf->can_dlc) == 0;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(can_cmp_echo_skb);
+
+/*
   * Remove the skb from the stack and free it.
   *
   * The function is typically called when TX failed.
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..b5de314 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -235,10 +235,11 @@ struct ican3_dev {
 
 	/* fast host interface */
 	unsigned int fastrx_start;
-	unsigned int fastrx_int;
 	unsigned int fastrx_num;
+	unsigned int fastrx_echo;
 	unsigned int fasttx_start;
 	unsigned int fasttx_num;
+	unsigned int fasttx_echo;
 
 	/* first free DPM page */
 	unsigned int free_page;
@@ -454,7 +455,7 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod)
 	/* save the start recv page */
 	mod->fastrx_start = mod->free_page;
 	mod->fastrx_num = 0;
-	mod->fastrx_int = 0;
+	mod->fastrx_echo = 0;
 
 	/* build a single fast tohost queue descriptor */
 	memset(&desc, 0, sizeof(desc));
@@ -491,6 +492,7 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod)
 	/* save the start xmit page */
 	mod->fasttx_start = mod->free_page;
 	mod->fasttx_num = 0;
+	mod->fasttx_echo = 0;
 
 	/* build a single fast fromhost queue descriptor */
 	memset(&desc, 0, sizeof(desc));
@@ -1150,6 +1152,14 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	/* convert the ICAN3 frame into Linux CAN format */
 	ican3_to_can_frame(mod, &desc, cf);
 
+	/* check if this is an ECHO frame */
+	if (can_cmp_echo_skb(skb, ndev, mod->fastrx_echo)) {
+		can_get_echo_skb(ndev, mod->fastrx_echo);
+		mod->fastrx_echo = (mod->fastrx_echo + 1) & (ICAN3_TX_BUFFERS - 1);
+		kfree_skb(skb);
+		goto err_noalloc;
+	}
+
 	/* receive the skb, update statistics */
 	netif_receive_skb(skb);
 	stats->rx_packets++;
@@ -1422,6 +1432,9 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	void __iomem *desc_addr;
 	unsigned long flags;
 
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
 	spin_lock_irqsave(&mod->lock, flags);
 
 	/* check that we can actually transmit */
@@ -1432,6 +1445,10 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 		return NETDEV_TX_BUSY;
 	}
 
+	/* add to the ECHO stack */
+	can_put_echo_skb(skb, ndev, mod->fasttx_echo);
+	mod->fasttx_echo = (mod->fasttx_echo + 1) & ~(ICAN3_TX_BUFFERS - 1);
+
 	/* copy the control bits of the descriptor */
 	ican3_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16));
 	desc_addr = mod->dpm + ((mod->fasttx_num % 16) * sizeof(desc));
@@ -1654,7 +1671,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	dev = &pdev->dev;
 
 	/* allocate the CAN device and private data */
-	ndev = alloc_candev(sizeof(*mod), 0);
+	ndev = alloc_candev(sizeof(*mod), ICAN3_TX_BUFFERS);
 	if (!ndev) {
 		dev_err(dev, "unable to allocate CANdev\n");
 		ret = -ENOMEM;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5d2efe7..904a938 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -93,6 +93,8 @@ void can_bus_off(struct net_device *dev);
 void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		      unsigned int idx);
 unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
+bool can_cmp_echo_skb(struct sk_buff *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);

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp]

On 04.07.2012 21:07, Ira W. Snyder wrote:


> Is the following patch what you have in mind? I tested it on my board,
> and it crashed after a few seconds, so there must be something wrong.
> Any ideas?
> 
> I won't have access to the machine to do more testing until tomorrow.
> 
> Ira
> 
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index c5fe3a3..f112f77 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -348,6 +348,39 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
>  EXPORT_SYMBOL_GPL(can_get_echo_skb);
>  
>  /*
> + * Compare an skb with an existing echo skb
> + *
> + * This function will be used on devices which have a hardware loopback.
> + * On these devices, this function can be used to compare a received skb
> + * with the saved echo skbs so that the hardware echo skb can be dropped.
> + *
> + * Returns true if the skb's are identical, false otherwise.
> + */
> +bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev,
> +		      unsigned int idx)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +
> +	BUG_ON(idx >= priv->echo_skb_max);
> +
> +	if (priv->echo_skb[idx]) {
> +		struct sk_buff *echo_skb = priv->echo_skb[idx];
> +		struct can_frame *echo_cf = (struct can_frame *)echo_skb->data;
> +		if (cf->can_id != echo_cf->can_id)
> +			return false;
> +
> +		if (cf->can_dlc != echo_cf->can_dlc)
> +			return false;
> +
> +		return memcmp(cf->data, echo_cf->data, cf->can_dlc) == 0;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(can_cmp_echo_skb);


Partly :-)

The question is when you receive any CAN frame, how do you know to what
echo_skb[index] this should be compared?

AFAIK every CAN driver manages it's echo_skb[] array itself. E.g. the sja1000
driver only uses index '0' for that reason that i handles only one tx process
at a time.

If you have more than one element in the echo_skb[] array, i would think of

for (i = 0; i < DRVNAME_ECHO_SKB_MAX; i++) {

> +	/* check if this is an ECHO frame */
> +	if (can_cmp_echo_skb(skb, ndev, i)) {
> +		stats->txbytes += can_get_echo_skb(ndev, i);
> +		kfree_skb(skb);
> +		goto some_exit;
> +	}

}

Regards,
Oliver

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Marc Kleine-Budde]

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

On 07/04/2012 06:23 PM, Oliver Hartkopp wrote:
> Anyway we currently have the flexcan and the ican3 hardware that generally
> supports a loopback on hardware level but looses the reference to the tx-skb
> and therefore to the sk pointer which is used to identify the originating
> socket for recv-own-msgs support.

For the record:
Since v3.4-rc1 flexcan implements proper loopback. (The flexcan driver
has been added before recv-own-msgs support has been implemented, so it
didn't make any difference back then.)

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: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp]

On 10.07.2012 12:20, Marc Kleine-Budde wrote:

> On 07/04/2012 06:23 PM, Oliver Hartkopp wrote:
>> Anyway we currently have the flexcan and the ican3 hardware that generally
>> supports a loopback on hardware level but looses the reference to the tx-skb
>> and therefore to the sk pointer which is used to identify the originating
>> socket for recv-own-msgs support.
> 
> For the record:
> Since v3.4-rc1 flexcan implements proper loopback. (The flexcan driver
> has been added before recv-own-msgs support has been implemented, so it
> didn't make any difference back then.)
> 


Yes. I did not want to state that the driver does not have a proper solution.
The point is that the flexcan driver has a hardware loopback (like the ican3)
but due to the lack of references in the CAN frame registers the (generally
nice) functionality cannot be used for us. sigh.

Regards,
Oliver

Re: Questions about janz-ican3 and CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

On Wed, Jul 04, 2012 at 11:18:38AM +0200, Wolfgang Grandegger wrote:
> Hi Ira,
> 
> On 07/04/2012 01:23 AM, Ira W. Snyder wrote:
> > Hello everyone,
> > 
> > I'm finally implementing SocketCAN support in our internal codebase, and
> > have run across some issues. We previously used a character driver provided
> > by the vendor.
> > 
> > I myself implemented the driver for the Janz ICAN3 card. It has been in
> > mainline Linux since 2.6.35.
> > 
> > The hardware is an SJA1000 chip plus a microcontroller and some RAM. All
> > messages go through the microcontroller firmware.
> > 
> > This firmware has the following features:
> > - it does have hardware-supported local loopback
> > - it does NOT have any sort of "tx-complete" notification or interrupt
> > - it does NOT have any indication that a frame went through the
> >   hardware-supported local loopback
> > 
> > To work around the lack of "tx-complete" interrupts, I used the hardware
> > local loopback feature. Every frame has hardware loopback set, which causes
> > the ican3_napi() routine to be called for each frame sent. This works fine,
> > except that sockets created with the default options (loopback on,
> > can_raw_recv_own_msgs off) do receive their own messages. This seems wrong.
> > 

This was the important sentence. As the driver is currently written,
sockets with can_raw_recv_own_msgs off DO receive their own messages.

This is not what most people expect, as you point out.

My coworker complained about this behavior, which is how I found out.
The candump program doesn't show anything abnormal, which is how I
missed the bug. The tst-rcv-own-msgs is the only test which shows the
bug.

We should really make sure that new drivers have run the test before
they are pushed upstream. I didn't know that the test existed until I
dug through mailing list archives looking for guidance on
can_raw_recv_own_msgs.

> > QUESTION 1:
> > The functions can_(get|put)_echo_skb() are not used at all. Is this wrong?
> 
> Not necessarily. IIRC, the early versions of the Flexcan driver used the
> hardware loopback as well but switched to classical echo-skb handling
> because of the problem with recv-own-msgs.
> 
> > QUESTION 2:
> > The socketcan test tst-rcv-own-msgs says:
> > Starting PF_CAN frame flow test.
> > checking socket default settings ... failure!
> > 
> > If I understand the code correctly, this means that the sending socket
> > received an echo frame when it should not have received one.
> > 
> > This matches what I expect the code to do.
> > 
> > How can I fix this?
> > 
> > OBSERVATION 1:
> > The following patch drops the relevant calls to netif_stop_queue() and
> > netif_wake_queue(), as well as the hardware-supported local loopback.
> > 
> > The tst-rcv-own-msgs test passes.
> > 
> > Is dropping these API calls a reasonable thing to do?
> 
> Well, that controller will not allow to support all features and options
> and we need to make a choice:
> 
>  recv-own-msgs support versus proper tx-done handling (correct timing)
> 
> As the recv-own-msgs is not used frequently, it would not be really
> dramatic if the feature is not available. Just my 0.02 cent.
> 
> Wolfgang.

It looks like when a driver returns NETDEV_TX_BUSY, the socket layer
just re-queues the packet and tries again. I've done some full bus load
testing with cangen/candump, and I can't notice any bad effects.

What is the problem with returning NETDEV_TX_BUSY when the hardware
sucks?

I may have thought of a workaround, though it might hurt performance
more than just returning NETDEV_TX_BUSY.

When the TX buffer is full, call netif_stop_queue(). Also start a timer
to check whether there is some free TX buffer space. If there is no free
TX buffer space, reschedule the timer. If there is some free TX buffer
space, call netif_wake_queue() and don't reschedule.

I'll code it up and try it out.

The other idea of using a can_peek_echo_skb() is an idea worth trying as
well.

Thanks for the input,
Ira