[PATCH 29/30] leon, greth: SMP support by using spin-lock protection

[Daniel Hellstrom]

On 2017-05-02 11:00, Sebastian Huber wrote:
>
>
> On 02/05/17 10:51, Daniel Hellstrom wrote:
>>
>> On 2017-05-02 10:27, Sebastian Huber wrote:
>>>
>>>
>>> On 02/05/17 10:16, Daniel Hellstrom wrote:
>>>> On 2017-05-02 10:00, Sebastian Huber wrote:
>>>>> On 02/05/17 09:32, Daniel Hellstrom wrote:
>>>>>> On 2017-05-02 07:48, Sebastian Huber wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 13/04/17 21:31, Daniel Hellstrom wrote:
>>>>>>>> ---
>>>>>>>>   c/src/lib/libbsp/sparc/shared/net/greth.c | 134 
>>>>>>>> ++++++++++++++++--------------
>>>>>>>>   1 file changed, 71 insertions(+), 63 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/c/src/lib/libbsp/sparc/shared/net/greth.c 
>>>>>>>> b/c/src/lib/libbsp/sparc/shared/net/greth.c
>>>>>>>> index 7726799..9af2de2 100644
>>>>>>>> --- a/c/src/lib/libbsp/sparc/shared/net/greth.c
>>>>>>>> +++ b/c/src/lib/libbsp/sparc/shared/net/greth.c
>>>>>>>> @@ -41,6 +41,18 @@
>>>>>>>>   #include <netinet/in.h>
>>>>>>>>   #include <netinet/if_ether.h>
>>>>>>>>   +#include <rtems/score/isrlock.h> /* spin-lock */
>>>>>>>> +
>>>>>>>> +/* map via ISR lock: */
>>>>>>>> +#define SPIN_DECLARE(lock) ISR_LOCK_MEMBER(lock)
>>>>>>>> +#define SPIN_INIT(lock, name) _ISR_lock_Initialize(lock, name)
>>>>>>>> +#define SPIN_LOCK(lock, level) _ISR_lock_Acquire_inline(lock, 
>>>>>>>> &level)
>>>>>>>> +#define SPIN_LOCK_IRQ(lock, level) 
>>>>>>>> _ISR_lock_ISR_disable_and_acquire(lock, &level)
>>>>>>>> +#define SPIN_UNLOCK(lock, level) 
>>>>>>>> _ISR_lock_Release_inline(lock, &level)
>>>>>>>> +#define SPIN_UNLOCK_IRQ(lock, level) 
>>>>>>>> _ISR_lock_Release_and_ISR_enable(lock, &level)
>>>>>>>> +#define SPIN_IRQFLAGS(k) ISR_lock_Context k
>>>>>>>> +#define SPIN_ISR_IRQFLAGS(k) SPIN_IRQFLAGS(k)
>>>>>>>
>>>>>>> Could you please use the rtems_interrupt_lock_*() API and not 
>>>>>>> the internal _ISR_lock_*() stuff.
>>>>>>
>>>>>> Thanks. I think the reason for this may come from that the inline 
>>>>>> equivalent. I will add a patch which adds inline macros to the 
>>>>>> rtems_interrupt_lock interface instead. I can't see any reason 
>>>>>> why that can not be supported?
>>>>>
>>>>> What do you mean with inline equivalent?
>>>> I mean we use _ISR_lock_Acquire_inline(lock, &level) from the ISR 
>>>> to avoid an extra function call. But there is only a 
>>>> rtems_interrupt_lock_acquire_isr() no 
>>>> rtems_interrupt_lock_acquire_INLINE_isr() which we would like to use.
>>>
>>> Ok, but does this extra function call really matter? If you think 
>>> so, then please add the *_inline variants to the 
>>> rtems_interrupt_lock_* API (with tests) and document it in the user 
>>> manual.
>> Ok. I will add that patch afterwards, first I will fix our patches to 
>> use the correct existing macros. 
>
> Ok. You can commit the patch set from my point of view (with the minor 
> fixes).

good, thanks for the review.

>
>> On SPARC it would be nice to avoid the extra window SAVE within the ISR.
>
> Since, greth_interrupt() conditionally calls rtems_bsdnet_event_send() 
> the greth_interrupt() already uses a SAVE, the SMP lock acquire is a 
> tail function.

I agree, I hope to use the macros in several ISRs in some it might not 
have an effect in others it may.

/Daniel