Re: [PATCH v4 2/2] clk: Add gpio-locked fixed clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] The `.is_enabled` callback completely inverts the clock status logic, returning error codes instead of boolean state.
- [High] The driver attempts to aggregate multiple dependent input clocks by registering them as CCF parents, causing a TOCTOU race and failing to enable parents.
- [High] Using local DT connection IDs to populate `init.parent_names` causes global clock matching to fail and risks NULL pointer dereferences.
- [High] The `enable` callback returns `-EBUSY` immediately rather than waiting for the hardware lock, contradicting CCF semantics.
- [High] The driver requests the lock GPIOs with `GPIOD_ASIS` rather than `GPIOD_IN`, potentially failing to read the pin state correctly.
- [High] The driver fails to validate that provided GPIOs are non-sleeping, risking a kernel panic if slow GPIO controllers are used.
- [Medium] Calling `dev_warn` from a spinlocked, atomic context causes severe latencies.
--

--- Patch [2]: [PATCH v4 2/2] clk: Add gpio-locked fixed clock driver ---
commit 86eabe35b2750726ea6024147b3e2f51a67773ca
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

clk: Add gpio-locked fixed clock driver

This commit introduces a new clock driver that aggregates input clocks and
GPIOs to expose a clock-valid/PLL-lock status for FPGA-assisted clocking
designs.

> 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
> @@ -0,0 +1,306 @@
[ ... ]
> +static int is_gpio_ready(struct gpio_locked_clk_priv *priv)
> +{
> +	unsigned long values[BITS_TO_LONGS(MAX_INPUT_GPIO_COUNT)] = {0};
> +	int ret = 0;
[ ... ]
> +	for (int i = 0; i < priv->gpios->ndescs; i++) {
> +		if (!test_bit(i, values)) {
> +			dev_warn(priv->dev, "GPIO %s is not ready", priv->gpio_names[i]);

[Severity: Medium]
Is it safe to call dev_warn() while holding the clock framework's enable_lock?

Because is_gpio_ready() is called from the .enable and .is_enabled callbacks,
it executes under a spinlock with hard interrupts disabled. Unconditionally
logging to the console from this context can cause severe IRQ latencies or
hard lockups, especially if a consumer polls the clock state.

> +			return -EBUSY;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int gpio_locked_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct gpio_locked_clk_priv *priv = to_gpio_locked_clk_priv(hw);
> +	int ret = 0;
> +
> +	if (priv->num_gpios > 0) {
> +		ret = is_gpio_ready(priv);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	// Now check for the clocks
> +	for (int i = 0; i < priv->num_clks; i++) {
> +		struct clk_hw *hw_clk = __clk_get_hw(priv->clks[i].clk);
> +
> +		if (!clk_hw_is_enabled(hw_clk)) {
> +			dev_dbg(priv->dev, "Clock %i (%s) is not ready",
> +				i, priv->clks[i].id);
> +			return -EBUSY;
> +		}
> +	}
> +
> +	return 0;

[Severity: Critical]
Does this function invert the expected return logic for .is_enabled?

The clock framework expects .is_enabled to return a non-zero value when the
clock is enabled, and 0 when it is disabled. By returning -EBUSY when not
ready and 0 when ready, the framework will consider the clock enabled when
it is actually not ready, and disabled when it is ready.

> +}
> +
> +/* 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]
Should the enable callback block until the hardware lock is acquired?

Standard clock consumers do not poll clk_prepare_enable(). By doing a single
non-blocking check and failing immediately with -EBUSY, consumer driver probes
will fail if the hardware lock is not instantaneously ready at the moment of
enablement.

> +}
> +
> +/* 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 hardcoding the parent to index 0 break the aggregation of multiple inputs?

When a clock is registered with num_parents > 1 and implements get_parent,
the clock framework treats it as a multiplexer where only one parent is active.

By always returning 0, the framework is told to abandon tracking for all other
parents. This means parents 1 through N will not be enabled when this clock
is enabled.

> +}
> +
> +static const struct clk_ops gpio_locked_clk_ops = {
> +	.enable =	gpio_locked_clk_enable,
> +	.is_enabled =	gpio_locked_clk_is_enabled,
> +	.get_parent =	gpio_locked_clk_get_parent,
> +};
[ ... ]
> +static int gpio_locked_clk_parse_gpios(struct gpio_locked_clk_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	int i;
> +
> +	priv->gpios = devm_gpiod_get_array_optional(dev, "locked", GPIOD_ASIS);

[Severity: High]
Could requesting the GPIO array with GPIOD_ASIS leave the pins in an undefined
state?

Without enforcing the input direction using GPIOD_IN, the hardware
configuration relies on the bootloader. If the bootloader leaves the pins
floating or configured as outputs, reading the pin state may fail to detect
the lock correctly.

[Severity: High]
Does the driver need to verify that these GPIOs are non-sleeping?

The clock framework's .enable and .is_enabled callbacks run in an atomic
context with interrupts disabled. If the device tree provides a sleeping GPIO
(such as an I2C or SPI expander), calling gpiod_get_array_value() from
is_gpio_ready() will trigger a scheduling-while-atomic BUG.

Could the probe function check !gpiod_cansleep() to reject sleeping GPIOs?

> +	if (IS_ERR(priv->gpios)) {
> +		dev_err(dev, "failed to get GPIO array: %ld\n",
> +				PTR_ERR(priv->gpios));
[ ... ]
> +static int gpio_locked_clk_parse_outputs(struct gpio_locked_clk_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	struct clk_init_data init = {};
> +	int ret;
[ ... ]
> +	init.flags = 0;
> +	init.num_parents = priv->num_clks;
> +
> +	if (priv->num_clks) {
> +		const char **parent_names;
> +		int j;
> +
> +		parent_names = devm_kcalloc(dev, priv->num_clks,
> +					    sizeof(*parent_names),
> +					    GFP_KERNEL);
> +		if (!parent_names)
> +			return -ENOMEM;
> +
> +		for (j = 0; j < priv->num_clks; j++)
> +			parent_names[j] = priv->clks[j].id;

[Severity: High]
Does this assignment pass local device tree connection IDs to the clock
framework instead of globally registered provider names?

During probe, devm_clk_bulk_get_all() populates the id field with local
consumer names from the device tree. The clock framework expects
init.parent_names to contain global provider names. This mismatch will
cause the output clock to fail to match its parents.

Additionally, if the optional clock-names property is missing in the DT,
priv->clks[j].id will be NULL, causing a NULL pointer dereference when the
framework evaluates init.parent_names.

> +
> +		init.parent_names = parent_names;
> +	}
> +
> +	priv->output_hw_clk.init = &init;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-feature-clock-guard-v4-0-e9c8b372b71c@bruker.com?part=2