Re: [PATCH v11 5/6] clk: scmi: Introduce common header for SCMI clock interface

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Architecture / Unsafe Global State Exposure
- [Low] Compilation Error / Missing Include
- [Low] Header Incompleteness / Missing Includes
- [Low] Linker Error / Missing Export
--

--- Patch [5]: [PATCH v11 5/6] clk: scmi: Introduce common header for SCMI clock interface ---
commit 543bf2610f13161874461756bcae09693a9e083b
Author: Peng Fan <[email protected]>

clk: scmi: Introduce common header for SCMI clock interface

Added a new header file 'clk-scmi.h' to define common structures and
interfaces for the SCMI clock driver. This header will also be used by
OEM-specific extensions to ensure consistency and reusability.

> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 7c562559ad8bb..d88e78cc9a12e 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -13,32 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/scmi_protocol.h>

[Severity: Low]
Is an #include "clk-scmi.h" missing here?

Since struct scmi_clk and to_scmi_clk were moved to the new header, they will
be undefined in this file without the include. For example, in
scmi_clk_recalc_rate(), struct scmi_clk *clk = to_scmi_clk(hw); will trigger a
compilation error.

>  
> -#define NOT_ATOMIC	false
> -#define ATOMIC		true

[ ... ]

> -
> -#define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw)
> +const struct scmi_clk_proto_ops *scmi_proto_clk_ops;

[Severity: Low]
If this global variable is meant to be shared via extern for OEM modules, does
it need EXPORT_SYMBOL_GPL(scmi_proto_clk_ops)?

Without it, external modules will likely encounter linker errors when they
attempt to use it.

[Severity: Medium]
Could this global state exposure lead to a NULL pointer dereference?

The pointer is populated asynchronously during probe in scmi_clocks_probe():

	scmi_proto_clk_ops =
		handle->devm_protocol_get(sdev, SCMI_PROTOCOL_CLOCK, &ph);

If an external OEM module loads and accesses scmi_proto_clk_ops before the
main driver completes probe, it could dereference a NULL pointer.

> diff --git a/drivers/clk/clk-scmi.h b/drivers/clk/clk-scmi.h
> new file mode 100644
> index 0000000000000..6ef6adc77c836
> --- /dev/null
> +++ b/drivers/clk/clk-scmi.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Copyright 2025 NXP
> + */
> +
> +#ifndef __SCMI_CLK_H
> +#define __SCMI_CLK_H
> +
> +#include <linux/bits.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/types.h>
> +

[ ... ]

> +
> +struct scmi_clk {
> +	u32 id;
> +	struct device *dev;
> +	struct clk_hw hw;

[Severity: Low]
Should this header include <linux/clk-provider.h>?

It embeds struct clk_hw hw by value, which requires the full definition of the
struct. Any external source file including clk-scmi.h that hasn't previously
included <linux/clk-provider.h> will fail to compile.

> +	const struct scmi_clock_info *info;
> +	const struct scmi_protocol_handle *ph;
> +	struct clk_parent_data *parent_data;
> +};
> +
> +#define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw)
> +
> +extern const struct scmi_clk_proto_ops *scmi_proto_clk_ops;
> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5