Re: peak_pci: TX Frame Loss

[Stephane Grosjean <s.grosjean@peak-system.com>]

Hi Andri,

We're currently investigating hard onto this issue. On my side, I'm 
checking the software part and I'd like to discuss with you about one 
point:

AFAIK the "sja1000_start_xmit()" function calls are really serialized by 
the network core. So there's actually no need of any lock to share the 
access of the SJA1000 registers, regarding the Tx path. Right. But I 
don't see in this function any test condition over the Transmit Status 
bit before writing the outgoing CAN frame (aka "reg[SJA1000_SR] & SR_TS").

Without testing the Transmit Status bit before writing, wouldn't it be 
possible that any 2nd task overwrite a 1st frame written by a 1st task 
before it has been really sent by the SJA1000?

Regards,

Stéphane

Le 08/12/2015 13:24, Andri Yngvason a écrit :
> Hi Stephane,
>
> On Tue, Dec 08, 2015 at 12:42:58PM +0100, Stephane Grosjean wrote:
>> Hi Andri,
>>
>> So, you mean that, for example, the below sequence (extract from _xmit()):
>>
>>                  priv->write_reg(priv, SJA1000_FI, fi);
>>                  priv->write_reg(priv, SJA1000_ID1, (id & 0x000007f8) >> 3);
>>                  priv->write_reg(priv, SJA1000_ID2, (id & 0x00000007) << 5);
>>                  ...
>>                  (1)
>>                  ...
>>                  sja1000_write_cmdreg(priv, CMD_TR);
>>
>> can't be interrupted at all (especially at (1)), by any IRQ which ISR could
>> write (again) some new ID?
>> I mean, shouldn't the SJA1000 and the sequence order of the cmds be
>> considered as shared resource?
>>
> AFAIK only _xmit() writes to these registers and I think the _xmit() function is
> only ever executed on one core at a time. Executing two of them at the same time
> wouldn't make much sense. It would just cause out-of-order transmission.
>
> Also, the interrupt routine does not call _xmit() directly. It just calls
> netif_wake_queue() which AFAIK schedules the _xmit() routine to be called later.
>
> I'm just saying that we shouldn't place locks where they are not needed because
> it really just obfuscates the real issue. It's the equivalent of leaving the
> mess on the ground and just walking around it instead of cleaning it up. Someone
> else will step into it.
>
> Besides, ftrace does not show _xmit() being called concurrently for the frames
> that are lost.
>
> By the way, I'm not sure that I understood you correctly. Did placing the spin
> locks prevent the issue? I'm not sure this "guess what" rhetoric works well on a
> mailing list. ;)
>
> Thanks,
> Andri

--

We hereby inform you that PEAK-System Europe will be closed from 24 December 2015 untill 4 January 2016. 
During this period there will be only limited software and technical support available.

 We wish you all a merry christmas and a happy new year!

 -The PEAK-System Europe  Team -

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt
Handelsregister Darmstadt HRB 9183 
Geschaeftsfuehrung: Alexander Gach, Uwe Wilhelm
--