Re: [PATCH v15 13/22] media: i2c: add Maxim GMSL2/3 serializer and deserializer framework

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Using devres allocation (`devm_kcalloc`) in a dynamically called helper function leads to an unbounded memory leak.
- [Medium] max_serdes_get_tpg_timings() returns 0 (success) without initializing the timings output parameter if entry is NULL.
- [Medium] The commit message claims the framework enables support for multiple features that are entirely absent from the code.
- [Medium] Output parameter 'doubled_bpp' is left uninitialized on success path in max_serdes_process_bpps.
- [Low] Most of the framework functions lack EXPORT_SYMBOL_GPL, rendering them unusable by external modules.
--

--- Patch [13]: [PATCH v15 13/22] media: i2c: add Maxim GMSL2/3 serializer and deserializer framework ---
commit 318efe00d7eeba7a3e3cd45eca83e30dec1c313f
Author: Cosmin Tanislav <[email protected]>

media: i2c: add Maxim GMSL2/3 serializer and deserializer framework

These drivers are meant to be used as a common framework for Maxim
GMSL2/3 serializers and deserializers.

> This framework enables support for the following new features across
> all the chips:
>  * Full Streams API support
>  * .get_frame_desc()
>  * .get_mbus_config()
>  * I2C ATR
>  * automatic GMSL link version negotiation
>  * automatic stream id selection
>  * automatic VC remapping
>  * automatic pixel mode / tunnel mode selection
>  * automatic double mode selection / data padding
>  * logging of internal state and chip status registers via .log_status()
>  * PHY modes
>  * serializer pinctrl
>  * TPG

[Severity: Medium]
The commit message explicitly claims that the framework enables support
for .get_mbus_config(), automatic GMSL link version negotiation, VC remapping,
pinctrl, and other features.

However, the submitted code only includes basic data structures and a few helper
functions for streams and TPG. The logic for the claimed features appears to be
missing.

Could the commit message be updated to reflect only the features
currently implemented in this patch?

> diff --git a/drivers/media/i2c/maxim-serdes/max_serdes.c b/drivers/media/i2c/maxim-serdes/max_serdes.c
> new file mode 100644
> index 0000000000000..7d013c4739d11
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_serdes.c

[ ... ]

> +int max_serdes_process_bpps(struct device *dev, u32 bpps,
> +			    u32 allowed_double_bpps, unsigned int *doubled_bpp)
> +{
> +	unsigned int min_bpp;
> +	unsigned int max_bpp;
> +	bool doubled = false;
> +
> +	if (!bpps)
> +		return 0;
> +
> +	*doubled_bpp = 0;

[Severity: Medium]
If a caller invokes max_serdes_process_bpps() with bpps == 0, the function
returns 0 (success) immediately before the *doubled_bpp = 0 assignment. 

Will this cause the caller to consume an uninitialized value for doubled_bpp?
Should *doubled_bpp = 0 be moved before the if (!bpps) check?

[ ... ]

> +int max_serdes_get_streams_masks(struct device *dev,
> +				 const struct v4l2_subdev_state *state,
> +				 u32 pad, u64 updated_streams_mask,
> +				 u32 num_pads, u64 *old_streams_masks,
> +				 u64 **new_streams_masks, bool enable)
> +{
> +	u64 *streams_masks;
> +	unsigned int i;
> +
> +	streams_masks = devm_kcalloc(dev, num_pads, sizeof(*streams_masks), GFP_KERNEL);

[Severity: High]
Does using devm_kcalloc() here lead to an unbounded memory leak?

This helper function appears to be called dynamically during stream
enable/disable operations. Because devm_kcalloc() ties the memory
lifecycle to the device rather than the stream operation, won't repeated
stream start/stop cycles continually leak memory until the device is unbound?

[ ... ]

> +int max_serdes_get_tpg_timings(const struct max_serdes_tpg_entry *entry,
> +			       struct max_serdes_tpg_timings *timings)
> +{
> +	u32 fps;
> +
> +	if (!entry)
> +		return 0;

[Severity: Medium]
When a caller invokes max_serdes_get_tpg_timings() with entry == NULL, this
early return path yields 0 (success) without initializing the timings
output parameter. 

Will callers relying on a success return code then consume uninitialized
stack or heap data?

[ ... ]

> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_NS_GPL(max_serdes_get_tpg_timings, "MAX_SERDES");

[Severity: Low]
Most of the framework functions in this file lack EXPORT_SYMBOL_GPL
declarations. 

Since this framework is built as a module and intended to be consumed by
external serializer drivers, will dependent modules fail to link against
these unexported symbols?

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