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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] clk: gpio-locked-fixed-clock breaks topology by passing global name to fw_name
- [High] clk: inverted boolean returns in .is_enabled corrupt clock state
- [High] clk: sleeping gpiod_get_value called from atomic .enable callback
- [Medium] clk: parent clock unconditionally enabled defeating PM, no lock polling
--

commit 5585fb0b2689bcfcfc380b86553df6e0d756293f
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
Subject: clk: Add gpio-locked fixed clock driver

This commit adds a gpio-locked clock driver, which exposes a clock whose
status is determined by a GPIO signal. It is intended for FPGA-assisted
clocking designs, allowing consumers to wait until the input clock is
locked before initializing dependent peripherals.

> diff --git a/drivers/clk/clk-gpio-locked.c b/drivers/clk/clk-gpio-locked.c
> new file mode 100644
> index 0000000000000..b648f8763922e
> --- /dev/null
> +++ b/drivers/clk/clk-gpio-locked.c

[ ... ]

> +static int gpio_locked_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct gpio_locked_clk_priv *priv = to_gpio_locked_clk_priv(hw);
> +
> +	int data = gpiod_get_value(priv->gpios);

[Severity: High]
Could this trigger a sleeping-while-atomic panic if the GPIO is on a slow
bus?

The core clock framework executes the .is_enabled and .enable callbacks
with the enable_lock spinlock held (interrupts disabled). 

Since the probe function doesn't check gpiod_cansleep(), accessing a
potentially sleepable GPIO here could result in a crash.

Would it be better to either use .prepare and .is_prepared instead, which
execute under a mutex, or explicitly reject sleepable GPIOs during probe?

> +
> +	if (data < 0) {
> +		dev_err(priv->dev, "Failed to get data gpio val: %d\n",
> +				data);
> +		return data;
> +	} else if (!data) {
> +		dev_warn(priv->dev, "GPIO is not ready");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Does this logic invert the clock state for the common clock framework?

The framework expects .is_enabled to return a boolean-equivalent value
(non-zero for enabled, 0 for disabled). 

Returning -EBUSY evaluates to true (enabled), and 0 evaluates to false
(disabled). 

Could this break clock state tracking and bypass protection mechanisms like
clk_disable_unused by telling the framework the clock is enabled when it
is actually locked, 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: Medium]
Does this instantly fail instead of waiting for the lock as the commit
message suggests?

The commit message indicates consumers can wait until the input clock is
locked. However, because this immediately returns -EBUSY if the GPIO is low
rather than polling or blocking, consumers cannot natively wait for the
lock.

Could we implement a polling loop with a timeout inside a .prepare callback
instead?

[ ... ]

> +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;
> +	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);
> +
> +	priv->output_hw_clk.init =
> +		CLK_HW_INIT_FW_NAME(priv->output_clock_name,
> +				    __clk_get_name(priv->input_clk),
> +				    &gpio_locked_clk_ops, 0);

[Severity: High]
Does using the global clock name with CLK_HW_INIT_FW_NAME break the
parent-child topology?

The CLK_HW_INIT_FW_NAME macro assigns the provided string to the .fw_name
field, which the clock framework uses to look up the parent via
of_parse_clkspec() against the local device tree's clock-names property.

Since __clk_get_name() returns a globally unique provider name instead of a
local device tree name, the lookup will fail and register this clock as an
orphan. 

Since the driver already has the input clock pointer, would it work better
to use CLK_HW_INIT_HW and pass __clk_get_hw(priv->input_clk) instead?

[ ... ]

> +static int gpio_locked_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct gpio_locked_clk_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->input_clk = devm_clk_get_enabled(priv->dev, NULL);

[Severity: Medium]
Does acquiring an irreversible enable reference here at probe time defeat
dynamic power management?

By permanently enabling the parent clock at probe time, it seems the 
framework loses the ability to disable the parent clock when the
gpio-locked clock is unused.

Would it be better to use devm_clk_get() instead, relying on the consumers
to trigger the enable path when needed?

> +	if (IS_ERR(priv->input_clk))
> +		return dev_err_probe(priv->dev, PTR_ERR(priv->input_clk),
> +				"Failed to get locked fixed clock, not yet ready\n");
> +
> +	priv->gpios = devm_gpiod_get(priv->dev, "locked", GPIOD_ASIS);

[Severity: High]
Should this check if the GPIO is safe to use in atomic context?

If the driver intends to use this GPIO inside the atomic .enable callback,
should it verify gpiod_cansleep() is false here to avoid potential panics
when reading the value later?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-feature-clock-guard-v5-0-42ab5dc3a6aa@bruker.com?part=2