[RFC,09/18] staging: vchiq_core: do not initialize semaphores twice
diff mbox series

Message ID 20181026134813.7775-10-nsaenzjulienne@suse.de
State New, archived
Headers show
Series
  • staging: vchiq: remove dead code & misc fixes
Related show

Commit Message

Nicolas Saenz Julienne Oct. 26, 2018, 1:48 p.m. UTC
vchiq_init_state() initialises a series of semaphores to then call
remote_event_create() on the same semaphores, which initializes them
again.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c   | 5 -----
 1 file changed, 5 deletions(-)

Comments

Stefan Wahren Oct. 28, 2018, 8:45 p.m. UTC | #1
Hi Nicolas,

> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> hat am 26. Oktober 2018 um 15:48 geschrieben:
> 
> 
> vchiq_init_state() initialises a series of semaphores to then call
> remote_event_create() on the same semaphores, which initializes them
> again.

i would prefer to have all init stuff at one place in vchiq_init_state() and drop this ugliness from remote_event_create() instead. Is this possible?
Nicolas Saenz Julienne Nov. 6, 2018, 3:41 p.m. UTC | #2
Hi Stefan,
thanks for spending the time reviewing the code. I took note of the
rest of comments.

On Sun, 2018-10-28 at 21:45 +0100, Stefan Wahren wrote:
> Hi Nicolas,
> 
> > Nicolas Saenz Julienne <nsaenzjulienne@suse.de> hat am 26. Oktober
> > 2018 um 15:48 geschrieben:
> > 
> > 
> > vchiq_init_state() initialises a series of semaphores to then call
> > remote_event_create() on the same semaphores, which initializes
> > them
> > again.
> 
> i would prefer to have all init stuff at one place in
> vchiq_init_state() and drop this ugliness from remote_event_create()
> instead. Is this possible?

As I'm sure you're aware of, REMOTE_EVENT_T is shared between the CPU
and VC4, which can't be expanded. And since storing a pointer is out of
question because of arm64, I can only think of storing an index to an
array of completions in the shared structure instead of the pointer
magic implemented right now. It would be a little more explicit. Then
we could completely decouple both initializations. I'm not sure if it's
similar to what you had in mind. 

On a semi-related topic, I'm curious to know why these shared
structures aren't set with the "__packed" preprocessor macro. Any
ideas? As fas as I've been told, in general, the compiler may reorder
or add unexpected padding to any structure. Which would be very bad in
this case.

Regards,
Nicolas
Stefan Wahren Nov. 6, 2018, 4:06 p.m. UTC | #3
Am 06.11.18 um 16:41 schrieb Nicolas Saenz Julienne:
> Hi Stefan,
> thanks for spending the time reviewing the code. I took note of the
> rest of comments.
>
> On Sun, 2018-10-28 at 21:45 +0100, Stefan Wahren wrote:
>> Hi Nicolas,
>>
>>> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> hat am 26. Oktober
>>> 2018 um 15:48 geschrieben:
>>>
>>>
>>> vchiq_init_state() initialises a series of semaphores to then call
>>> remote_event_create() on the same semaphores, which initializes
>>> them
>>> again.
>> i would prefer to have all init stuff at one place in
>> vchiq_init_state() and drop this ugliness from remote_event_create()
>> instead. Is this possible?
> As I'm sure you're aware of, REMOTE_EVENT_T is shared between the CPU
> and VC4, which can't be expanded. And since storing a pointer is out of
> question because of arm64, I can only think of storing an index to an
> array of completions in the shared structure instead of the pointer
> magic implemented right now. It would be a little more explicit. Then
> we could completely decouple both initializations. I'm not sure if it's
> similar to what you had in mind. 

I don't think so, this was my intention:

 static inline void
 remote_event_create(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
 {
    event->armed = 0;
    /* Don't clear the 'fired' flag because it may already have been set
    ** by the other side. */
-    sema_init((struct semaphore *)((char *)state + event->event), 0);
 }


>
> On a semi-related topic, I'm curious to know why these shared
> structures aren't set with the "__packed" preprocessor macro. Any
> ideas? As fas as I've been told, in general, the compiler may reorder
> or add unexpected padding to any structure. Which would be very bad in
> this case.

This would be better, but i assume the firmware side uses the same
source code. So using __packed only on ARM side could also break :-(

>
> Regards,
> Nicolas
Nicolas Saenz Julienne Nov. 6, 2018, 6:28 p.m. UTC | #4
On Tue, 2018-11-06 at 17:06 +0100, Stefan Wahren wrote:
> Am 06.11.18 um 16:41 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for spending the time reviewing the code. I took note of the
> > rest of comments.
> > 
> > On Sun, 2018-10-28 at 21:45 +0100, Stefan Wahren wrote:
> > > Hi Nicolas,
> > > 
> > > > Nicolas Saenz Julienne <nsaenzjulienne@suse.de> hat am 26.
> > > > Oktober
> > > > 2018 um 15:48 geschrieben:
> > > > 
> > > > 
> > > > vchiq_init_state() initialises a series of semaphores to then
> > > > call
> > > > remote_event_create() on the same semaphores, which initializes
> > > > them
> > > > again.
> > > i would prefer to have all init stuff at one place in
> > > vchiq_init_state() and drop this ugliness from
> > > remote_event_create()
> > > instead. Is this possible?
> > As I'm sure you're aware of, REMOTE_EVENT_T is shared between the
> > CPU
> > and VC4, which can't be expanded. And since storing a pointer is
> > out of
> > question because of arm64, I can only think of storing an index to
> > an
> > array of completions in the shared structure instead of the pointer
> > magic implemented right now. It would be a little more explicit.
> > Then
> > we could completely decouple both initializations. I'm not sure if
> > it's
> > similar to what you had in mind. 
> 
> I don't think so, this was my intention:
> 
>  static inline void
>  remote_event_create(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
>  {
>     event->armed = 0;
>     /* Don't clear the 'fired' flag because it may already have been
> set
>     ** by the other side. */
> -    sema_init((struct semaphore *)((char *)state + event->event),
> 0);
>  }

Fair enough, even simpler.

> 
> 
> > On a semi-related topic, I'm curious to know why these shared
> > structures aren't set with the "__packed" preprocessor macro. Any
> > ideas? As fas as I've been told, in general, the compiler may
> > reorder
> > or add unexpected padding to any structure. Which would be very bad
> > in
> > this case.
> 
> This would be better, but i assume the firmware side uses the same
> source code. So using __packed only on ARM side could also break :-(

True. Yet in that case, aren't we still relying on the fact that both
compilers are going to behave the same way?
 
> 
> > Regards,
> > Nicolas

Patch
diff mbox series

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index dee5ea7bfe4f..6242357735b9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2200,11 +2200,6 @@  vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T *slot_zero)
 
 	sema_init(&state->connect, 0);
 	mutex_init(&state->mutex);
-	sema_init(&state->trigger_event, 0);
-	sema_init(&state->recycle_event, 0);
-	sema_init(&state->sync_trigger_event, 0);
-	sema_init(&state->sync_release_event, 0);
-
 	mutex_init(&state->slot_mutex);
 	mutex_init(&state->recycle_mutex);
 	mutex_init(&state->sync_mutex);