Re: [PATCH v2 1/2] clk: Add gpio-locked clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Bypassing the consumer clock API to use `__clk_get_hw()` on parent clocks leads to a Use-After-Free condition.
- [High] The `.is_enabled` callback returns inverted boolean states, breaking CCF state tracking.
- [High] The driver calls `gpiod_get_array_value()` within atomic CCF callbacks, triggering a sleep-in-atomic panic for sleepable GPIOs.
- [High] The driver registers multiple input clocks as parents of a multiplexer, which permanently disables all but the first input clock.
- [High] The driver incorrectly assigns local consumer connection IDs to `init.parent_names`, causing CCF parent resolution to fail.
- [Medium] The driver fails to explicitly configure the GPIO pins as inputs, potentially reading output latches.
- [Medium] The `is_gpio_ready` function prints warnings inside an atomic context and can pass a NULL pointer to a `%s` format specifier.
--

commit 3bd5345760ec5123d8088eb2dc06d995c3669406
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

clk: Add gpio-locked clock driver

A gpio-locked clock aggregates one or more input clocks and/or one or
more GPIOs. 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
> 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);

Will this trigger a sleep-in-atomic panic for sleepable GPIOs?

is_gpio_ready() is called from the .enable and .is_enabled callbacks,
which execute in an atomic context holding the enable_lock spinlock.

If a consumer attaches a GPIO from a sleepable bus controller (e.g., an I2C or
SPI expander), the underlying GPIO driver will attempt to sleep while the
spinlock is held.

> +
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to read GPIOs");
> +		return -EIO;
> +	}
> +
> +	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]);

Can this spam the kernel log from an atomic context?

This warning executes under a spinlock during CCF sysfs reads or consumer
polling.

Also, if the gpio-names DT property is missing, priv->gpio_names[i] will
be NULL, which is then passed to the %s format specifier.

> +			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);

Does bypassing the consumer clock API lead to a Use-After-Free condition?

The driver acquires consumer clock handles (struct clk *) via
devm_clk_bulk_get_all(). However, this extracts the provider's
struct clk_hw * pointer using the internal API __clk_get_hw().

If the parent clock's provider is unbound (e.g., via sysfs), its clk_hw
memory is freed, but the consumer's struct clk * handle remains valid.
Subsequent state checks will dereference the freed hw pointer.

> +
> +		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;
> +}

Does this return inverted boolean states?

The Common Clock Framework (CCF) expects the .is_enabled callback to return
a boolean-like integer (non-zero for enabled, 0 for disabled).

This function returns 0 when all inputs are locked and ready, and -EBUSY
(which evaluates to a non-zero, true value) when the inputs are not ready.
This will cause CCF to track the clock as actively enabled when its
dependencies are unavailable, and disabled when it is fully ready.

[ ... ]
> +static u8 gpio_locked_clk_get_parent(struct clk_hw *hw)
> +{
> +	return 0;
> +}
[ ... ]
> +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);

Could this read output latches instead of the physical external signal?

The GPIOD_ASIS flag leaves the pins in their current or default state.
If the bootloader or prior configuration left a pin as an output,
gpiod_get_array_value() will read the output latch value.

Explicitly requesting GPIOD_IN configures the pins safely as inputs.

[ ... ]
> +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;
> +
> +	of_property_read_string_index(np, "clock-output-names", 0,
> +		&priv->output_clock_name);
> +
> +	if (!priv->output_clock_name)
> +		priv->output_clock_name = dev_name(priv->dev);
> +
> +	init.name = priv->output_clock_name;
> +	init.ops = &gpio_locked_clk_ops;
> +	init.flags = 0;
> +	init.num_parents = priv->num_clks;

Does this register multiple input clocks as parents of a multiplexer?

By setting init.num_parents = priv->num_clks and defining a .get_parent
callback that strictly returns 0, the driver configures itself as a clock
multiplexer.

In the CCF topology, a multiplexer only activates its currently selected
parent (index 0). Any additional input clocks (parents 1 to N) will never
be prepared or enabled by the framework.

> +
> +	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;

Will this cause CCF parent resolution to fail?

The driver assigns priv->clks[j].id to init.parent_names[j]. This id
string populated by devm_clk_bulk_get_all() is the local consumer name from
the device tree's clock-names property.

However, init.parent_names requires the globally registered provider names
of the parent clocks. Supplying the local consumer IDs will cause string
matching to fail, leaving the clock orphaned.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-feature-clock-guard-v2-0-6c25458d5340@bruker.com?part=1