Re: [RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger <wg@grandegger.com>]

On 12/07/2012 12:34 AM, Marc Kleine-Budde wrote:
> On 12/06/2012 06:14 PM, Alexander Stein wrote:
>>> This is a typical out-of-sequence reception with a 
>>> RX-goes-into-first-free-mailbox hardware. I've implemented the
>>> algorithm used in the at91 and fixed the one for the ti_hecc.
>>
>> Marc, do you mean out-of-sequence reception with such mailbox types
>> can happen even with the algorithm used in c_can and at91?
> 
> It can happen, if the algorithm isn't implemented properly and adopted
> to the needs of the hardware. On the unmodified ti_hecc we see this
> out-of-sequence problems, too.

Yep, that's the tricky part and the pch_can and c_can use a different
approach to solve the problem. Let's concentrate on the c_can driver.
Key-point is that the controller does put the message into the FIFO
object with the lowest number. Here is the relevant code:

/*
 * theory of operation:
 *
 * c_can core saves a received CAN message into the first free message
 * object it finds free (starting with the lowest). Bits NEWDAT and
 * INTPND are set for this message object indicating that a new message
 * has arrived. To work-around this issue, we keep two groups of message
 * objects whose partitioning is defined by C_CAN_MSG_OBJ_RX_SPLIT.
 *
 * To ensure in-order frame reception we use the following
 * approach while re-activating a message object to receive further
 * frames:
 * - if the current message object number is lower than
 *   C_CAN_MSG_RX_LOW_LAST, do not clear the NEWDAT bit while clearing
 *   the INTPND bit.
 * - if the current message object number is equal to
 *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of all lower
 *   receive message objects.
 * - if the current message object number is greater than
 *   C_CAN_MSG_RX_LOW_LAST then clear the NEWDAT bit of
 *   only this message object.
 */
static int c_can_do_rx_poll(struct net_device *dev, int quota)
{
	u32 num_rx_pkts = 0;
	unsigned int msg_obj, msg_ctrl_save;
	struct c_can_priv *priv = netdev_priv(dev);
	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);

	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
			msg_obj++) {
		/*
		 * as interrupt pending register's bit n-1 corresponds to
		 * message object n, we need to handle the same properly.
		 */
		if (val & (1 << (msg_obj - 1))) {
			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
					~IF_COMM_TXRQST);
			msg_ctrl_save = priv->read_reg(priv,
					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));

			if (msg_ctrl_save & IF_MCONT_EOB)
				return num_rx_pkts;

			if (msg_ctrl_save & IF_MCONT_MSGLST) {
				c_can_handle_lost_msg_obj(dev, IFACE_RX,
								msg_obj);
				num_rx_pkts++;
				quota--;
				continue;
			}

			if (!(msg_ctrl_save & IF_MCONT_NEWDAT))
				continue;

			/* read the data from the message object */
			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);

			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
				c_can_mark_rx_msg_obj(dev, IFACE_RX,
						msg_ctrl_save, msg_obj);
			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
				/* activate this msg obj */
				c_can_activate_rx_msg_obj(dev, IFACE_RX,
						msg_ctrl_save, msg_obj);
			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
				/* activate all lower message objects */
				c_can_activate_all_lower_rx_msg_obj(dev,
						IFACE_RX, msg_ctrl_save);

			num_rx_pkts++;
			quota--;
		}
	}

	return num_rx_pkts;
}


I see a problem if the current message object number is greater than
C_CAN_MSG_RX_LOW_LAST, which is #8. Lets assume the FIFO filled up to
message number #8 when we read INTPND1_REG. Before the we call
c_can_activate_all_lower_rx_msg_obj(), another message is received
into #9. Then we call c_can_activate_all_lower_rx_msg_obj() and shortly
after another messages is received into #0. The next time we enter
this function it will read first #1 and then #9, which is the wrong
order.

Wolfgang.