[PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

As reported by Manfred Schlaegl here

   http://marc.info/?l=linux-netdev&m=143482089824232&w=2

commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
overlapping CAN filters" requires the skb->tstamp to be set to check for
identical CAN skbs.

As net timestamping is influenced by several players (netstamp_needed and
netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
CAN frame loss.

As skb timestamping became now mandatory for CAN related skbs this patch
makes sure that received CAN skbs always have a proper timestamp set.
Maybe there's a better solution in the future but this patch fixes the
CAN frame loss so far.

Reported-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c   | 5 +++++
 drivers/net/can/slcan.c | 1 +
 drivers/net/can/vcan.c  | 3 +++
 net/can/af_can.c        | 6 +++++-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b0f6924..e9b1810 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -440,6 +440,9 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 		struct can_frame *cf = (struct can_frame *)skb->data;
 		u8 dlc = cf->can_dlc;
 
+		if (!(skb->tstamp.tv64))
+			__net_timestamp(skb);
+
 		netif_rx(priv->echo_skb[idx]);
 		priv->echo_skb[idx] = NULL;
 
@@ -575,6 +578,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 	if (unlikely(!skb))
 		return NULL;
 
+	__net_timestamp(skb);
 	skb->protocol = htons(ETH_P_CAN);
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -603,6 +607,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 	if (unlikely(!skb))
 		return NULL;
 
+	__net_timestamp(skb);
 	skb->protocol = htons(ETH_P_CANFD);
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index c837eb9..f64f529 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -207,6 +207,7 @@ static void slc_bump(struct slcan *sl)
 	if (!skb)
 		return;
 
+	__net_timestamp(skb);
 	skb->dev = sl->dev;
 	skb->protocol = htons(ETH_P_CAN);
 	skb->pkt_type = PACKET_BROADCAST;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 674f367..0ce868d 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -78,6 +78,9 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
+	if (!(skb->tstamp.tv64))
+		__net_timestamp(skb);
+
 	netif_rx_ni(skb);
 }
 
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 32d710e..689c818 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -310,8 +310,12 @@ int can_send(struct sk_buff *skb, int loop)
 		return err;
 	}
 
-	if (newskb)
+	if (newskb) {
+		if (!(newskb->tstamp.tv64))
+			__net_timestamp(newskb);
+
 		netif_rx_ni(newskb);
+	}
 
 	/* update statistics */
 	can_stats.tx_frames++;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

As reported by Manfred Schlaegl here

   http://marc.info/?l=linux-netdev&m=143482089824232&w=2

commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
overlapping CAN filters" requires the skb->tstamp to be set to check for
identical CAN skbs.

As net timestamping is influenced by several players (netstamp_needed and
netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
CAN frame loss.

As skb timestamping became now mandatory for CAN related skbs this patch
makes sure that received CAN skbs always have a proper timestamp set.
Maybe there's a better solution in the future but this patch fixes the
CAN frame loss so far.

Reported-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c   | 5 +++++
 drivers/net/can/slcan.c | 1 +
 drivers/net/can/vcan.c  | 3 +++
 net/can/af_can.c        | 6 +++++-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b0f6924..e9b1810 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -440,6 +440,9 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 		struct can_frame *cf = (struct can_frame *)skb->data;
 		u8 dlc = cf->can_dlc;
 
+		if (!(skb->tstamp.tv64))
+			__net_timestamp(skb);
+
 		netif_rx(priv->echo_skb[idx]);
 		priv->echo_skb[idx] = NULL;
 
@@ -575,6 +578,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 	if (unlikely(!skb))
 		return NULL;
 
+	__net_timestamp(skb);
 	skb->protocol = htons(ETH_P_CAN);
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -603,6 +607,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 	if (unlikely(!skb))
 		return NULL;
 
+	__net_timestamp(skb);
 	skb->protocol = htons(ETH_P_CANFD);
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
index c837eb9..f64f529 100644
--- a/drivers/net/can/slcan.c
+++ b/drivers/net/can/slcan.c
@@ -207,6 +207,7 @@ static void slc_bump(struct slcan *sl)
 	if (!skb)
 		return;
 
+	__net_timestamp(skb);
 	skb->dev = sl->dev;
 	skb->protocol = htons(ETH_P_CAN);
 	skb->pkt_type = PACKET_BROADCAST;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 674f367..0ce868d 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -78,6 +78,9 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
+	if (!(skb->tstamp.tv64))
+		__net_timestamp(skb);
+
 	netif_rx_ni(skb);
 }
 
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 32d710e..689c818 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -310,8 +310,12 @@ int can_send(struct sk_buff *skb, int loop)
 		return err;
 	}
 
-	if (newskb)
+	if (newskb) {
+		if (!(newskb->tstamp.tv64))
+			__net_timestamp(newskb);
+
 		netif_rx_ni(newskb);
+	}
 
 	/* update statistics */
 	can_stats.tx_frames++;
-- 
2.1.4

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Marc Kleine-Budde]

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

On 06/21/2015 06:50 PM, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
> 
>    http://marc.info/?l=linux-netdev&m=143482089824232&w=2
> 
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
> 
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
> 
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
> 
> Reported-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

I'll send a pullrequest to David asap, I'll add stable on Cc if this
doesn't make it into v4.1 anymore.

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: 455 bytes --]

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

On 21.06.2015 18:57, Marc Kleine-Budde wrote:

>
> I'll send a pullrequest to David asap, I'll add stable on Cc if this
> doesn't make it into v4.1 anymore.

Thanks Marc!

I sent it to netdev and Dave directly too as he was working on his backlog - 
and I was not sure if you are available until the next pull request for 4.1.

Best regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

On 21.06.2015 18:57, Marc Kleine-Budde wrote:

>
> I'll send a pullrequest to David asap, I'll add stable on Cc if this
> doesn't make it into v4.1 anymore.

Thanks Marc!

I sent it to netdev and Dave directly too as he was working on his backlog - 
and I was not sure if you are available until the next pull request for 4.1.

Best regards,
Oliver

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello!

On 2015-06-21 18:50, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
> 
>    http://marc.info/?l=linux-netdev&m=143482089824232&w=2
> 
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
> 
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
> 
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
> 

I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.

If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.

Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.

What do you thing about this?

best regards,
Manfred
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello!

On 2015-06-21 18:50, Oliver Hartkopp wrote:
> As reported by Manfred Schlaegl here
> 
>    http://marc.info/?l=linux-netdev&m=143482089824232&w=2
> 
> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
> overlapping CAN filters" requires the skb->tstamp to be set to check for
> identical CAN skbs.
> 
> As net timestamping is influenced by several players (netstamp_needed and
> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
> CAN frame loss.
> 
> As skb timestamping became now mandatory for CAN related skbs this patch
> makes sure that received CAN skbs always have a proper timestamp set.
> Maybe there's a better solution in the future but this patch fixes the
> CAN frame loss so far.
> 

I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.

If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.

Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.

What do you thing about this?

best regards,
Manfred

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

Hello Manfred,

On 22.06.2015 12:10, Manfred Schlaegl wrote:
> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>> As reported by Manfred Schlaegl here
>>
>>     http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>
>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>> identical CAN skbs.
>>
>> As net timestamping is influenced by several players (netstamp_needed and
>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>> CAN frame loss.
>>
>> As skb timestamping became now mandatory for CAN related skbs this patch
>> makes sure that received CAN skbs always have a proper timestamp set.
>> Maybe there's a better solution in the future but this patch fixes the
>> CAN frame loss so far.
>>
>
> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>
> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.

The only change is that the timestamps for CAN skbs are generated always.
The idea behind the timestamping by user control is to omit the timestamping 
when it's not needed. There's no user visible change in behaviour when the 
timestamp is set in the CAN skbs all time.

> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.

The change only affects CAN skbs.
These skbs are allocated at CAN frame reception time, filled with content and 
then sent to the network layer.

AFAICS the timestamp becomes more precise for CAN related skbs.
I did not see any case of 'early allocation' in linux/drivers/net/can, did you?

Best regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

Hello Manfred,

On 22.06.2015 12:10, Manfred Schlaegl wrote:
> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>> As reported by Manfred Schlaegl here
>>
>>     http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>
>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>> identical CAN skbs.
>>
>> As net timestamping is influenced by several players (netstamp_needed and
>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>> CAN frame loss.
>>
>> As skb timestamping became now mandatory for CAN related skbs this patch
>> makes sure that received CAN skbs always have a proper timestamp set.
>> Maybe there's a better solution in the future but this patch fixes the
>> CAN frame loss so far.
>>
>
> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>
> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.

The only change is that the timestamps for CAN skbs are generated always.
The idea behind the timestamping by user control is to omit the timestamping 
when it's not needed. There's no user visible change in behaviour when the 
timestamp is set in the CAN skbs all time.

> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.

The change only affects CAN skbs.
These skbs are allocated at CAN frame reception time, filled with content and 
then sent to the network layer.

AFAICS the timestamp becomes more precise for CAN related skbs.
I did not see any case of 'early allocation' in linux/drivers/net/can, did you?

Best regards,
Oliver

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello Oliver,

On 2015-06-22 12:34, Oliver Hartkopp wrote:
> Hello Manfred,
> 
> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>>> As reported by Manfred Schlaegl here
>>>
>>>     http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>>
>>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>>> identical CAN skbs.
>>>
>>> As net timestamping is influenced by several players (netstamp_needed and
>>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>>> CAN frame loss.
>>>
>>> As skb timestamping became now mandatory for CAN related skbs this patch
>>> makes sure that received CAN skbs always have a proper timestamp set.
>>> Maybe there's a better solution in the future but this patch fixes the
>>> CAN frame loss so far.
>>>
>>
>> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>>
>> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.
> 
> The only change is that the timestamps for CAN skbs are generated always.
> The idea behind the timestamping by user control is to omit the timestamping when it's not needed. There's no user visible change in behaviour when the timestamp is set in the CAN skbs all time.
> 
>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
> 
> The change only affects CAN skbs.
> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
> 
> AFAICS the timestamp becomes more precise for CAN related skbs.
> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?

No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.

But I trust your expertise, so if you are fine with it, I'm too. ;-)

Best regards,
Manfred
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Manfred Schlaegl]

Hello Oliver,

On 2015-06-22 12:34, Oliver Hartkopp wrote:
> Hello Manfred,
> 
> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>> On 2015-06-21 18:50, Oliver Hartkopp wrote:
>>> As reported by Manfred Schlaegl here
>>>
>>>     http://marc.info/?l=linux-netdev&m=143482089824232&w=2
>>>
>>> commit 514ac99c64b "can: fix multiple delivery of a single CAN frame for
>>> overlapping CAN filters" requires the skb->tstamp to be set to check for
>>> identical CAN skbs.
>>>
>>> As net timestamping is influenced by several players (netstamp_needed and
>>> netdev_tstamp_prequeue) Manfred missed a proper timestamp which leads to
>>> CAN frame loss.
>>>
>>> As skb timestamping became now mandatory for CAN related skbs this patch
>>> makes sure that received CAN skbs always have a proper timestamp set.
>>> Maybe there's a better solution in the future but this patch fixes the
>>> CAN frame loss so far.
>>>
>>
>> I'm not sure, but maybe this patch (and also my original one) opens a new potential issue with timestamps.
>>
>> If the timestamp is set at allocation time, this cancels setting the timestamp at delivery (by net_timestamp_check in, for example, netif_receive_skb_internal.) -> So it changes the behavior of timestamping (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/timestamping.txt?id=b953c0d234bc72e8489d3bf51a276c5c4ec85345) generally.
> 
> The only change is that the timestamps for CAN skbs are generated always.
> The idea behind the timestamping by user control is to omit the timestamping when it's not needed. There's no user visible change in behaviour when the timestamp is set in the CAN skbs all time.
> 
>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
> 
> The change only affects CAN skbs.
> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
> 
> AFAICS the timestamp becomes more precise for CAN related skbs.
> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?

No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.

But I trust your expertise, so if you are fine with it, I'm too. ;-)

Best regards,
Manfred

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

On 22.06.2015 13:49, Manfred Schlaegl wrote:
> On 2015-06-22 12:34, Oliver Hartkopp wrote:
>> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
>>
>> The change only affects CAN skbs.
>> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
>>
>> AFAICS the timestamp becomes more precise for CAN related skbs.
>> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?
>
> No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
> I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.
>
> But I trust your expertise, so if you are fine with it, I'm too. ;-)

I don't claim to be 'an expert' :-)

But our usual use-case is CAN logging which enables the timestamping on all 
CAN interfaces anyway - without any problems.

As the timestamp is calculated only once this patch moves the timestamp 
creation closer to the CAN frame arrival time - so latency finally decreases.

Best regards,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH - regression 4.1-rc8] can: fix loss of CAN frames in raw_rcv

[Oliver Hartkopp]

On 22.06.2015 13:49, Manfred Schlaegl wrote:
> On 2015-06-22 12:34, Oliver Hartkopp wrote:
>> On 22.06.2015 12:10, Manfred Schlaegl wrote:
>>> Hypothetical example: If timestamping is enabled by the user and there is a significant delay between allocation and delivery of a skb (early allocation in driver or something) the timestamp does not reflect the reception time anymore.
>>
>> The change only affects CAN skbs.
>> These skbs are allocated at CAN frame reception time, filled with content and then sent to the network layer.
>>
>> AFAICS the timestamp becomes more precise for CAN related skbs.
>> I did not see any case of 'early allocation' in linux/drivers/net/can, did you?
>
> No, I also did not find this case in current driver implementations -- because of that I gave the hypothetical example.
> I just was worried about that this may be a potential latent issue for future driver implementations and wanted to indicate this.
>
> But I trust your expertise, so if you are fine with it, I'm too. ;-)

I don't claim to be 'an expert' :-)

But our usual use-case is CAN logging which enables the timestamping on all 
CAN interfaces anyway - without any problems.

As the timestamp is calculated only once this patch moves the timestamp 
creation closer to the CAN frame arrival time - so latency finally decreases.

Best regards,
Oliver