WIP: spinlock: fix struct comparison for ticket spinlock

[sumpfralle]

Summary

Previously compilation with ticket spinlock enabled failed for the rp2040 board due to comparisons between structs:

CC:  netdb/lib_proto.c chip/rp2040_testset.c: In function 'up_testset':
chip/rp2040_testset.c:73:11: error: invalid operands to binary == (have 'spinlock_t' {aka 'union spinlock_u'} and 'union spinlock_u')
   73 |   if (ret == SP_UNLOCKED)
      |           ^~

Now all occurrences of comparisons are replaced with the existing macro spin_is_locked. Most of these occurrences were not relevant, since they were located in the spinlock code branches for "non-ticket" mode (with spinlock_t being an integer). But in order to keep everything clear and predictable, we want to avoid potential struct comparisons everywhere.

Impact

Based on the existing definition of the spin_is_locked macro for non-ticket mode there should be no change compared to the previous simple comparison:

#ifdef CONFIG_TICKET_SPINLOCK
#  define spin_is_locked(l) ((*l).tickets.owner != (*l).tickets.next)
#else
#  define spin_is_locked(l) (*(l) == SP_LOCKED)
#endif

Testing

Now the code compiles cleanly for rp2040.

[sumpfralle]

OK, I am feeling a bit stupid now.

The change obviously fails to compile for non-ticket spinlock mode due to the interface of spin_is_locked (insisting on a pointer instead of a value):

semaphore/spinlock.c: In function 'spin_lock_wo_note':
semaphore/spinlock.c:128:25: error: lvalue required as unary '&' operand
  128 |   while (spin_is_locked(&up_testset(lock)))
      |                         ^

A possible solution would be to change the interface of the macro from spin_is_locked(spinlock_t*) to spin_is_locked(spinlock_t).

This should work easily, since every caller is using the address operator for the macro argument:

$ grep -r "spin_is_locked([^&]" | wc -l
2
$ grep -r "spin_is_locked(&" | wc -l
71

Spoiler: the two exceptions above are located in the macro definition itself.

In addition, spin_is_locked is not used in the nuttx-apps repository:

$ grep -r spin_is_locked ../nuttx-apps/ | wc -l
0

Changing the interface from a pointer to a value obviously relies on the argument being a small value (or the evaluation always being implemented as a preprocessor macro).

But I am not in the position to judge, whether it would be acceptable to change the interface of spin_is_locked.

(the obvious alternatives would be to (1) introduce another macro accepting a value or (2) adding conditional compilation directives around all comparisons related to spinlock_t)

What do you think?

[acassis]

@sumpfralle I think you forgot to include the header files, this is causing the failures in the CI

[sumpfralle]

Thank you for taking a look!

I think you forgot to include the header files, this is causing the failures in the CI

I think, everything is there. The CI (a static checker?) complains, since the macro is sadly defined in the header file below the one function using it. This function (in this header file) is a static inline function. Thus, during compilation the macro is already known whenever this inline function is embedded. At least I did not see a complaint in my compiler run.

But I can obviously move the previously existing macro definition (spin_is_locked) a bit higher in order to satisfy the static checker (and to avoid confusion for future readers). I just wanted to keep the diff minimal.

But the real problem is the pointer/value issue, which I described above, I guess.