Re: [PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver state structure `st` is never saved to the device's drvdata, leading to a guaranteed NULL pointer dereference in PM callbacks.
- [Low] Redundant error code argument in `dev_err_probe()` format string.
--

commit d1ce5988d50518ee3fa99e6870abfe673fb42169
Author: Chris Morgan <macromorgan@hotmail.com>

    iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
    
    Add the core component of a new inv_icm42607 driver. This includes
    a few setup functions and the full register definition in the
    header file.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> new file mode 100644
> index 0000000000000..74d4cb03f7b86
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> +int inv_icm42607_core_probe(struct regmap *regmap,
> +			    const struct inv_icm42607_hw *hw,
> +			    inv_icm42607_bus_setup bus_setup)
> +{
> +	struct device *dev = regmap_get_device(regmap);
> +	struct inv_icm42607_state *st;
> +	int ret;
> +
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	st->hw = hw;
> +	st->map = regmap;
> +
> +	ret = iio_read_mount_matrix(dev, &st->orientation);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to retrieve mounting matrix %d\n", ret);

[Severity: Low]
This isn't a bug, but is it necessary to include the %d format specifier and
ret argument in this dev_err_probe() call?

The dev_err_probe() API natively prefixes the log message with the provided
error code, so passing it as an explicit format argument might result in the
error code being printed twice.

> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get vdd regulator\n");

[ ... ]

> +	/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
> +	ret = inv_icm42607_setup(st, bus_setup);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: High]
Should this code save the driver state structure to the device's drvdata?

Because runtime PM is enabled at the end of the driver series,
inv_icm42607_runtime_suspend() will automatically trigger after the
autosuspend delay.

It calls dev_get_drvdata(dev) to retrieve the state structure, which will
return NULL since it was never set here.

Could this lead to a NULL pointer dereference shortly after initialization
when the PM callbacks try to access the state structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604201832.60656-1-macroalpha82@gmail.com?part=3