Re: [PATCH] can: ti_hecc: fix rx wrong sequence issue

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 11/05/2012 12:34 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> On 10/08/2012 08:40 AM, AnilKumar Ch wrote:
>> Fix "received wrong sequence count. expected: x, got: y" errors
>> reported by cansequence in a stress test (--verbose).
>>
>> While processing the receive packets in mailboxes, upper mailboxes
>> need to enable while processing 12th (RX_BUFFER mailbox) mailbox.
>> This requires a status check to identify whether the receiving of
>> packets in progress or not. If the mailboxes are enabled before the
>> receive packet status is cleared then there is a possibility of
>> receiving out of order packets.
>>
>> With this patch mailboxes are enabled once the receive status is
>> cleared.
>>
[..]

> Then I had a closer look at AnilKumar Ch's patch, the canme register is
> not changed if a the CAN core currently receives a CAN frame. I added
> AnilKumar Ch's busy loop before modifying the canme register, and it
> works now. So I conclude that the "pick next free mailbox" algorithm in
> the CAN core is racy. You are not allowed to do something that changes a
> mailbox's status from disabled/full to enabled/free when a CAN frame is
> received. As you cannot control CAN frame reception, this is a nice
> hardware race condition. Please add this to the manual and errata sheets.

I really want to know where the race window starts and ends, in order to
specify proper timeouts.

This is a hunk from AnilKumar Ch's patch:

>  			/* enable high bank mailboxes */
> +			timeout = jiffies + usecs_to_jiffies(RM_TIMEOUT_US);
> +			while (hecc_get_bit(priv, HECC_CANES, HECC_CANES_RM)) {
> +				cpu_relax();
> +				if (time_after(jiffies, timeout)) {
> +					netdev_warn(ndev, "receiving pkt\n");
> +					break;
> +				}
> +			}
> +

You first wait for reception to finish, but then might block on this
spin_lock. This introduces a software race condition.

>  			spin_lock_irqsave(&priv->mbx_lock, flags);
>  			mbx_mask = hecc_read(priv, HECC_CANME);
>  			mbx_mask |= HECC_RX_HIGH_MBOX_MASK;

We have to use the spin_lock because the CANME register is shared
between the rx and tx path. I think it's better to make use of
CANRPM+CANMIM, because I think the critical path is the modification of
the CANRPM register, which is not shared, so it can be written without
the need for the spin_lock.

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