Received-SPF: pass (google.com: domain of linux-kernel+bounces-25053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Jan 2024 17:00:04 -0500
From: Ben Wolsieffer <ben.wolsieffer@hefring.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Michael Turquette <mturquette@baylibre.com>
Subject: Re: [PATCH 1/2] clk: stm32: initialize syscon after clocks are
 registered
Message-ID: <ZaG2ZDCLP34jcI6Y@dell-precision-5540>
References: <20231002180854.1603452-1-ben.wolsieffer@hefring.com>
 <20231002180854.1603452-2-ben.wolsieffer@hefring.com>
 <883a61872f94c972cc410da84eaf7b97.sboyd@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <883a61872f94c972cc410da84eaf7b97.sboyd@kernel.org>

On Sun, Dec 17, 2023 at 03:05:01PM -0800, Stephen Boyd wrote:
> Quoting Ben Wolsieffer (2023-10-02 11:08:53)
> > The stm32-power-config syscon (PWR peripheral) is used in this driver
> > and the STM32 RTC driver to enable write access to backup domain
> > registers. The syscon's clock has a gate controlled by this clock
> > driver, but this clock is currently not registered in the device tree.
> > This only happens to work currently because all relevant clock setup and
> > RTC initialization happens before clk_disabled_unused(). After this
> > point, all syscon register writes are ignored.
> 
> Seems like we should mark those clks as CLK_IGNORE_UNUSED and add a
> comment to that fact.

That seems like a worse solution than specifying the clock dependency in
the device tree.

> 
> > 
> > If we simply add the syscon clock in the device tree, we end up with a
> > circular dependency because the clock has not been registered at the
> > point this driver requests the syscon.
> > 
> > This patch avoids this circular dependency by moving the syscon lookup
> > after the clocks are registered. This does appear to create a possible
> > race condition where someone could attempt to perform an operation on a
> > backup domain clock before the syscon has been initialized. This would
> > result in the operation having no effect because backup domain writes
> > could not be enabled. I'm not sure if this is a problem or if there is
> > a way to avoid it.
> 
> There's no comment in the code that says the regmap must be set there
> instead of earlier. What's to stop someone from tripping over this
> problem later? At the least, please add a comment.

Yeah, I'll fix that. Do you have any thoughts on the race condition I
described? Should I add some kind of locking to block
enable/disable_power_domain_write_protection() until stm32f4_rcc_init()
attempts to initialize the syscon?

Thank you, Ben