Resending to a larger e-mail list...
On Mon, 2017-01-30 at 04:57 -0800, Michael Zoran wrote:
> I'm looking at linux-next:
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>
> First it looks this is some kind of startup notification mechanism
> and
> it is used by the custom RPI kernel on www.github.com. Client drivers
> call vchiq_add_connected_callback to register for a notification that
> the driver is good to go.
>
> But I see a few interesting issues:
>
> 1. vchiq_add_connected_callback can be called by multiple clients at
> the same time. If this happens they will all find that g_once_init
> isn't set, so all the clients will call mutex_init at the same time.
> That's sounds like a possible corruption issue.
>
> 2. On a multiprocessor machine, I didn't think it was OK to simply
> set
> g_once_init without any kind of protection either since it may not
> propagate to the other cores. You know some kind of barrier
> instruction.
>
> 3. A change was made in checkin
> 826d73b3024485677163253b59ef9bd187ff765.
>
> This changed the function mutex_lock_interruptable which was at that
> time a macro to a custom function called
> mutex_lock_interruptable_killable to simply mutex_lock_killable. The
> old function does a dance with setting the process signal masks, but
> mutex_lock_killable doesn't.
>
> Like I said, I'm new to trying to contribute to the linux kernel. But
> I'm not sure the two are completely a drop in replacement. I was
> wondering if the change is doing the correct thing?
>
> It looks a fix would be easy to implement just by adding an atomic
> exchange instead of a simple test and set.
>
> Thoughts?
>
>
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>
> static void connected_init(void)
> {
> if (!g_once_init) {
> mutex_init(&g_connected_mutex);
> g_once_init = 1;
> }
> }
>
> /********************************************************************
> **
> ******
> *
> * This function is used to defer initialization until the vchiq stack
> is
> * initialized. If the stack is already initialized, then the callback
> will
> * be made immediately, otherwise it will be deferred until
> * vchiq_call_connected_callbacks is called.
> *
> *********************************************************************
> **
> ****/
>
> void vchiq_add_connected_callback(VCHIQ_CONNECTED_CALLBACK_T
> callback)
> {
> connected_init();
>
> if (mutex_lock_killable(&g_connected_mutex) != 0)
> return;
>
> if (g_connected)
> /* We're already connected. Call the callback
> immediately. */
>
> callback();
> else {
> if (g_num_deferred_callbacks >= MAX_CALLBACKS)
> vchiq_log_error(vchiq_core_log_level,
> "There already %d callback registered
> -
> "
> "please increase MAX_CALLBACKS",
> g_num_deferred_callbacks);
> else {
> g_deferred_callback[g_num_deferred_callbacks]
> =
> callback;
> g_num_deferred_callbacks++;
> }
> }
> mutex_unlock(&g_connected_mutex);
> }
>
> /********************************************************************
> **
> ******
> *
> * This function is called by the vchiq stack once it has been
> connected
> to
> * the videocore and clients can start to use the stack.
> *
> *********************************************************************
> **
> ****/
>
> void vchiq_call_connected_callbacks(void)
> {
> int i;
>
> connected_init();
>
> if (mutex_lock_killable(&g_connected_mutex) != 0)
> return;
>
> for (i = 0; i < g_num_deferred_callbacks; i++)
> g_deferred_callback[i]();
>
> g_num_deferred_callbacks = 0;
> g_connected = 1;
> mutex_unlock(&g_connected_mutex);
> }
> EXPORT_SYMBOL(vchiq_add_connected_callback);
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
