can: c_can: TX handling

can: c_can: TX handling

[Jan Altenberg]

Hi all,

I did some more testing on the SocketCAN driver for the Bosch c_can
controller and I observed some strange behaviour for the TX handling.
First of all the TX bytes are not accounted correctly. The reason for that
seems to be quite obvious if we look into c_can_do_tx():

[...]

c_can_inval_msg_object(dev, 0, msg_obj_no);
val = c_can_read_reg32(priv, &priv->regs->txrqst1);
if (!(val & (1 << msg_obj_no))) {
        can_get_echo_skb(dev,
                        msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
        stats->tx_bytes += priv->read_reg(priv,
                        &priv->regs->ifregs[0].msg_cntrl)
                        & IF_MCONT_DLC_MASK;

So, we first invalidate the message object and afterwards we read the DLC
value from the msg_cntrl (which is 0 after invalidating the
message object) to account the TX bytes. So tx_bytes will always be 0. The
fix should be easy, I think, we can just move
c_can_inval_msg_object to the end of that loop.

The second problem is related to tx_next, which should hold the number of
the oldest CAN frame, which was not on the line:

for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
        msg_obj_no = get_tx_echo_msg_obj(priv);
        c_can_inval_msg_object(dev, 0, msg_obj_no);
        val = c_can_read_reg32(priv, &priv->regs->txrqst1);
        if (!(val & (1 << msg_obj_no))) {
                can_get_echo_skb(dev,
                                msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
                stats->tx_bytes += priv->read_reg(priv,
                                &priv->regs->ifregs[0].msg_cntrl)
                                & IF_MCONT_DLC_MASK;
                stats->tx_packets++;
        }
}

But tx_echo is incremented unconditionally and we don't actually track the
number of the oldest unsent frame.
Let's assume the following scenario: We bring up can0 and send 3
frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is
still pending. If we go through the above loop in that situation, we will
skip message object 0, because the txrqst bit is still set.  We will
account message object 1 and 2. That's correct, but afterwards tx_echo is
set to 2, BUT the oldest message which is pending is 0. Am I right or did
I get something wrong?
The operation of c_can_do_tx() is described as follows: "We iterate from
priv->tx_echo to priv->tx_next and check if the packet has been
transmitted, echo it back to the CAN framework. If we discover a not yet
transmitted package, stop looking for more." The actual
implementation doesn't seem to stop if we discover a not yet
transmitted package. But I'm not sure if just stopping might be a
good idea, because in that case, the echo skb for already transmitted
messages might be delayed by not yet transmitted messages.

Cheers,
        Jan

Re: can: c_can: TX echo

[Kurt Van Dijck]

I'm not too familiar with the c_can chip.
So I have a few questions inside my reply.

On Tue, Mar 22, 2011 at 04:59:23PM +0100, Jan Altenberg wrote:
> Hi all,
> 
> I did some more testing on the SocketCAN driver for the Bosch c_can
> controller and I observed some strange behaviour for the TX handling.
> First of all the TX bytes are not accounted correctly. The reason for that
> seems to be quite obvious if we look into c_can_do_tx():
> 
> [...]
> 
> c_can_inval_msg_object(dev, 0, msg_obj_no);
> val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> if (!(val & (1 << msg_obj_no))) {
>         can_get_echo_skb(dev,
>                         msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>         stats->tx_bytes += priv->read_reg(priv,
>                         &priv->regs->ifregs[0].msg_cntrl)
>                         & IF_MCONT_DLC_MASK;
> 
> So, we first invalidate the message object and afterwards we read the DLC
> value from the msg_cntrl (which is 0 after invalidating the
> message object) to account the TX bytes. So tx_bytes will always be 0. The
> fix should be easy, I think, we can just move
> c_can_inval_msg_object to the end of that loop.

IMO, it looks necessary to call c_can_inval_msg_object inside the if (),
after it has been transmitted. Otherwise, if for some other (TX) reason you
get in this loop, you may clear a pending transmission?
Again, I haven't read this one's datasheet. I was familiar with its predecessor.
> 
> Cheers,
>         Jan

Regards,
Kurt

Re: can: c_can: TX echo

[Jan Altenberg]

Hi,

>> So, we first invalidate the message object and afterwards we read
>> the DLC value from the msg_cntrl (which is 0 after invalidating the
>> message object) to account the TX bytes. So tx_bytes will always be
>> 0.  The fix should be easy, I think, we can just move
>> c_can_inval_msg_object to the end of that loop.
>
> IMO, it looks necessary to call c_can_inval_msg_object inside the if
> (), after it has been transmitted. Otherwise, if for some other (TX)
> reason you get in this loop, you may clear a pending transmission?
> Again, I haven't read this one's datasheet. I was familiar with its
> predecessor.

I tried to check, but the datasheet is a bit unclear regarding that
point ;-) My interpretation is, that c_can_inval_msg_object()
shouldn't affect the txrqst bit, but nevertheless calling it inside
the if() statement would make the code more readable.

I can prepare a patch, but I'd like to wait for Wolfgang's / Bupesh's
feedback.

Cheers,
        Jan

Re: can: c_can: TX echo

[Wolfgang Grandegger]

Hi Jan,

On 03/23/2011 02:54 PM, Jan Altenberg wrote:
> Hi,
> 
>>> So, we first invalidate the message object and afterwards we read
>>> the DLC value from the msg_cntrl (which is 0 after invalidating the
>>> message object) to account the TX bytes. So tx_bytes will always be
>>> 0.  The fix should be easy, I think, we can just move
>>> c_can_inval_msg_object to the end of that loop.

This means that "ifconfig" will report *0* TX Bytes!?

>> IMO, it looks necessary to call c_can_inval_msg_object inside the if
>> (), after it has been transmitted. Otherwise, if for some other (TX)
>> reason you get in this loop, you may clear a pending transmission?
>> Again, I haven't read this one's datasheet. I was familiar with its
>> predecessor.
> 
> I tried to check, but the datasheet is a bit unclear regarding that
> point ;-) My interpretation is, that c_can_inval_msg_object()
> shouldn't affect the txrqst bit, but nevertheless calling it inside
> the if() statement would make the code more readable.
> 
> I can prepare a patch, but I'd like to wait for Wolfgang's / Bupesh's
> feedback.

I'm following the discussion and as I'm not familiar with that chip, I'm
waiting for Bupesh's answer as well... before I start digging. You can
also have a look to the pch_can driver, which is C_CAN based as well.

Wolfgang.

Re: can: c_can: TX echo

[Jan Altenberg]

Hi Wolfgang,

> This means that "ifconfig" will report *0* TX Bytes!?

Yep, ifconfig reports *0* TX Bytes.

> I'm following the discussion and as I'm not familiar with that chip,
> I'm waiting for Bupesh's answer as well... before I start digging.
> You can also have a look to the pch_can driver, which is C_CAN based
> as well.

I'm also quite new to that chip, so lets wait for Bupesh's feedback.

Cheers,
   Jan

Re: can: c_can: TX delivery

[Kurt Van Dijck]

Jan,

I split your 2 questions in 2 replies.

On Tue, Mar 22, 2011 at 04:59:23PM +0100, Jan Altenberg wrote:
> Hi all,
> 

> The second problem is related to tx_next, which should hold the number of
> the oldest CAN frame, which was not on the line:
> 
> for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
>         msg_obj_no = get_tx_echo_msg_obj(priv);
>         c_can_inval_msg_object(dev, 0, msg_obj_no);
>         val = c_can_read_reg32(priv, &priv->regs->txrqst1);
>         if (!(val & (1 << msg_obj_no))) {
>                 can_get_echo_skb(dev,
>                                 msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>                 stats->tx_bytes += priv->read_reg(priv,
>                                 &priv->regs->ifregs[0].msg_cntrl)
>                                 & IF_MCONT_DLC_MASK;
>                 stats->tx_packets++;
>         }
> }
> 
> But tx_echo is incremented unconditionally and we don't actually track the
> number of the oldest unsent frame.
> Let's assume the following scenario: We bring up can0 and send 3
> frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is
> still pending. If we go through the above loop in that situation, we will
> skip message object 0, because the txrqst bit is still set.  We will
> account message object 1 and 2. That's correct, but afterwards tx_echo is
> set to 2, BUT the oldest message which is pending is 0. Am I right or did
> I get something wrong?
> The operation of c_can_do_tx() is described as follows: "We iterate from
> priv->tx_echo to priv->tx_next and check if the packet has been
> transmitted, echo it back to the CAN framework. If we discover a not yet
> transmitted package, stop looking for more." The actual
> implementation doesn't seem to stop if we discover a not yet
> transmitted package. But I'm not sure if just stopping might be a
> good idea, because in that case, the echo skb for already transmitted
> messages might be delayed by not yet transmitted messages.

It is better to deliver the echo's in the order they were delivered
on CAN.
For that, we can replace the tx_next/tx_echo pair with a single tx_bitmask.
In the tx_bitmask, we could set a bit when the TX is queued, and test for
those bits in the tx interrupt.
When tx is done, clear the bit in tx_bitmask & stuff.

16 tx objects fit inside a single int.

c_can_start_xmit would look a bit different. I think of something like this:

	/* find free object */
	for (j = 0; j < TX_MAX; ++j) {
		if (!(priv->tx_bitmask & (1 << j)))
			break;
	}
	if (j >= TX_MAX)
		/* no free objects */
		return NETDEV_TX_BUSY;
		
	c_can_write_...(j, ...)
	can_put_echo_skb(, j, ...)
	priv->tx_bitmask |= (1 << j);
	return NETDEV_TX_OK;

not sure if I made my point.
Note that this will eliminate the need for explicit wrap-around. It's
done implicitely.
> 
> Cheers,
>         Jan
> 
Regards,
Kurt

Re: can: c_can: TX delivery

[Jan Altenberg]

Hi,

> I split your 2 questions in 2 replies.

Thanks :)

> not sure if I made my point.  Note that this will eliminate the need
> for explicit wrap-around.  It's done implicitely.

Hmmm, I double-checked the datasheet, which gives the following statement:
"The receive/transmit priority for the Message Objects is attached to
the message number.  Message Object 1 has the highest priority, while
Message Object 32 has the lowest priority. If more than one
transmission request is pending, they are serviced due to the priority
of the corresponding Message Object."

So, we shouldn't run into the scenario I described in my previous mail
and the existing implementation should be OK, right?!  I'm quite sure
I've seen a situation where msg_obj 17 "seemed" to be pending, while
msg_obj 18 and 19 already have been transmitted. But in that case, I
enabled ONESHOT for the can interface, which enables the DA mode
(automatic
retransmission is disabled). The errata sheet for c_can covers that
mode. There's a problem with "Concurrent transmission requests" and I'm
quite sure my test-case hit that problem.

I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback
(or beat me for causing some confusion ;-)).

Sorry for the confusion!
        Jan

Re: can: c_can: TX delivery

[Kurt Van Dijck]

On Wed, Mar 23, 2011 at 04:32:31PM +0100, Jan Altenberg wrote:
> Hi,
> 
> > I split your 2 questions in 2 replies.
> 
> Thanks :)
> 
> > not sure if I made my point.  Note that this will eliminate the need
> > for explicit wrap-around.  It's done implicitely.
> 
> Hmmm, I double-checked the datasheet, which gives the following statement:
> "The receive/transmit priority for the Message Objects is attached to
> the message number.  Message Object 1 has the highest priority, while
> Message Object 32 has the lowest priority. If more than one
> transmission request is pending, they are serviced due to the priority
> of the corresponding Message Object."

With its predecessor (I used it as IP inside infineon CPU's), this was also
true. But I _think_ that object 17 could send before object 16 if its identifier
is lower.
So, me too wait for Bhupesh ...

> I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback
> (or beat me for causing some confusion ;-)).
> 
> Sorry for the confusion!
>         Jan
> 
> 

Re: can: c_can: TX delivery

[Marc Kleine-Budde]

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

On 03/23/2011 04:32 PM, Jan Altenberg wrote:
> Hi,
> 
>> I split your 2 questions in 2 replies.
> 
> Thanks :)
> 
>> not sure if I made my point.  Note that this will eliminate the need
>> for explicit wrap-around.  It's done implicitely.
> 
> Hmmm, I double-checked the datasheet, which gives the following statement:
> "The receive/transmit priority for the Message Objects is attached to
> the message number.  Message Object 1 has the highest priority, while
> Message Object 32 has the lowest priority. If more than one
> transmission request is pending, they are serviced due to the priority
> of the corresponding Message Object."
> 
> So, we shouldn't run into the scenario I described in my previous mail

ACK - the tx implementation is borrowed from the at91 driver. There we
have a prio field per mailbox (which is _not_ the CAN id), and the
mailbox number itself. If two mailboxes have the same prio the mailbox
with the lower number is send first. So we start with the highest prio
in mailbox 0, until the last mb, then decrease the prio and start with
mb 0. Special care is taken in prio wrap around and all mailboxes full.

Please recheck if this is implemented on the c_can driver correctly.

> and the existing implementation should be OK, right?!  I'm quite sure
> I've seen a situation where msg_obj 17 "seemed" to be pending, while
> msg_obj 18 and 19 already have been transmitted. But in that case, I
> enabled ONESHOT for the can interface, which enables the DA mode
> (automatic
> retransmission is disabled). The errata sheet for c_can covers that

Oh...Oneshot means if TX fails (due to higher prio frame on the bus) it
won't be tx'ed again. IIRC in the at91 there's a bit signalling that TX
has been aborted due to oneshot. Maybe an interrupt can be enabled, too.

Hmm....the driver advertised it supports oneshot mode. Has this been
tested? If oneshot isn't working we should disable it in the driver
until it has been fixed.

> mode. There's a problem with "Concurrent transmission requests" and I'm
> quite sure my test-case hit that problem.
> 
> I'm quite new to Bosch's c_can, so maybe Bhupesh can give some feedback
> (or beat me for causing some confusion ;-)).

cheers, 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: c_can: TX handling

[Bhupesh SHARMA]

Hi Jan,

Thanks for your work on c_can.
This week has been very busy for me, so please excuse me for the late reply.

> Hi all,
> 
> I did some more testing on the SocketCAN driver for the Bosch c_can
> controller and I observed some strange behaviour for the TX handling.
> First of all the TX bytes are not accounted correctly. The reason for
> that
> seems to be quite obvious if we look into c_can_do_tx():
> 
> [...]
> 
> c_can_inval_msg_object(dev, 0, msg_obj_no);
> val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> if (!(val & (1 << msg_obj_no))) {
>         can_get_echo_skb(dev,
>                         msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>         stats->tx_bytes += priv->read_reg(priv,
>                         &priv->regs->ifregs[0].msg_cntrl)
>                         & IF_MCONT_DLC_MASK;
> 
> So, we first invalidate the message object and afterwards we read the
> DLC
> value from the msg_cntrl (which is 0 after invalidating the
> message object) to account the TX bytes. So tx_bytes will always be 0.
> The
> fix should be easy, I think, we can just move
> c_can_inval_msg_object to the end of that loop.

Hmmm.. 
As far as I remember, *ifconfig* used to provide the correct tx_bytes stats
on my SPEAr board. Let me recheck this again today evening and get back to you.

> The second problem is related to tx_next, which should hold the number
> of
> the oldest CAN frame, which was not on the line:
> 
> for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
>         msg_obj_no = get_tx_echo_msg_obj(priv);
>         c_can_inval_msg_object(dev, 0, msg_obj_no);
>         val = c_can_read_reg32(priv, &priv->regs->txrqst1);
>         if (!(val & (1 << msg_obj_no))) {
>                 can_get_echo_skb(dev,
>                                 msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>                 stats->tx_bytes += priv->read_reg(priv,
>                                 &priv->regs->ifregs[0].msg_cntrl)
>                                 & IF_MCONT_DLC_MASK;
>                 stats->tx_packets++;
>         }
> }
> 
> But tx_echo is incremented unconditionally and we don't actually track
> the
> number of the oldest unsent frame.
> Let's assume the following scenario: We bring up can0 and send 3
> frames: TX object: 0, 1, 2; 1 and 2 make it on the line, but 0 is
> still pending. If we go through the above loop in that situation, we
> will
> skip message object 0, because the txrqst bit is still set.  We will
> account message object 1 and 2. That's correct, but afterwards tx_echo
> is
> set to 2, BUT the oldest message which is pending is 0. Am I right or
> did
> I get something wrong?
> The operation of c_can_do_tx() is described as follows: "We iterate
> from
> priv->tx_echo to priv->tx_next and check if the packet has been
> transmitted, echo it back to the CAN framework. If we discover a not
> yet
> transmitted package, stop looking for more." The actual
> implementation doesn't seem to stop if we discover a not yet
> transmitted package. But I'm not sure if just stopping might be a
> good idea, because in that case, the echo skb for already transmitted
> messages might be delayed by not yet transmitted messages.
> 

Just to better understand this can you send me your *candump* output
when you see that the message object put earlier in the message RAM
being pending on the line.

Your point seems valid but I want to make sure that this is not done by the 
C_CAN core implicitly..  

Regards,
Bhupesh

RE: c_can: TX handling

[Jan Altenberg]

Hi,

> Thanks for your work on c_can.
> This week has been very busy for me, so please excuse me for the late reply.

No problem.

> Just to better understand this can you send me your *candump* output
> when you see that the message object put earlier in the message RAM
> being pending on the line.
> 
> Your point seems valid but I want to make sure that this is not done by the 
> C_CAN core implicitly..  

I'll try to prepare a (stripped down) test-case, but since I'm quite
busy at the moment, that might take a couple of days.
I'm quite sure, I've seen such a situation, but first of all, I'd like
to sort out, if that was related to the "DA mode".

Cheers,
	Jan

RE: c_can: TX handling

[Bhupesh SHARMA]

Hi Jan,

> > I did some more testing on the SocketCAN driver for the Bosch c_can
> > controller and I observed some strange behaviour for the TX handling.
> > First of all the TX bytes are not accounted correctly. The reason for
> > that
> > seems to be quite obvious if we look into c_can_do_tx():
> >
> > [...]
> >
> > c_can_inval_msg_object(dev, 0, msg_obj_no);
> > val = c_can_read_reg32(priv, &priv->regs->txrqst1);
> > if (!(val & (1 << msg_obj_no))) {
> >         can_get_echo_skb(dev,
> >                         msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> >         stats->tx_bytes += priv->read_reg(priv,
> >                         &priv->regs->ifregs[0].msg_cntrl)
> >                         & IF_MCONT_DLC_MASK;
> >
> > So, we first invalidate the message object and afterwards we read the
> > DLC
> > value from the msg_cntrl (which is 0 after invalidating the
> > message object) to account the TX bytes. So tx_bytes will always be
> 0.
> > The
> > fix should be easy, I think, we can just move
> > c_can_inval_msg_object to the end of that loop.
> 
> Hmmm..
> As far as I remember, *ifconfig* used to provide the correct tx_bytes
> stats
> on my SPEAr board. Let me recheck this again today evening and get back
> to you.

I managed to get hold of one SPEAr board and did some quick tests.
Indeed the tx_bytes field is 0 when we execute *ifconfig*

I think the easy fix would be to add c_can_inval_msg_object(dev, 0, msg_obj_no);
call inside the *if* construct. Could you please prepare a patch after
testing the same at your end.

Regards,
Bhupesh

[PATCH] can: c_can: Fix tx_bytes accounting

[Jan Altenberg]

Hi Bhupesh,

as discussed I moved c_can_inval_msg_object() to the end of the if()
statement. That should fix the tx_bytes accounting. For me it's working
fine now.

Signed-off-by: Jan Altenberg <jan@linutronix.de>
---
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 110eda0..f895c04 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -704,7 +704,6 @@ static void c_can_do_tx(struct net_device *dev)
 
 	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
 		msg_obj_no = get_tx_echo_msg_obj(priv);
-		c_can_inval_msg_object(dev, 0, msg_obj_no);
 		val = c_can_read_reg32(priv, &priv->regs->txrqst1);
 		if (!(val & (1 << msg_obj_no))) {
 			can_get_echo_skb(dev,
@@ -713,6 +712,7 @@ static void c_can_do_tx(struct net_device *dev)
 					&priv->regs->ifregs[0].msg_cntrl)
 					& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
+			c_can_inval_msg_object(dev, 0, msg_obj_no);
 		}
 	}
 

Re: [PATCH] can: c_can: Fix tx_bytes accounting

[Wolfgang Grandegger]

Hi Jan,

On 03/24/2011 11:38 AM, Jan Altenberg wrote:
> Hi Bhupesh,
> 
> as discussed I moved c_can_inval_msg_object() to the end of the if()
> statement. That should fix the tx_bytes accounting. For me it's working
> fine now.
> 
> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---

Could you please provide a proper commit message and put other comments
below "---".

Thanks,

Wolfgang.

[PATCH resend] can: c_can: Fix tx_bytes accounting

[Jan Altenberg]

The current SocketCAN implementation for the Bosch c_can cell doesn't
account the TX bytes correctly, because it calls
c_can_inval_msg_object() (which clears the msg ctrl register) before
reading the DLC value:

for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
        msg_obj_no = get_tx_echo_msg_obj(priv);
        c_can_inval_msg_object(dev, 0, msg_obj_no);
        val = c_can_read_reg32(priv, &priv->regs->txrqst1);
        if (!(val & (1 << msg_obj_no))) {
                can_get_echo_skb(dev,
                                msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
                stats->tx_bytes += priv->read_reg(priv,
                                &priv->regs->ifregs[0].msg_cntrl)
                                & IF_MCONT_DLC_MASK;
                stats->tx_packets++;
        }
}

So, we will always read 0 for the DLC value and "ifconfig" will report
*0* TX Bytes.

The fix is quite easy: Just move c_can_inval_msg_object() to the end of
the if() statement. So:
      * We only call c_can_inval_msg_object() if the message was
        actually transmitted
      * We read out the DLC value _before_ clearing the msg ctrl
        register

Signed-off-by: Jan Altenberg <jan@linutronix.de>
---
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 110eda0..f895c04 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -704,7 +704,6 @@ static void c_can_do_tx(struct net_device *dev)
 
 	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
 		msg_obj_no = get_tx_echo_msg_obj(priv);
-		c_can_inval_msg_object(dev, 0, msg_obj_no);
 		val = c_can_read_reg32(priv, &priv->regs->txrqst1);
 		if (!(val & (1 << msg_obj_no))) {
 			can_get_echo_skb(dev,
@@ -713,6 +712,7 @@ static void c_can_do_tx(struct net_device *dev)
 					&priv->regs->ifregs[0].msg_cntrl)
 					& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
+			c_can_inval_msg_object(dev, 0, msg_obj_no);
 		}
 	}
 

Re: [PATCH resend] can: c_can: Fix tx_bytes accounting

[Kurt Van Dijck]

On Thu, Mar 24, 2011 at 12:26:50PM +0100, Jan Altenberg wrote:
> The current SocketCAN implementation for the Bosch c_can cell doesn't
> account the TX bytes correctly, because it calls
> c_can_inval_msg_object() (which clears the msg ctrl register) before
> reading the DLC value:
> 
> The fix is quite easy: Just move c_can_inval_msg_object() to the end of
> the if() statement. So:
>       * We only call c_can_inval_msg_object() if the message was
>         actually transmitted
>       * We read out the DLC value _before_ clearing the msg ctrl
>         register
> 
> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Acked-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>

I cut the description a bit, to what I would have found sufficient.

Re: [PATCH resend] can: c_can: Fix tx_bytes accounting

[Wolfgang Grandegger]

On 03/24/2011 11:49 AM, Kurt Van Dijck wrote:
> On Thu, Mar 24, 2011 at 12:26:50PM +0100, Jan Altenberg wrote:
>> The current SocketCAN implementation for the Bosch c_can cell doesn't
>> account the TX bytes correctly, because it calls
>> c_can_inval_msg_object() (which clears the msg ctrl register) before
>> reading the DLC value:
>>
>> The fix is quite easy: Just move c_can_inval_msg_object() to the end of
>> the if() statement. So:
>>       * We only call c_can_inval_msg_object() if the message was
>>         actually transmitted
>>       * We read out the DLC value _before_ clearing the msg ctrl
>>         register
>>
>> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Acked-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

Re: [PATCH resend] can: c_can: Fix tx_bytes accounting

[David Miller]

From: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
Date: Thu, 24 Mar 2011 12:49:03 +0100

> On 03/24/2011 11:49 AM, Kurt Van Dijck wrote:
>> On Thu, Mar 24, 2011 at 12:26:50PM +0100, Jan Altenberg wrote:
>>> The current SocketCAN implementation for the Bosch c_can cell doesn't
>>> account the TX bytes correctly, because it calls
>>> c_can_inval_msg_object() (which clears the msg ctrl register) before
>>> reading the DLC value:
>>>
>>> The fix is quite easy: Just move c_can_inval_msg_object() to the end of
>>> the if() statement. So:
>>>       * We only call c_can_inval_msg_object() if the message was
>>>         actually transmitted
>>>       * We read out the DLC value _before_ clearing the msg ctrl
>>>         register
>>>
>>> Signed-off-by: Jan Altenberg <jan-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>> Acked-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
> Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

Applied.