netdev - Re: can: c_can: TX delivery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110323085340.GC346@e-circ.dyndns.org>
Date:	Wed, 23 Mar 2011 09:53:40 +0100
From:	Kurt Van Dijck <kurt.van.dijck@....be>
To:	Jan Altenberg <jan@...utronix.de>
Cc:	bhupesh.sharma@...com, wg@...ndegger.com, b.spranger@...utronix.de,
	netdev@...r.kernel.org
Subject: Re: can: c_can: TX delivery

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html