From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932597AbdJJSkV (ORCPT ); Tue, 10 Oct 2017 14:40:21 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:51054 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932391AbdJJSkQ (ORCPT ); Tue, 10 Oct 2017 14:40:16 -0400 X-Google-Smtp-Source: AOwi7QBp2kF8Ygr74Wz+Ty78X8Z4svJamQ/F04WciIy9nychE2TxBWHZ/LejUXKm2difjaKwuh3OAw== Date: Tue, 10 Oct 2017 13:40:14 -0500 From: Rob Herring To: Frank Rowand Cc: Pantelis Antoniou , David Airlie , Jyri Sarha , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Mark Rutland , Tomi Valkeinen , dri-devel Subject: Re: [PATCH 09/12] of: overlay: avoid race condition between applying multiple overlays Message-ID: <20171010184014.m2chslx36kbhvgug@rob-hp-laptop> References: <1507002826-16393-1-git-send-email-frowand.list@gmail.com> <1507002826-16393-10-git-send-email-frowand.list@gmail.com> <59D5A737.9090905@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <59D5A737.9090905@gmail.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 04, 2017 at 08:29:59PM -0700, Frank Rowand wrote: > On 10/04/17 08:19, Rob Herring wrote: > > On Mon, Oct 2, 2017 at 10:53 PM, wrote: > >> From: Frank Rowand > >> > >> The process of applying an overlay consists of: > >> - unflatten an overlay FDT (flattened device tree) into an > >> EDT (expanded device tree) > >> - fixup the phandle values in the overlay EDT to fit in a > >> range above the phandle values in the live device tree > >> - create the overlay changeset to reflect the contents of > >> the overlay EDT > >> - apply the overlay changeset, to modify the live device tree, > >> potentially changing the maximum phandle value in the live > >> device tree > >> > >> There is currently no protection against two overlay applies > >> concurrently determining what range of phandle values are in use > >> in the live device tree, and subsequently changing that range. > >> Add a mutex to prevent multiple overlay applies from occurring > >> simultaneously. > >> > >> Ignoring 2 checkpatch warnings: Prefer using '"%s...", __func__' > >> so that the WARN() string will be more easily grepped. > >> > >> Signed-off-by: Frank Rowand > >> --- > >> drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 7 +++++++ > >> drivers/of/overlay.c | 22 ++++++++++++++++++++++ > >> drivers/of/unittest.c | 21 +++++++++++++++++++++ > >> include/linux/of.h | 19 +++++++++++++++++++ > >> 4 files changed, 69 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > >> index 7a7be0515bfd..c99f7924b1c6 100644 > >> --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > >> @@ -221,6 +221,11 @@ static void __init tilcdc_convert_slave_node(void) > >> goto out; > >> } > >> > >> + /* > >> + * protect from of_resolve_phandles() through of_overlay_apply() > >> + */ > >> + of_overlay_mutex_lock(); > >> + > > > > We can't be relying on callers to get the locking right... > > Agreed. > > > > > >> overlay = tilcdc_get_overlay(&kft); > >> if (!overlay) > >> goto out; > >> @@ -256,6 +261,8 @@ static void __init tilcdc_convert_slave_node(void) > >> pr_info("%s: ti,tilcdc,slave node successfully converted\n", > >> __func__); > >> out: > >> + of_overlay_mutex_unlock(); > >> + > >> kfree_table_free(&kft); > >> of_node_put(i2c); > >> of_node_put(slave); > >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > >> index a0d3222febdc..4ed372af6ce7 100644 > >> --- a/drivers/of/overlay.c > >> +++ b/drivers/of/overlay.c > >> @@ -71,6 +71,28 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs, > >> const struct device_node *overlay_node, > >> bool is_symbols_node); > >> > >> +/* > >> + * of_resolve_phandles() finds the largest phandle in the live tree. > >> + * of_overlay_apply() may add a larger phandle to the live tree. > >> + * Do not allow race between two overlays being applied simultaneously: > >> + * mutex_lock(&of_overlay_phandle_mutex) > >> + * of_resolve_phandles() > >> + * of_overlay_apply() > >> + * mutex_unlock(&of_overlay_phandle_mutex) > > > > Why do these need to be separate functions? I think I mentioned it > > before, but essentially overlay_data_add() should be part of the > > overlay API. We may need to allow for callers to do each step, but > > generally I think the interface should just be "apply this fdt blob". > > Yes, that is where I want to end up. So, is that not doable now? To put it another way, why does of_resolve_phandles need to be a separate call? Seems like an internal detail of how you apply an overlay to me. Rob