Need a second set of eyeballs for a possible startup race condition in vc04_services/vchiq.

[Michael Zoran <mzoran@xxxxxxxxxxxx>]

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