From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1F8E630567B for ; Wed, 3 Jun 2026 11:49:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780487387; cv=none; b=TKnuci8rHii0wCtawFG+GUct7U9pmEZtw7Cv4pvKsZNL673F6yC+LgffFC+QmWHvsE8yehfKaVThFDZpxHjSGUzKbXlRA9PGC3nGi7lI8FAvWdSgc00UdxWUERI8sgXBT9Nj610XeHSXQn3wPqGfokfRximgrQnHPlvqF23Q5oI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780487387; c=relaxed/simple; bh=5WYqa1g6S3Srxp29PLvPbFXXLuQROA62D+QIdoTrc38=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=CJZ5nz43Qh/Dby+s4uhxxZe5gicKy+w7wRHAhQk/N4csADOteqRHqcvab/lh5mHKr9+A5v/4xR/5mlLJns/+e2PGiGPJ5ewj9DOkx64CMuhcdkxF23NyETN1vYv3kt8mM1BxsBNbDDxfw96JbIK1N6wUw5n34s8Ql3zT5GwVhgk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=MoPLcjQh; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="MoPLcjQh" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 598F31F00899; Wed, 3 Jun 2026 11:49:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780487385; bh=HkiPbtY5/X4lDL1CH+PIxT+P5jW+l5bFJW/iRFV33Mc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=MoPLcjQh5zGZAQCFIq5i/v2P6D7s/+2Y5y6cKLMHg3Vsq9gXYErBmxWx+PCFELWVu b3HBNO0lFVoIvFweGZoSogjSF3YBOSgCfm+RBBrTGYIqCmBVE6/Of/oV9bCHYqVUa1 mXjz1P+pc3+1+VvNPMyrc8T6+rh6V6immUPkIWAj8iRg89bd76P0SwcD+YUcG4Gqbw izsNLDMu8SEwObxywAMfUr4cOYTnDGvoF/673wCSe/pPbnHs1D+w/HZPU3PDeloL9U kBy1q9HVyQzIv8tzrYvrKRggavA0h1vwERjKBsA8s3gpHWzsUDhrVsG3BWSASc4Eb8 fNeLxj8SiEaFw== From: sashiko-bot@kernel.org Subject: Re: [PATCH v3 2/2] clk: Add gpio-locked fixed clock driver Reply-To: sashiko-reviews@lists.linux.dev To: "Vyacheslav Yurkov via B4 Relay" Cc: robh@kernel.org, conor+dt@kernel.org, devicetree@vger.kernel.org In-Reply-To: <20260603-feature-clock-guard-v3-2-01cca0aa04a5@bruker.com> References: <20260603-feature-clock-guard-v3-2-01cca0aa04a5@bruker.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Wed, 03 Jun 2026 11:49:44 +0000 Message-Id: <20260603114945.598F31F00899@smtp.kernel.org> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 7 potential issue(= s) to consider: - [High] The driver reuses its `.enable` function for `.is_enabled`, return= ing 0 for ready and `-EBUSY` for not ready, which inverts CCF's `.is_enable= d` boolean semantics. - [High] The driver returns `-EBUSY` immediately from `.enable()` instead o= f internally waiting for the clock to lock, inappropriately forcing consume= rs to poll. - [High] The driver attempts to aggregate multiple input clocks using `num_= parents > 1`, but CCF treats multiple parents as a multiplexer (MUX), not a= logical AND gate. - [High] The driver populates `init.parent_names` with local consumer conne= ction IDs instead of globally registered clock names, breaking parent clock= resolution. - [High] The driver violates API boundaries by using `__clk_get_hw()` on co= nsumer `struct clk` pointers to check `clk_hw_is_enabled()`, which only che= cks software state and risks use-after-free. - [High] Calling `gpiod_get_array_value()` from `.enable()` and `.is_enable= d()` triggers a sleeping-in-atomic BUG if the GPIOs are on a slow bus. - [Medium] Input GPIOs are requested with `GPIOD_ASIS` instead of `GPIOD_IN= `, failing to guarantee the lines are configured as inputs. -- commit a891e5fe0dbdc27c3cfbbf69bfe6b147ef67ce9c Author: Vyacheslav Yurkov clk: Add gpio-locked fixed clock driver A gpio-locked clock aggregates one or more input clocks and/or one or more GPIOs. It's a FPGA-assisted clocking design where peripheral clocks are generated by FPGA PLLs that are outside CPU control, with clock-valid/PLL-lock status exposed through GPIO signals. Consumers can use the output clock to wait until all input clocks are locked and only then initialize dependent peripherals. > diff --git a/drivers/clk/clk-gpio-locked.c b/drivers/clk/clk-gpio-locked.c > new file mode 100644 > index 0000000000000..79098f9b65326 > --- /dev/null > +++ b/drivers/clk/clk-gpio-locked.c [ ... ] > +static int is_gpio_ready(struct gpio_locked_clk_priv *priv) > +{ > + unsigned long values[BITS_TO_LONGS(MAX_INPUT_GPIO_COUNT)] =3D {0}; > + int ret =3D 0; > + > + if (priv->num_gpios =3D=3D 0) > + return 0; > + > + ret =3D gpiod_get_array_value(priv->gpios->ndescs, > + priv->gpios->desc, > + priv->gpios->info, > + values); [Severity: High] Will this trigger a sleeping in atomic context warning if the GPIOs are on a slow bus? CCF .enable() and .is_enabled() callbacks execute in an atomic context under the global enable_lock spinlock with IRQs disabled. If the GPIOs are on an I2C/SPI expander, gpiod_get_array_value() requires a sleepable context. Should .prepare() and .is_prepared() be used instead? > + > + if (ret) { > + dev_err(priv->dev, "Failed to read GPIOs"); > + return -EIO; > + } [ ... ] > +static int gpio_locked_clk_is_enabled(struct clk_hw *hw) > +{ > + struct gpio_locked_clk_priv *priv =3D to_gpio_locked_clk_priv(hw); > + int ret =3D 0; > + > + if (priv->num_gpios > 0) { > + ret =3D is_gpio_ready(priv); > + if (ret < 0) > + return ret; > + } [Severity: High] Does this logic invert the CCF state machine? The framework expects .is_enabled() to return non-zero if the clock is enabled and 0 if disabled. This function returns 0 when ready, and -EBUSY when not ready. Because -EBUSY (-16) is non-zero, CCF interprets a 'not ready' state as 'enabled', and a 'ready' state as 'disabled'. > + > + // Now check for the clocks > + for (int i =3D 0; i < priv->num_clks; i++) { > + struct clk_hw *hw_clk =3D __clk_get_hw(priv->clks[i].clk); > + > + if (!clk_hw_is_enabled(hw_clk)) { [Severity: High] Is it safe to use __clk_get_hw() on arbitrary consumer struct clk pointers? This internal CCF macro returns a raw pointer that could be freed if the provider unbinds, creating a use-after-free risk. Additionally, clk_hw_is_enabled() only checks the software enable_count rather than actual hardware PLL lock states. > + dev_dbg(priv->dev, "Clock %i (%s) is not ready", > + i, priv->clks[i].id); > + return -EBUSY; > + } > + } > + > + return 0; > +} > + > +/* We can't enable the clock, but the Common Clock Framework calls only > + * enable() not is_enabled() > + */ > +static int gpio_locked_clk_enable(struct clk_hw *hw) > +{ > + return gpio_locked_clk_is_enabled(hw); > +} [Severity: High] Can this cause consumers to fail instantly without waiting for the clock to lock? Standard clock consumers expect clk_prepare_enable() to block until the clock is stable or return a timeout error. By returning -EBUSY immediately without polling, consumer drivers calling clk_prepare_enable() during probe will instantly fail if the clock isn't already locked. Should the clock provider poll the lock status internally (preferably within its .prepare() callback)? > + > +/* We have to implement it, but we are not going to control > + * parent clock selection > + */ > +static u8 gpio_locked_clk_get_parent(struct clk_hw *hw) > +{ > + return 0; > +} [Severity: High] Does CCF treat num_parents > 1 as a multiplexer rather than an AND-gate? When CCF propagates clk_enable(), it only enables the single active parent determined by .get_parent(), which hardcodes to 0 here. This means input clocks 1 through N-1 are never automatically enabled, causing the checks in gpio_locked_clk_is_enabled() to permanently fail unless those clocks happen to be enabled externally. > + > +static const struct clk_ops gpio_locked_clk_ops =3D { > + .enable =3D gpio_locked_clk_enable, > + .is_enabled =3D gpio_locked_clk_is_enabled, > + .get_parent =3D gpio_locked_clk_get_parent, > +}; [ ... ] > +static int gpio_locked_clk_parse_gpios(struct gpio_locked_clk_priv *priv) > +{ > + struct device *dev =3D priv->dev; > + struct device_node *np =3D dev->of_node; > + int i; > + > + priv->gpios =3D devm_gpiod_get_array_optional(dev, "locked", GPIOD_ASIS= ); [Severity: Medium] Does requesting with GPIOD_ASIS guarantee the lines are configured as input= s? If the bootloader left the pins in an output state, reading them might retu= rn the internal output latch value instead of the external signal. Should GPIOD_IN be used instead? > + if (IS_ERR(priv->gpios)) { > + dev_err(dev, "failed to get GPIO array: %ld\n", > + PTR_ERR(priv->gpios)); > + return PTR_ERR(priv->gpios); > + } [ ... ] > +static int gpio_locked_clk_parse_outputs(struct gpio_locked_clk_priv *pr= iv) > +{ > + struct device *dev =3D priv->dev; > + struct device_node *np =3D dev->of_node; > + struct clk_init_data init =3D {}; > + int ret; [ ... ] > + if (priv->num_clks) { > + const char **parent_names; > + int j; > + > + parent_names =3D devm_kcalloc(dev, priv->num_clks, > + sizeof(*parent_names), > + GFP_KERNEL); > + if (!parent_names) > + return -ENOMEM; > + > + for (j =3D 0; j < priv->num_clks; j++) > + parent_names[j] =3D priv->clks[j].id; > + > + init.parent_names =3D parent_names; [Severity: High] Does this assign local consumer connection names to init.parent_names instead of globally registered clock names? devm_clk_bulk_get_all() fills the id field with the local consumer connection name from the clock-names DT property, but init.parent_names expects global provider names. This could prevent CCF from linking the clock hierarchy correctly. Could init.parent_hws or init.parent_data be used instead? > + } > + > + priv->output_hw_clk.init =3D &init; > + > + ret =3D devm_clk_hw_register(dev, &priv->output_hw_clk); --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260603-feature-cl= ock-guard-v3-0-01cca0aa04a5@bruker.com?part=3D2