All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: Stefan Wahren <stefan.wahren@i2se.com>
Cc: linux-rpi-kernel@lists.infradead.org, eric@anholt.net,
	gregkh@linuxfoundation.org, linux-arm-kernel@lists.infradead.org,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
	dave.stevenson@raspberrypi.org
Subject: Re: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice
Date: Tue, 06 Nov 2018 16:41:19 +0100	[thread overview]
Message-ID: <ddef268b97543abc9de05842b92e1fc5f0134de8.camel@suse.de> (raw)
In-Reply-To: <1781917429.4071.1540759539282@email.ionos.de>

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

WARNING: multiple messages have this Message-ID
From: nsaenzjulienne@suse.de (Nicolas Saenz Julienne)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice
Date: Tue, 06 Nov 2018 16:41:19 +0100	[thread overview]
Message-ID: <ddef268b97543abc9de05842b92e1fc5f0134de8.camel@suse.de> (raw)
In-Reply-To: <1781917429.4071.1540759539282@email.ionos.de>

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181106/39a145f7/attachment.sig>

  reply	other threads:[~2018-11-06 15:41 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-26 13:47 [PATCH RFC 00/18] staging: vchiq: remove dead code & misc fixes Nicolas Saenz Julienne
2018-10-26 13:47 ` Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 01/18] staging: vchiq_core: rework vchiq_get_config Nicolas Saenz Julienne
2018-10-26 13:47   ` Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 02/18] staging: vchiq_arm: rework close/remove_service IOCTLS Nicolas Saenz Julienne
2018-10-26 13:47   ` Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 03/18] staging: vchiq_shim: delete vchi_service_create Nicolas Saenz Julienne
2018-10-26 13:47   ` Nicolas Saenz Julienne
2018-10-26 13:47 ` [PATCH RFC 04/18] stagning: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list Nicolas Saenz Julienne
2018-10-26 13:47   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 05/18] staging: vchiq_arm: get rid of vchi_mh.h Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 06/18] staging: vchiq_arm: rework vchiq_ioc_copy_element_data Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 07/18] staging: vchiq-core: get rid of is_master distinction Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 08/18] staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-28 20:45   ` Stefan Wahren
2018-10-28 20:45     ` Stefan Wahren
2018-11-06 15:41     ` Nicolas Saenz Julienne [this message]
2018-11-06 15:41       ` Nicolas Saenz Julienne
2018-11-06 16:06       ` Stefan Wahren
2018-11-06 16:06         ` Stefan Wahren
2018-11-06 18:28         ` Nicolas Saenz Julienne
2018-11-06 18:28           ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 10/18] staging: vchiq_core: don't add a wmb() before remote_event_signal() Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 11/18] staging: vchiq_arm: use completions instead of semaphores Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-28 21:00   ` Stefan Wahren
2018-10-28 21:00     ` Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 12/18] staging: vchiq_util: " Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 13/18] staging: vchiq_core: " Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 14/18] staging: vchiq_util: get rid of unneeded memory barriers Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 15/18] stagning: vchiq_core: fix logic redundancy in parse_open Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-28 20:58   ` Stefan Wahren
2018-10-28 20:58     ` Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 16/18] staging: vchiq_arm: rework probe and init functions Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-28 21:02   ` Stefan Wahren
2018-10-28 21:02     ` Stefan Wahren
2018-10-26 13:48 ` [PATCH RFC 17/18] staging: vchiq_arm: fix open/release cdev functions Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-26 13:48 ` [PATCH RFC 18/18] staging: vchiq: add more tasks to the TODO list Nicolas Saenz Julienne
2018-10-26 13:48   ` Nicolas Saenz Julienne
2018-10-28 21:11   ` Stefan Wahren
2018-10-28 21:11     ` Stefan Wahren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ddef268b97543abc9de05842b92e1fc5f0134de8.camel@suse.de \
    --to=nsaenzjulienne@suse.de \
    --cc=dave.stevenson@raspberrypi.org \
    --cc=devel@driverdev.osuosl.org \
    --cc=eric@anholt.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=stefan.wahren@i2se.com \
    --subject='Re: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.