Re: peak_pci: TX Frame Loss

[Andri Yngvason <andri.yngvason@marel.com>]

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