Re: [PATCH v3 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:
- [High] The driver reuses its `.enable` function for `.is_enabled`, returning 0 for ready and `-EBUSY` for not ready, which inverts CCF's `.is_enabled` boolean semantics.
- [High] The driver returns `-EBUSY` immediately from `.enable()` instead of internally waiting for the clock to lock, inappropriately forcing consumers 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 connection IDs instead of globally registered clock names, breaking parent clock resolution.
- [High] The driver violates API boundaries by using `__clk_get_hw()` on consumer `struct clk` pointers to check `clk_hw_is_enabled()`, which only checks software state and risks use-after-free.
- [High] Calling `gpiod_get_array_value()` from `.enable()` and `.is_enabled()` 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 <V.Yurkov.EXT@bruker.com>

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)] = {0};
> +	int ret = 0;
> +
> +	if (priv->num_gpios == 0)
> +		return 0;
> +
> +	ret = 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 = to_gpio_locked_clk_priv(hw);
> +	int ret = 0;
> +
> +	if (priv->num_gpios > 0) {
> +		ret = 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 = 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)) {

[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 = {
> +	.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: Medium]
Does requesting with GPIOD_ASIS guarantee the lines are configured as inputs?

If the bootloader left the pins in an output state, reading them might return
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 *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np = dev->of_node;
> +	struct clk_init_data init = {};
> +	int ret;
[ ... ]
> +	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;
> +
> +		init.parent_names = 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 = &init;
> +
> +	ret = devm_clk_hw_register(dev, &priv->output_hw_clk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-feature-clock-guard-v3-0-01cca0aa04a5@bruker.com?part=2