From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753327AbdA3NC5 (ORCPT ); Mon, 30 Jan 2017 08:02:57 -0500 Received: from gw.crowfest.net ([52.42.241.221]:43140 "EHLO gw.crowfest.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753087AbdA3NCq (ORCPT ); Mon, 30 Jan 2017 08:02:46 -0500 Message-ID: <1485781364.13537.11.camel@crowfest.net> Subject: Need a second set of eyeballs for a possible startup race condition in vc04_services/vchiq. From: Michael Zoran To: linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Date: Mon, 30 Jan 2017 05:02:44 -0800 In-Reply-To: <1485781074.13537.9.camel@crowfest.net> References: <1485781074.13537.9.camel@crowfest.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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);