From: Maud Spierings maudspierings@gocontroll.com
Throughout the various probe functions &indio_dev->dev is used before it is initialized. This caused a kernel panic in st_sensors_power_enable when the call to devm_regulator_bulk_get_enable() fails and then calls dev_err_probe() with the uninitialized device.
This seems to only cause a panic with dev_err_probe(), dev_err, dev_warn and dev_info don't seem to cause a panic, but are fixed as well.
Signed-off-by: Maud Spierings maudspierings@gocontroll.com --- When I search for general &indio_dev->dev usage, I see quite a lot more hits, but I am not sure if there are issues with those too.
This issue has existed for a long time it seems and therefore it is nearly impossible to find a proper fixes tag. I would love to see it at least backported to 6.12 as that is where I encountered it, and I believe the patch should apply without conflicts.
The investigation into this issue can be found in this thread [1]
[1]: https://lore.kernel.org/all/AM7P189MB100986A83D2F28AF3FFAF976E39EA@AM7P189MB... --- Changes in v2: - Added SoB in commit message - Link to v1: https://lore.kernel.org/r/20250522-st_iio_fix-v1-1-d689b35f1612@gocontroll.c... --- drivers/iio/accel/st_accel_core.c | 10 +++---- drivers/iio/common/st_sensors/st_sensors_core.c | 35 +++++++++++----------- drivers/iio/common/st_sensors/st_sensors_trigger.c | 18 +++++------ 3 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c index 99cb661fabb2d9cc1943fa8d0a6f3becb71126e6..a7961c610ed203d039bbf298c8883031a578fb0b 100644 --- a/drivers/iio/accel/st_accel_core.c +++ b/drivers/iio/accel/st_accel_core.c @@ -1353,6 +1353,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) union acpi_object *ont; union acpi_object *elements; acpi_status status; + struct device *parent = indio_dev->dev.parent; int ret = -EINVAL; unsigned int val; int i, j; @@ -1371,7 +1372,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) };
- adev = ACPI_COMPANION(indio_dev->dev.parent); + adev = ACPI_COMPANION(parent); if (!adev) return -ENXIO;
@@ -1380,8 +1381,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) if (status == AE_NOT_FOUND) { return -ENXIO; } else if (ACPI_FAILURE(status)) { - dev_warn(&indio_dev->dev, "failed to execute _ONT: %d\n", - status); + dev_warn(parent, "failed to execute _ONT: %d\n", status); return status; }
@@ -1457,12 +1457,12 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) }
ret = 0; - dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n"); + dev_info(parent, "computed mount matrix from ACPI\n");
out: kfree(buffer.pointer); if (ret) - dev_dbg(&indio_dev->dev, + dev_dbg(parent, "failed to apply ACPI orientation data: %d\n", ret);
return ret; diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c index 8ce1dccfea4f5aaff45d3d40f6542323dd1f0b09..11cbf561b16d41f429745abb516c137cfbb302bb 100644 --- a/drivers/iio/common/st_sensors/st_sensors_core.c +++ b/drivers/iio/common/st_sensors/st_sensors_core.c @@ -154,7 +154,7 @@ static int st_sensors_set_fullscale(struct iio_dev *indio_dev, unsigned int fs) return err;
st_accel_set_fullscale_error: - dev_err(&indio_dev->dev, "failed to set new fullscale.\n"); + dev_err(indio_dev->dev.parent, "failed to set new fullscale.\n"); return err; }
@@ -231,7 +231,7 @@ int st_sensors_power_enable(struct iio_dev *indio_dev) ARRAY_SIZE(regulator_names), regulator_names); if (err) - return dev_err_probe(&indio_dev->dev, err, + return dev_err_probe(parent, err, "unable to enable supplies\n");
return 0; @@ -241,13 +241,14 @@ EXPORT_SYMBOL_NS(st_sensors_power_enable, "IIO_ST_SENSORS"); static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev, struct st_sensors_platform_data *pdata) { + struct device *parent = indio_dev->dev.parent; struct st_sensor_data *sdata = iio_priv(indio_dev);
/* Sensor does not support interrupts */ if (!sdata->sensor_settings->drdy_irq.int1.addr && !sdata->sensor_settings->drdy_irq.int2.addr) { if (pdata->drdy_int_pin) - dev_info(&indio_dev->dev, + dev_info(parent, "DRDY on pin INT%d specified, but sensor does not support interrupts\n", pdata->drdy_int_pin); return 0; @@ -256,29 +257,27 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev, switch (pdata->drdy_int_pin) { case 1: if (!sdata->sensor_settings->drdy_irq.int1.mask) { - dev_err(&indio_dev->dev, - "DRDY on INT1 not available.\n"); + dev_err(parent, "DRDY on INT1 not available.\n"); return -EINVAL; } sdata->drdy_int_pin = 1; break; case 2: if (!sdata->sensor_settings->drdy_irq.int2.mask) { - dev_err(&indio_dev->dev, - "DRDY on INT2 not available.\n"); + dev_err(parent, "DRDY on INT2 not available.\n"); return -EINVAL; } sdata->drdy_int_pin = 2; break; default: - dev_err(&indio_dev->dev, "DRDY on pdata not valid.\n"); + dev_err(parent, "DRDY on pdata not valid.\n"); return -EINVAL; }
if (pdata->open_drain) { if (!sdata->sensor_settings->drdy_irq.int1.addr_od && !sdata->sensor_settings->drdy_irq.int2.addr_od) - dev_err(&indio_dev->dev, + dev_err(parent, "open drain requested but unsupported.\n"); else sdata->int_pin_open_drain = true; @@ -336,6 +335,7 @@ EXPORT_SYMBOL_NS(st_sensors_dev_name_probe, "IIO_ST_SENSORS"); int st_sensors_init_sensor(struct iio_dev *indio_dev, struct st_sensors_platform_data *pdata) { + struct device *parent = indio_dev->dev.parent; struct st_sensor_data *sdata = iio_priv(indio_dev); struct st_sensors_platform_data *of_pdata; int err = 0; @@ -343,7 +343,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, mutex_init(&sdata->odr_lock);
/* If OF/DT pdata exists, it will take precedence of anything else */ - of_pdata = st_sensors_dev_probe(indio_dev->dev.parent, pdata); + of_pdata = st_sensors_dev_probe(parent, pdata); if (IS_ERR(of_pdata)) return PTR_ERR(of_pdata); if (of_pdata) @@ -370,7 +370,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, if (err < 0) return err; } else - dev_info(&indio_dev->dev, "Full-scale not possible\n"); + dev_info(parent, "Full-scale not possible\n");
err = st_sensors_set_odr(indio_dev, sdata->odr); if (err < 0) @@ -405,7 +405,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, mask = sdata->sensor_settings->drdy_irq.int2.mask_od; }
- dev_info(&indio_dev->dev, + dev_info(parent, "set interrupt line to open drain mode on pin %d\n", sdata->drdy_int_pin); err = st_sensors_write_data_with_mask(indio_dev, addr, @@ -593,21 +593,20 @@ EXPORT_SYMBOL_NS(st_sensors_get_settings_index, "IIO_ST_SENSORS"); int st_sensors_verify_id(struct iio_dev *indio_dev) { struct st_sensor_data *sdata = iio_priv(indio_dev); + struct device *parent = indio_dev->dev.parent; int wai, err;
if (sdata->sensor_settings->wai_addr) { err = regmap_read(sdata->regmap, sdata->sensor_settings->wai_addr, &wai); if (err < 0) { - dev_err(&indio_dev->dev, - "failed to read Who-Am-I register.\n"); - return err; + return dev_err_probe(parent, err, + "failed to read Who-Am-I register.\n"); }
if (sdata->sensor_settings->wai != wai) { - dev_warn(&indio_dev->dev, - "%s: WhoAmI mismatch (0x%x).\n", - indio_dev->name, wai); + dev_warn(parent, "%s: WhoAmI mismatch (0x%x).\n", + indio_dev->name, wai); } }
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c index 9d4bf822a15dfcdd6c2835f6b9d7698cd3cb0b08..32c3278968089699dff5329e943d92b151b55fdf 100644 --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c @@ -127,7 +127,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->trig = devm_iio_trigger_alloc(parent, "%s-trigger", indio_dev->name); if (sdata->trig == NULL) { - dev_err(&indio_dev->dev, "failed to allocate iio trigger.\n"); + dev_err(parent, "failed to allocate iio trigger.\n"); return -ENOMEM; }
@@ -143,7 +143,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, case IRQF_TRIGGER_FALLING: case IRQF_TRIGGER_LOW: if (!sdata->sensor_settings->drdy_irq.addr_ihl) { - dev_err(&indio_dev->dev, + dev_err(parent, "falling/low specified for IRQ but hardware supports only rising/high: will request rising/high\n"); if (irq_trig == IRQF_TRIGGER_FALLING) irq_trig = IRQF_TRIGGER_RISING; @@ -156,21 +156,21 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->sensor_settings->drdy_irq.mask_ihl, 1); if (err < 0) return err; - dev_info(&indio_dev->dev, + dev_info(parent, "interrupts on the falling edge or active low level\n"); } break; case IRQF_TRIGGER_RISING: - dev_info(&indio_dev->dev, + dev_info(parent, "interrupts on the rising edge\n"); break; case IRQF_TRIGGER_HIGH: - dev_info(&indio_dev->dev, + dev_info(parent, "interrupts active high level\n"); break; default: /* This is the most preferred mode, if possible */ - dev_err(&indio_dev->dev, + dev_err(parent, "unsupported IRQ trigger specified (%lx), enforce rising edge\n", irq_trig); irq_trig = IRQF_TRIGGER_RISING; } @@ -179,7 +179,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, if (irq_trig == IRQF_TRIGGER_FALLING || irq_trig == IRQF_TRIGGER_RISING) { if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) { - dev_err(&indio_dev->dev, + dev_err(parent, "edge IRQ not supported w/o stat register.\n"); return -EOPNOTSUPP; } @@ -214,13 +214,13 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->trig->name, sdata->trig); if (err) { - dev_err(&indio_dev->dev, "failed to request trigger IRQ.\n"); + dev_err(parent, "failed to request trigger IRQ.\n"); return err; }
err = devm_iio_trigger_register(parent, sdata->trig); if (err < 0) { - dev_err(&indio_dev->dev, "failed to register iio trigger.\n"); + dev_err(parent, "failed to register iio trigger.\n"); return err; } indio_dev->trig = iio_trigger_get(sdata->trig);
--- base-commit: 7bac2c97af4078d7a627500c9bcdd5b033f97718 change-id: 20250522-st_iio_fix-1c58fdd4d420
Best regards,
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v2] iio: common: st_sensors: Fix use of uninitialize device structs Link: https://lore.kernel.org/stable/20250522-st_iio_fix-v2-1-07a32655a996%40gocon...
On Thu, 22 May 2025 13:18:55 +0200 Maud Spierings via B4 Relay devnull+maudspierings.gocontroll.com@kernel.org wrote:
From: Maud Spierings maudspierings@gocontroll.com
Throughout the various probe functions &indio_dev->dev is used before it is initialized. This caused a kernel panic in st_sensors_power_enable when the call to devm_regulator_bulk_get_enable() fails and then calls dev_err_probe() with the uninitialized device.
Hi Maud,
Curious. Given the device_initialize() is in the allocation function it isn't immediately obvious that something needed might not have been initialized. Any idea what is being accessed in there that fails? (i.e. any idea if my shallow detective work found it ;)
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/iio/industrialio-cor... in iio_device_alloc() is where device_initialize() is called.
Using the device in the iio_dev structure is almost certainly wrong but I'm surprised it crashes... So I had a quick dig.
The only path that isn't a simple print that I can spot is device_set_deferred_reason() That accesses dev->p (device private data) Which is initialized only in device_add().
I wonder if we should harden device_set_deferred_reason() against p == NULL, perhaps with a message strongly advising against using it with a device that hasn't been added?
Being in error paths this is the sort of subtle bug that rarely rears it's head :(
+CC Greg and Dragan for thoughts before anyone spins a patch.
This change is good either way. I'm just on wrong computer to pick it up right now.
Jonathan
This seems to only cause a panic with dev_err_probe(), dev_err, dev_warn and dev_info don't seem to cause a panic, but are fixed as well.
Signed-off-by: Maud Spierings maudspierings@gocontroll.com
When I search for general &indio_dev->dev usage, I see quite a lot more hits, but I am not sure if there are issues with those too.
This issue has existed for a long time it seems and therefore it is nearly impossible to find a proper fixes tag. I would love to see it at least backported to 6.12 as that is where I encountered it, and I believe the patch should apply without conflicts.
The investigation into this issue can be found in this thread [1]
[1]: https://lore.kernel.org/all/AM7P189MB100986A83D2F28AF3FFAF976E39EA@AM7P189MB...
Changes in v2:
- Added SoB in commit message
- Link to v1: https://lore.kernel.org/r/20250522-st_iio_fix-v1-1-d689b35f1612@gocontroll.c...
drivers/iio/accel/st_accel_core.c | 10 +++---- drivers/iio/common/st_sensors/st_sensors_core.c | 35 +++++++++++----------- drivers/iio/common/st_sensors/st_sensors_trigger.c | 18 +++++------ 3 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c index 99cb661fabb2d9cc1943fa8d0a6f3becb71126e6..a7961c610ed203d039bbf298c8883031a578fb0b 100644 --- a/drivers/iio/accel/st_accel_core.c +++ b/drivers/iio/accel/st_accel_core.c @@ -1353,6 +1353,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) union acpi_object *ont; union acpi_object *elements; acpi_status status;
- struct device *parent = indio_dev->dev.parent; int ret = -EINVAL; unsigned int val; int i, j;
@@ -1371,7 +1372,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) };
- adev = ACPI_COMPANION(indio_dev->dev.parent);
- adev = ACPI_COMPANION(parent); if (!adev) return -ENXIO;
@@ -1380,8 +1381,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) if (status == AE_NOT_FOUND) { return -ENXIO; } else if (ACPI_FAILURE(status)) {
dev_warn(&indio_dev->dev, "failed to execute _ONT: %d\n",status);
dev_warn(parent, "failed to execute _ONT: %d\n", status);@@ -1457,12 +1457,12 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev) } ret = 0;
- dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n");
- dev_info(parent, "computed mount matrix from ACPI\n");
out: kfree(buffer.pointer); if (ret)
dev_dbg(&indio_dev->dev,
dev_dbg(parent, "failed to apply ACPI orientation data: %d\n", ret);return ret; diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c index 8ce1dccfea4f5aaff45d3d40f6542323dd1f0b09..11cbf561b16d41f429745abb516c137cfbb302bb 100644 --- a/drivers/iio/common/st_sensors/st_sensors_core.c +++ b/drivers/iio/common/st_sensors/st_sensors_core.c @@ -154,7 +154,7 @@ static int st_sensors_set_fullscale(struct iio_dev *indio_dev, unsigned int fs) return err; st_accel_set_fullscale_error:
- dev_err(&indio_dev->dev, "failed to set new fullscale.\n");
- dev_err(indio_dev->dev.parent, "failed to set new fullscale.\n"); return err;
} @@ -231,7 +231,7 @@ int st_sensors_power_enable(struct iio_dev *indio_dev) ARRAY_SIZE(regulator_names), regulator_names); if (err)
return dev_err_probe(&indio_dev->dev, err,
return dev_err_probe(parent, err, "unable to enable supplies\n");return 0; @@ -241,13 +241,14 @@ EXPORT_SYMBOL_NS(st_sensors_power_enable, "IIO_ST_SENSORS"); static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev, struct st_sensors_platform_data *pdata) {
- struct device *parent = indio_dev->dev.parent; struct st_sensor_data *sdata = iio_priv(indio_dev);
/* Sensor does not support interrupts */ if (!sdata->sensor_settings->drdy_irq.int1.addr && !sdata->sensor_settings->drdy_irq.int2.addr) { if (pdata->drdy_int_pin)
dev_info(&indio_dev->dev,
dev_info(parent, "DRDY on pin INT%d specified, but sensor does not support interrupts\n", pdata->drdy_int_pin);@@ -256,29 +257,27 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev, switch (pdata->drdy_int_pin) { case 1: if (!sdata->sensor_settings->drdy_irq.int1.mask) {
dev_err(&indio_dev->dev,"DRDY on INT1 not available.\n");
dev_err(parent, "DRDY on INT1 not available.\n"); return -EINVAL;
dev_err(&indio_dev->dev,"DRDY on INT2 not available.\n");
dev_err(parent, "DRDY on INT2 not available.\n"); return -EINVAL;
dev_err(&indio_dev->dev, "DRDY on pdata not valid.\n");
dev_err(parent, "DRDY on pdata not valid.\n");if (pdata->open_drain) { if (!sdata->sensor_settings->drdy_irq.int1.addr_od && !sdata->sensor_settings->drdy_irq.int2.addr_od)
dev_err(&indio_dev->dev,
dev_err(parent, "open drain requested but unsupported.\n");@@ -336,6 +335,7 @@ EXPORT_SYMBOL_NS(st_sensors_dev_name_probe, "IIO_ST_SENSORS"); int st_sensors_init_sensor(struct iio_dev *indio_dev, struct st_sensors_platform_data *pdata) {
- struct device *parent = indio_dev->dev.parent; struct st_sensor_data *sdata = iio_priv(indio_dev); struct st_sensors_platform_data *of_pdata; int err = 0;
@@ -343,7 +343,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, mutex_init(&sdata->odr_lock); /* If OF/DT pdata exists, it will take precedence of anything else */
- of_pdata = st_sensors_dev_probe(indio_dev->dev.parent, pdata);
- of_pdata = st_sensors_dev_probe(parent, pdata); if (IS_ERR(of_pdata)) return PTR_ERR(of_pdata); if (of_pdata)
@@ -370,7 +370,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, if (err < 0) return err; } else
dev_info(&indio_dev->dev, "Full-scale not possible\n");
dev_info(parent, "Full-scale not possible\n");err = st_sensors_set_odr(indio_dev, sdata->odr); if (err < 0) @@ -405,7 +405,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev, mask = sdata->sensor_settings->drdy_irq.int2.mask_od; }
dev_info(&indio_dev->dev,
dev_info(parent, "set interrupt line to open drain mode on pin %d\n", sdata->drdy_int_pin);@@ -593,21 +593,20 @@ EXPORT_SYMBOL_NS(st_sensors_get_settings_index, "IIO_ST_SENSORS"); int st_sensors_verify_id(struct iio_dev *indio_dev) { struct st_sensor_data *sdata = iio_priv(indio_dev);
- struct device *parent = indio_dev->dev.parent; int wai, err;
if (sdata->sensor_settings->wai_addr) { err = regmap_read(sdata->regmap, sdata->sensor_settings->wai_addr, &wai); if (err < 0) {
dev_err(&indio_dev->dev,"failed to read Who-Am-I register.\n");return err;
return dev_err_probe(parent, err,"failed to read Who-Am-I register.\n");if (sdata->sensor_settings->wai != wai) {
dev_warn(&indio_dev->dev,"%s: WhoAmI mismatch (0x%x).\n",indio_dev->name, wai);
dev_warn(parent, "%s: WhoAmI mismatch (0x%x).\n",indio_dev->name, wai);diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c index 9d4bf822a15dfcdd6c2835f6b9d7698cd3cb0b08..32c3278968089699dff5329e943d92b151b55fdf 100644 --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c @@ -127,7 +127,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->trig = devm_iio_trigger_alloc(parent, "%s-trigger", indio_dev->name); if (sdata->trig == NULL) {
dev_err(&indio_dev->dev, "failed to allocate iio trigger.\n");
dev_err(parent, "failed to allocate iio trigger.\n");@@ -143,7 +143,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, case IRQF_TRIGGER_FALLING: case IRQF_TRIGGER_LOW: if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
dev_err(&indio_dev->dev,
dev_err(parent, "falling/low specified for IRQ but hardware supports only rising/high: will request rising/high\n"); if (irq_trig == IRQF_TRIGGER_FALLING) irq_trig = IRQF_TRIGGER_RISING;@@ -156,21 +156,21 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->sensor_settings->drdy_irq.mask_ihl, 1); if (err < 0) return err;
dev_info(&indio_dev->dev,
dev_info(parent, "interrupts on the falling edge or active low level\n");
dev_info(&indio_dev->dev,
dev_info(parent, "interrupts on the rising edge\n");
dev_info(&indio_dev->dev,
dev_info(parent, "interrupts active high level\n");
dev_err(&indio_dev->dev,
dev_err(parent, "unsupported IRQ trigger specified (%lx), enforce rising edge\n", irq_trig);@@ -179,7 +179,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, if (irq_trig == IRQF_TRIGGER_FALLING || irq_trig == IRQF_TRIGGER_RISING) { if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
dev_err(&indio_dev->dev,
dev_err(parent, "edge IRQ not supported w/o stat register.\n"); return -EOPNOTSUPP;@@ -214,13 +214,13 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, sdata->trig->name, sdata->trig); if (err) {
dev_err(&indio_dev->dev, "failed to request trigger IRQ.\n");
dev_err(parent, "failed to request trigger IRQ.\n");err = devm_iio_trigger_register(parent, sdata->trig); if (err < 0) {
dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
dev_err(parent, "failed to register iio trigger.\n");
base-commit: 7bac2c97af4078d7a627500c9bcdd5b033f97718 change-id: 20250522-st_iio_fix-1c58fdd4d420
Best regards,
On 5/22/25 19:12, Jonathan Cameron wrote:
On Thu, 22 May 2025 13:18:55 +0200 Maud Spierings via B4 Relay devnull+maudspierings.gocontroll.com@kernel.org wrote:
From: Maud Spierings maudspierings@gocontroll.com
Throughout the various probe functions &indio_dev->dev is used before it is initialized. This caused a kernel panic in st_sensors_power_enable when the call to devm_regulator_bulk_get_enable() fails and then calls dev_err_probe() with the uninitialized device.
Hi Maud,
Curious. Given the device_initialize() is in the allocation function it isn't immediately obvious that something needed might not have been initialized. Any idea what is being accessed in there that fails? (i.e. any idea if my shallow detective work found it ;)
It is indeed what you describe below, in the stack trace from my initial bug report it can indeed be seen that the panic happens in device_set_deferred_probe_reason(), __device_set_deferred_probe_reason() to be specific.
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/iio/industrialio-cor... in iio_device_alloc() is where device_initialize() is called.
Using the device in the iio_dev structure is almost certainly wrong but I'm surprised it crashes... So I had a quick dig.
The only path that isn't a simple print that I can spot is device_set_deferred_reason() That accesses dev->p (device private data) Which is initialized only in device_add().
I wonder if we should harden device_set_deferred_reason() against p == NULL, perhaps with a message strongly advising against using it with a device that hasn't been added?
Being in error paths this is the sort of subtle bug that rarely rears it's head :(
+CC Greg and Dragan for thoughts before anyone spins a patch.
This change is good either way. I'm just on wrong computer to pick it up right now.
I think I may need to send a v3, I didn't add the stable cc in my commit message above the SoB tag which the kernel test bot is informing me about.
On Thu, May 22, 2025 at 06:12:36PM +0100, Jonathan Cameron wrote:
On Thu, 22 May 2025 13:18:55 +0200 Maud Spierings via B4 Relay devnull+maudspierings.gocontroll.com@kernel.org wrote:
...
This change is good either way. I'm just on wrong computer to pick it up right now.
I have a few nit-picks, please take them into consideration as well.
On Thu, May 22, 2025 at 01:18:55PM +0200, Maud Spierings via B4 Relay wrote:
Throughout the various probe functions &indio_dev->dev is used before it is initialized. This caused a kernel panic in st_sensors_power_enable
st_sensors_power_enable()
when the call to devm_regulator_bulk_get_enable() fails and then calls dev_err_probe() with the uninitialized device.
This seems to only cause a panic with dev_err_probe(), dev_err,
dev_err()
dev_warn and dev_info don't seem to cause a panic, but are fixed
dev_warn() dev_info()
as well.
Signed-off-by: Maud Spierings maudspierings@gocontroll.com
When I search for general &indio_dev->dev usage, I see quite a lot more hits, but I am not sure if there are issues with those too.
This issue has existed for a long time it seems and therefore it is nearly impossible to find a proper fixes tag. I would love to see it at least backported to 6.12 as that is where I encountered it, and I believe the patch should apply without conflicts.
The investigation into this issue can be found in this thread [1]
Shouldn't it be moved to the commit message as Link tag?
...
return dev_err_probe(&indio_dev->dev, err,
return dev_err_probe(parent, err, "unable to enable supplies\n");
Now it can be put on one line (yes, only 1 or 2 characters longer than 80).
...
dev_info(&indio_dev->dev,
dev_info(parent, "interrupts on the rising edge\n");
Ditto.
...
dev_info(&indio_dev->dev,
dev_info(parent, "interrupts active high level\n");
Ditto.
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Jonathan Cameron -
kernel test robot -
Maud Spierings -
Maud Spierings via B4 Relay