Re: at91_can.c: Data transmission stops

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

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

On 11/28/2012 04:09 PM, Henrik Bork Steffensen wrote:
> On 11/28/2012 03:29 PM, Marc Kleine-Budde wrote:
>> On 11/28/2012 03:22 PM, Henrik Bork Steffensen wrote:
>>> On 11/27/2012 05:31 PM, Wolfgang Grandegger wrote:
>>>> On 11/27/2012 03:11 PM, Henrik Bork Steffensen wrote:
>>>> Hm, could you show your diffs.
>>> Do You mean a diff on these 7 lines, or a diff to the original file?
>>>
>>>>> I this case "at91_poll" is basicly the same as "c_can_poll", in both
>>>>> cases they call the function with the spinlock in the rx chain.
>>>> You don't need to protect against RX. Sorry, forgot that. On the c_can
>>>> this is necessary due to concurrent accesses to the same message RAM.
>>> Ok, I think that at91_can.c might have an issue in register access.
>>> I am not sure, but I will look into it.
>>>
>>>>> Looking at the patch Wolfgang sugested, I became uncertain of what
>>>>> this
>>>>> patch actually wants to protect.
>>>>> Is it the registers in the cpu can interface? (mailboxes and control
>>>>> regs, i don't know the hw)
>>>> As mentioned above, on the c_can there is definitely a race with the
>>>> message ram due to the busy wait after accessing it. See:
>>>>
>>>>    
>>>> http://lxr.linux.no/#linux+v3.6.8/drivers/net/can/c_can/c_can.c#L237
>>>>
>>>>> Or is it the potential race between "c_can_start_xmit" and
>>>>> "c_can_do_tx" ?
>>>>> Or even the access to the net api?
>>>>>
>>>>> Would someone care to explain?
>>>> I will try. In at91_start_xmit, if we get interrupted
>>>>
>>>>      if (!(at91_read(priv, AT91_MSR(get_tx_next_mb(priv)))&
>>>>                 AT91_MSR_MRDY) ||
>>>>                (priv->tx_next&   get_next_mask(priv)) == 0)
>>>>
>>>>          /* HERE */
>>>>
>>>>          netif_stop_queue(dev);
>>>>
>>>> and then at91_irq_tx() is called executing netif_wake_queue() we may
>>>> end
>>>> up with a stopped tx queue. But I'm not yet 100% sure.
>>> Ok, thanks a lot.
>>>
>>> In my case i changed the driver to only use one mailbox for
>>> transmission.
>>> Which means that the "net_stop_queue" will be called every time a packet
>>> is tx'ed.
>>> And the "net_wake_queue" will be called after the packet is actually
>>> transmitted.
>> In your first mail you've written that using only one mailbox increases
>> the probability for a lockup.
>>
>>> This is as far as i can see this is 100% safe, provided that no further
>>> "ndo_start_xmit"
>>> calls come before the wake_queue call.
>>>
>>>
>>> Anyway, after removing the spin_lock from rx, it loads fine and seems to
>>> work.
>>> I will do a test with the suggested changes to the tx chain and get to
>>> the list
>>> if anything interesting appears.
>>>
>>> Thank You very much for Your help so far :-)
>> Can you send me a diff of your current changes?
> 
> Hi,
> 
> Please note that i have not yet tested it for lockup.
> So far i just did a simple rx/tx test.

How many TX mailboxes are you using? According to your patch, the number
is unchanged.

> Patch attached.
> Expect large offsets in line numbers.
> 
> I have a few other small changes to at91_can.c which might be interesting.
> I will get back with them, after checking them against a recent kernel
> version.

Cool, I'm looking forward to see them.

tnx,
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: 261 bytes --]