Remove error prints for devm_add_action_or_reset()

PATCH Superseded Waqar Hameed 8 participants 13 messages Jul 1, 2025 – Jul 2, 2025

Subsystems (42)

Networking Network drivers SCSI USB +38 more subsystems

lore

Share & tools

lore

Waqar Hameed

author

2025-07-01 15:03 UTC #0 lore raw

PATCH 1/1 Remove error prints for devm_add_action_or_reset()

When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.

Signed-off-by Waqar Hameed <[email protected]>

---
This patch was inspired from a discussion from another patch:

https://lore.kernel.org/all/[email protected]/

The calls were identified with

rgrep -A10 "devm_add_action_or_reset(" | grep -B10 dev_err_probe

The resulting output was then parsed semi-manually and the changes created with the help of Emacs macros.

 drivers/counter/ti-ecap-capture.c                | 2 +-
 drivers/gpio/gpio-twl4030.c                      | 4 +---
 drivers/i2c/muxes/i2c-mux-mule.c                 | 3 +--
 drivers/iio/accel/msa311.c                       | 2 +-
 drivers/iio/adc/ad4130.c                         | 3 +--
 drivers/iio/adc/ad7124.c                         | 2 +-
 drivers/iio/adc/ad7173.c                         | 3 +--
 drivers/iio/adc/mt6577_auxadc.c                  | 3 +--
 drivers/iio/adc/pac1921.c                        | 3 +--
 drivers/iio/adc/rockchip_saradc.c                | 3 +--
 drivers/iio/adc/ti-ads1119.c                     | 3 +--
 drivers/iio/adc/ti-ads7924.c                     | 6 ++----
 drivers/iio/frequency/adf4350.c                  | 3 +--
 drivers/iio/light/al3000a.c                      | 2 +-
 drivers/iio/light/apds9306.c                     | 2 +-
 drivers/iio/light/bh1745.c                       | 3 +--
 drivers/iio/light/opt4001.c                      | 3 +--
 drivers/iio/light/opt4060.c                      | 3 +--
 drivers/iio/magnetometer/als31300.c              | 2 +-
 drivers/iio/magnetometer/tmag5273.c              | 2 +-
 drivers/iio/proximity/vl53l0x-i2c.c              | 3 +--
 drivers/iio/temperature/mlx90635.c               | 6 ++----
 drivers/input/touchscreen/zforce_ts.c            | 3 +--
 drivers/mmc/host/litex_mmc.c                     | 3 +--
 drivers/net/ethernet/freescale/enetc/enetc4_pf.c | 3 +--
 drivers/phy/samsung/phy-exynos5-usbdrd.c         | 3 +--
 drivers/power/supply/mt6370-charger.c            | 4 ++--
 drivers/power/supply/rt9467-charger.c            | 8 ++++----
 drivers/pwm/pwm-meson.c                          | 3 +--
 drivers/spi/spi-nxp-fspi.c                       | 2 +-
 drivers/ufs/core/ufshcd.c                        | 3 +--
 drivers/usb/misc/qcom_eud.c                      | 3 +--
 sound/soc/sof/imx/imx-common.c                   | 2 +-
 33 files changed, 39 insertions(+), 64 deletions(-)

drivers/counter/ti-ecap-capture.c +1 -1

index 3faaf7f60539..114f2d33f193 100644
@@ -528,7 +528,7 @@ static int ecap_cnt_probe(struct platform_device *pdev)
 	/* Register a cleanup callback to care for disabling PM */
 	ret = devm_add_action_or_reset(dev, ecap_cnt_pm_disable, dev);
 	if (ret)
-		return dev_err_probe(dev, ret, "failed to add pm disable action\n");
+		return ret;
 
 	ret = devm_counter_add(dev, counter_dev);
 	if (ret)

drivers/gpio/gpio-twl4030.c +1 -3

index 0d17985a5fdc..172e7a6283dc 100644
@@ -596,9 +596,7 @@ static int gpio_twl4030_probe(struct platform_device *pdev)
 
 		ret = devm_add_action_or_reset(&pdev->dev, gpio_twl4030_power_off_action, d);
 		if (ret)
-			return dev_err_probe(&pdev->dev, ret,
-					     "failed to install power off handler\n");
-
+			return ret;
 	}
 
 	return 0;

drivers/i2c/muxes/i2c-mux-mule.c +1 -2

index 284ff4afeeac..617ca058d2c9 100644
@@ -91,8 +91,7 @@ static int mule_i2c_mux_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(mux_dev, mule_i2c_mux_remove, muxc);
 	if (ret)
-		return dev_err_probe(mux_dev, ret,
-				     "Failed to register mux remove\n");
+		return ret;
 
 	/* Create device adapters */
 	for_each_child_of_node(mux_dev->of_node, dev) {

drivers/iio/accel/msa311.c +1 -1

index c31c53abc3d0..f18b06832c0a 100644
@@ -1197,7 +1197,7 @@ static int msa311_probe(struct i2c_client *i2c)
 	 */
 	err = devm_add_action_or_reset(dev, msa311_powerdown, msa311);
 	if (err)
-		return dev_err_probe(dev, err, "can't add powerdown action\n");
+		return err;
 
 	err = pm_runtime_set_active(dev);
 	if (err)

drivers/iio/adc/ad4130.c +1 -2

index 6cf790ff3eb5..ff778a44323f 100644
@@ -2035,8 +2035,7 @@ static int ad4130_probe(struct spi_device *spi)
 
 	ret = devm_add_action_or_reset(dev, ad4130_disable_regulators, st);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to add regulators disable action\n");
+		return ret;
 
 	ret = ad4130_soft_reset(st);
 	if (ret)

drivers/iio/adc/ad7124.c +1 -1

index 92596f15e797..fd86470bd838 100644
@@ -1306,7 +1306,7 @@ static int ad7124_probe(struct spi_device *spi)
 		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
 					       st->vref[i]);
 		if (ret)
-			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
+			return ret;
 	}
 
 	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");

drivers/iio/adc/ad7173.c +1 -2

index b3e6bd2a55d7..a7f67d8ee9e0 100644
@@ -1707,8 +1707,7 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 
 	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to add regulators disable action\n");
+		return ret;
 
 	ret = device_property_match_property_string(dev, "clock-names",
 						    ad7173_clk_sel,

drivers/iio/adc/mt6577_auxadc.c +1 -2

index 3343b54e8e44..fe9e3ece3fda 100644
@@ -297,8 +297,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(&pdev->dev, mt6577_power_off, adc_dev);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "Failed to add action to managed power off\n");
+		return ret;
 
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret < 0)

drivers/iio/adc/pac1921.c +1 -2

index 72aa4ca2e5a4..35433250b008 100644
@@ -1279,8 +1279,7 @@ static int pac1921_probe(struct i2c_client *client)
 	ret = devm_add_action_or_reset(dev, pac1921_regulator_disable,
 				       priv->vdd);
 	if (ret)
-		return dev_err_probe(dev, ret,
-			"Cannot add action for vdd regulator disposal\n");
+		return ret;
 
 	msleep(PAC1921_POWERUP_TIME_MS);

drivers/iio/adc/rockchip_saradc.c +1 -2

index 325e3747a134..02d7bf137f2e 100644
@@ -529,8 +529,7 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
 	ret = devm_add_action_or_reset(&pdev->dev,
 				       rockchip_saradc_regulator_disable, info);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "failed to register devm action\n");
+		return ret;
 
 	ret = regulator_get_voltage(info->vref);
 	if (ret < 0)

drivers/iio/adc/ti-ads1119.c +1 -2

index d280c949cf47..bf23b7beb5f8 100644
@@ -780,8 +780,7 @@ static int ads1119_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(dev, ads1119_powerdown, st);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to add powerdown action\n");
+		return ret;
 
 	return devm_iio_device_register(dev, indio_dev);
 }

drivers/iio/adc/ti-ads7924.c +2 -4

index b1f745f75dbe..46255c530cb4 100644
@@ -399,8 +399,7 @@ static int ads7924_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(dev, ads7924_reg_disable, data->vref_reg);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to add regulator disable action\n");
+		return ret;
 
 	ret = ads7924_reset(indio_dev);
 	if (ret < 0)

@@ -414,8 +413,7 @@ static int ads7924_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(dev, ads7924_set_idle_mode, data);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to add idle mode action\n");
+		return ret;
 
 	/* Use minimum signal acquire time. */
 	ret = regmap_update_bits(data->regmap, ADS7924_ACQCONFIG_REG,

drivers/iio/frequency/adf4350.c +1 -2

index 47f1c7e9efa9..6665409a9a87 100644
@@ -673,8 +673,7 @@ static int adf4350_probe(struct spi_device *spi)
 
 	ret = devm_add_action_or_reset(&spi->dev, adf4350_power_down, indio_dev);
 	if (ret)
-		return dev_err_probe(&spi->dev, ret,
-				     "Failed to add action to managed power down\n");
+		return ret;
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }

drivers/iio/light/al3000a.c +1 -1

index 6f301c067045..9871096cbab3 100644
@@ -94,7 +94,7 @@ static int al3000a_init(struct al3000a_data *data)
 
 	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "failed to add action\n");
+		return ret;
 
 	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
 	if (ret)

drivers/iio/light/apds9306.c +1 -1

index e9b237de180a..7d2f3738b967 100644
@@ -1305,7 +1305,7 @@ static int apds9306_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "failed to add action or reset\n");
+		return ret;
 
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)

drivers/iio/light/bh1745.c +1 -2

index 56ab5fe90ff9..613561dd4083 100644
@@ -816,8 +816,7 @@ static int bh1745_init(struct bh1745_data *data)
 
 	ret = devm_add_action_or_reset(dev, bh1745_power_off, data);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to add action or reset\n");
+		return ret;
 
 	return 0;
 }

drivers/iio/light/opt4001.c +1 -2

index ba4eb82d9bc2..95167273bb90 100644
@@ -428,8 +428,7 @@ static int opt4001_probe(struct i2c_client *client)
 					opt4001_chip_off_action,
 					chip);
 	if (ret < 0)
-		return dev_err_probe(&client->dev, ret,
-				     "Failed to setup power off action\n");
+		return ret;
 
 	return devm_iio_device_register(&client->dev, indio_dev);
 }

drivers/iio/light/opt4060.c +1 -2

index f4085020e03e..e2f42d87760a 100644
@@ -1301,8 +1301,7 @@ static int opt4060_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);
 	if (ret < 0)
-		return dev_err_probe(dev, ret,
-				     "Failed to setup power off action\n");
+		return ret;
 
 	ret = opt4060_setup_buffer(chip, indio_dev);
 	if (ret)

drivers/iio/magnetometer/als31300.c +1 -1

index f72af829715f..928e1378304c 100644
@@ -373,7 +373,7 @@ static int als31300_probe(struct i2c_client *i2c)
 
 	ret = devm_add_action_or_reset(dev, als31300_power_down, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "failed to add powerdown action\n");
+		return ret;
 
 	indio_dev->info = &als31300_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;

drivers/iio/magnetometer/tmag5273.c +1 -1

index 2ca5c26f0091..bdb656b031b1 100644
@@ -642,7 +642,7 @@ static int tmag5273_probe(struct i2c_client *i2c)
 	 */
 	ret = devm_add_action_or_reset(dev, tmag5273_power_down, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "failed to add powerdown action\n");
+		return ret;
 
 	ret = pm_runtime_set_active(dev);
 	if (ret < 0)

drivers/iio/proximity/vl53l0x-i2c.c +1 -2

index ef4aa7b2835e..b92fbd27755b 100644
@@ -352,8 +352,7 @@ static int vl53l0x_probe(struct i2c_client *client)
 
 	error = devm_add_action_or_reset(&client->dev, vl53l0x_power_off, data);
 	if (error)
-		return dev_err_probe(&client->dev, error,
-				     "Failed to install poweroff action\n");
+		return ret;
 
 	indio_dev->name = "vl53l0x";
 	indio_dev->info = &vl53l0x_info;

drivers/iio/temperature/mlx90635.c +2 -4

index f7f88498ba0e..1175c7887ae1 100644
@@ -977,8 +977,7 @@ static int mlx90635_probe(struct i2c_client *client)
 	ret = devm_add_action_or_reset(&client->dev, mlx90635_disable_regulator,
 				       mlx90635);
 	if (ret < 0)
-		return dev_err_probe(&client->dev, ret,
-				     "failed to setup regulator cleanup action\n");
+		return ret;
 
 	ret = mlx90635_wakeup(mlx90635);
 	if (ret < 0)

@@ -986,8 +985,7 @@ static int mlx90635_probe(struct i2c_client *client)
 
 	ret = devm_add_action_or_reset(&client->dev, mlx90635_sleep, mlx90635);
 	if (ret < 0)
-		return dev_err_probe(&client->dev, ret,
-				     "failed to setup low power cleanup\n");
+		return ret;
 
 	ret = regmap_read(mlx90635->regmap_ee, MLX90635_EE_VERSION, &dsp_version);
 	if (ret < 0)

drivers/input/touchscreen/zforce_ts.c +1 -2

index df42fdf36ae3..4d000b5b3ae6 100644
@@ -739,8 +739,7 @@ static int zforce_probe(struct i2c_client *client)
 
 	error = devm_add_action_or_reset(&client->dev, zforce_reset, ts);
 	if (error)
-		return dev_err_probe(&client->dev, error,
-				     "failed to register reset action\n");
+		return error;
 
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));

drivers/mmc/host/litex_mmc.c +1 -2

index b338ccfa8f33..08764cb8aa3d 100644
@@ -531,8 +531,7 @@ static int litex_mmc_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev, litex_mmc_free_host_wrapper, mmc);
 	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Can't register mmc_free_host action\n");
+		return ret;
 
 	host = mmc_priv(mmc);
 	host->mmc = mmc;

drivers/net/ethernet/freescale/enetc/enetc4_pf.c +1 -2

index b3dc1afeefd1..38fb81db48c2 100644
@@ -1016,8 +1016,7 @@ static int enetc4_pf_probe(struct pci_dev *pdev,
 
 	err = devm_add_action_or_reset(dev, enetc4_pci_remove, pdev);
 	if (err)
-		return dev_err_probe(dev, err,
-				     "Add enetc4_pci_remove() action failed\n");
+		return err;
 
 	/* si is the private data. */
 	si = pci_get_drvdata(pdev);

drivers/phy/samsung/phy-exynos5-usbdrd.c +1 -2

index 917a76d584f0..28ba11e42823 100644
@@ -1859,8 +1859,7 @@ static int exynos5_usbdrd_setup_notifiers(struct exynos5_usbdrd_phy *phy_drd)
 					       exynos5_usbdrd_orien_switch_unregister,
 					       phy_drd);
 		if (ret)
-			return dev_err_probe(phy_drd->dev, ret,
-					     "Failed to register TypeC orientation devm action\n");
+			return ret;
 	}
 
 	return 0;

drivers/power/supply/mt6370-charger.c +2 -2

index 98579998b300..29510af4e595 100644
@@ -898,7 +898,7 @@ static int mt6370_chg_probe(struct platform_device *pdev)
 	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_attach_lock,
 				       &priv->attach_lock);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
+		return ret;
 
 	priv->attach = MT6370_ATTACH_STAT_DETACH;

@@ -909,7 +909,7 @@ static int mt6370_chg_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_wq, priv->wq);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init wq\n");
+		return ret;
 
 	ret = devm_work_autocancel(dev, &priv->bc12_work, mt6370_chg_bc12_work_func);
 	if (ret)

drivers/power/supply/rt9467-charger.c +4 -4

index e9aba9ad393c..e2ff9c4609ef 100644
@@ -1218,25 +1218,25 @@ static int rt9467_charger_probe(struct i2c_client *i2c)
 	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_adc_lock,
 				       &data->adc_lock);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init ADC lock\n");
+		return ret;
 
 	mutex_init(&data->attach_lock);
 	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_attach_lock,
 				       &data->attach_lock);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init attach lock\n");
+		return ret;
 
 	mutex_init(&data->ichg_ieoc_lock);
 	ret = devm_add_action_or_reset(dev, rt9467_chg_destroy_ichg_ieoc_lock,
 				       &data->ichg_ieoc_lock);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init ICHG/IEOC lock\n");
+		return ret;
 
 	init_completion(&data->aicl_done);
 	ret = devm_add_action_or_reset(dev, rt9467_chg_complete_aicl_done,
 				       &data->aicl_done);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init AICL done completion\n");
+		return ret;
 
 	ret = rt9467_do_charger_init(data);
 	if (ret)

drivers/pwm/pwm-meson.c +1 -2

index 8c6bf3d49753..e90d37d4f956 100644
@@ -520,8 +520,7 @@ static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
 		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
 					       meson->channels[i].clk);
 		if (ret)
-			return dev_err_probe(dev, ret,
-					     "Failed to add clk_put action\n");
+			return ret;
 	}
 
 	return 0;

drivers/spi/spi-nxp-fspi.c +1 -1

index e63c77e41823..1bdc8a51befb 100644
@@ -1283,7 +1283,7 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev, nxp_fspi_cleanup, f);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to register nxp_fspi_cleanup\n");
+		return ret;
 
 	return devm_spi_register_controller(&pdev->dev, ctlr);
 }

drivers/ufs/core/ufshcd.c +1 -2

index 50adfb8b335b..3be165080a15 100644
@@ -10476,8 +10476,7 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
 	err = devm_add_action_or_reset(dev, ufshcd_devres_release,
 				       host);
 	if (err)
-		return dev_err_probe(dev, err,
-				     "failed to add ufshcd dealloc action\n");
+		return err;
 
 	host->nr_maps = HCTX_TYPE_POLL + 1;
 	hba = shost_priv(host);

drivers/usb/misc/qcom_eud.c +1 -2

index 83079c414b4f..67832790acad 100644
@@ -193,8 +193,7 @@ static int eud_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(chip->dev, eud_role_switch_release, chip);
 	if (ret)
-		return dev_err_probe(chip->dev, ret,
-				"failed to add role switch release action\n");
+		return ret;
 
 	chip->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(chip->base))

sound/soc/sof/imx/imx-common.c +1 -1

index 62bf707aa909..0ea232a38533 100644
@@ -404,7 +404,7 @@ static int imx_probe(struct snd_sof_dev *sdev)
 				       imx_unregister_action,
 				       sdev);
 	if (ret)
-		return dev_err_probe(sdev->dev, ret, "failed to add devm action\n");
+		return ret;
 
 	common->ipc_handle = dev_get_drvdata(&common->ipc_dev->dev);
 	if (!common->ipc_handle)

Signature · 1 line

2.39.5

David Lechner

reviewer

2025-07-01 15:17 UTC #1 lore raw

Quoted 8 lines · Waqar Hameed

When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.
Signed-off-by Waqar Hameed <[email protected]>
---

I can't speak for all subsystems, but this would probably be acceptable in the iio subsystem.

However, I don't think anyone is going to accept a patch that touches all of these files at the same time across subsystems.

So I would suggest to split this up into one patch per driver and create one series per subsystem. This way, each subsystem isn't bothered by unrelated patches that they don't particularly need to care about. And note that some subsystems like net have additional expectations, e.g for the patch subject so that it gets picked up by automated tools, so be sure to check the docs for this.

Waqar Hameed

author

2025-07-01 16:14 UTC #2 lore raw

Quoted 15 lines · On Tue, Jul 01, 2025 at 10:16 -0500 David Lechner <[email protected]> wrote:

Quoted 8 lines · Waqar Hameed
When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.
Signed-off-by Waqar Hameed <[email protected]>
---
I can't speak for all subsystems, but this would probably be acceptable in the iio subsystem.
However, I don't think anyone is going to accept a patch that touches all of these files at the same time across subsystems.

Quoted 6 lines

So I would suggest to split this up into one patch per driver and create one series per subsystem. This way, each subsystem isn't bothered by unrelated patches that they don't particularly need to care about. And note that some subsystems like net have additional expectations, e.g for the patch subject so that it gets picked up by automated tools, so be sure to check the docs for this.

Thanks for the suggestion David! I will do that then.

(I was contemplating on doing that at first, but gambled on this, since I saw some other commits patches touching multiple files in different sub-systems.)

Geraldo Nascimento

2025-07-01 15:25 UTC #3 lore raw

In reply to Waqar Hameed

Quoted 5 lines · Waqar Hameed

When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.

Hi Waqar,

thank you for the patch. However I personally advise you to split the patches per-file and remember to then precede each individual patch subject with the proper subsystem and driver touched.

While this looks like a nit-pick, it really isn't, and my suggestion will make reviewing much more easier and you'll get your Reviewed-by's and Acked-by's much more smoothly.

The cover-letter should probably be preceded by "treewide" instead of a specific subsystem.

Thank you,
Geraldo Nascimento

Waqar Hameed

author

2025-07-01 16:15 UTC #4 lore raw

Quoted 15 lines · On Tue, Jul 01, 2025 at 12:25 -0300 Geraldo Nascimento…

[Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Quoted 5 lines · Waqar Hameed
When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.
Hi Waqar,
thank you for the patch. However I personally advise you to split the patches per-file and remember to then precede each individual patch subject with the proper subsystem and driver touched.

Quoted 6 lines

While this looks like a nit-pick, it really isn't, and my suggestion will make reviewing much more easier and you'll get your Reviewed-by's and Acked-by's much more smoothly.
The cover-letter should probably be preceded by "treewide" instead of a specific subsystem.

Thank you for the suggestion Geraldo! I will do that (as also answered to David).

Geraldo Nascimento

2025-07-01 16:25 UTC #5 lore raw

Quoted 21 lines · Waqar Hameed

Quoted 19 lines · On Tue, Jul 01, 2025 at 12:25 -0300 Geraldo Nascimento…
[Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Quoted 5 lines · Waqar Hameed
When devm_add_action_or_reset() fails, it is due to a failed memory allocation and will thus return -ENOMEM. dev_err_probe() doesn't do anything when error is -ENOMEM. Therefore, remove the useless call to dev_err_probe() when devm_add_action_or_reset() fails, and just return the value instead.
Hi Waqar,
thank you for the patch. However I personally advise you to split the patches per-file and remember to then precede each individual patch subject with the proper subsystem and driver touched.
While this looks like a nit-pick, it really isn't, and my suggestion will make reviewing much more easier and you'll get your Reviewed-by's and Acked-by's much more smoothly.

Quoted 5 lines

Quoted 2 lines
The cover-letter should probably be preceded by "treewide" instead of a specific subsystem.
Thank you for the suggestion Geraldo! I will do that (as also answered to David).

You're welcome Waqar! Note that David's suggestion is even smarter than
mine: instead of patch-bombing lots of maintainers with changes unrelated to their subsystems through a treewide change, he suggests you split the patch into one series per subsystem. This is indeed advisable.

Thanks,
Geraldo Nascimento

Uwe Kleine-König

maintainer

2025-07-01 17:44 UTC #6 lore raw

In reply to Waqar Hameed

Hello,

Quoted 1 line · Waqar Hameed

 drivers/pwm/pwm-meson.c                          | 3 +--

Looking at this driver I tried the following:

drivers/base/core.c +2 -2

index 3809baed42f3..58a2ab74f14c 100644
@@ -5062,7 +5062,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
  *
  * Returns @err.
  */
-int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 {
 	va_list vargs;

@@ -5075,7 +5075,7 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(dev_err_probe);
+EXPORT_SYMBOL_GPL(_dev_err_probe);
 
 /**
  * dev_warn_probe - probe error check and log helper

include/linux/dev_printk.h +7 -1

index eb2094e43050..23ef250727f1 100644
@@ -275,7 +275,13 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
-__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+__printf(3, 4) int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
+#define dev_err_probe(dev, err, ...) (								\
+	(__builtin_constant_p(err) && err == -ENOMEM) ? err : _dev_err_probe(dev, err, __VA_ARGS__)	\
+)
+
+
 __printf(3, 4) int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...);
 
 /* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */

include/linux/device/devres.h +4 -1

index ae696d10faff..abfa5152b5a7 100644
@@ -157,8 +157,11 @@ static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(
 	int ret;
 
 	ret = __devm_add_action(dev, action, data, name);
-	if (ret)
+	if (ret) {
+		if (ret != -ENOMEM)
+			__builtin_unreachable();
 		action(data);
+	}
 
 	return ret;
 }

With that

ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
meson->channels[i].clk);
if (ret)
return dev_err_probe(dev, ret,
"Failed to add clk_put action\n");

from drivers/pwm/pwm-meson.c is optimized to

        ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
                                       meson->channels[i].clk);
        if (ret)
                return ret;

I would prefer this approach, because a) there is no need to drop all dev_err_probe()s after devm_add_action_or_reset() and b) the dev_err_probe()s could stay for consistency in the error paths of a driver.

Best regards
Uwe

Jonathan Cameron

maintainer

2025-07-01 17:55 UTC #7 lore raw

Quoted 7 lines · On Tue, 1 Jul 2025 19:44:17 +0200 Uwe Kleine-König <[email protected]> wrote:

Hello,
Quoted 1 line · Waqar Hameed
 drivers/pwm/pwm-meson.c                          | 3 +--  
Looking at this driver I tried the following:

Hi Uqe,

I'm not sure what we actually want here.

My thought when suggesting removing instances of this particular combination wasn't saving on code size, but rather just general removal of pointless code that was getting cut and paste into new drivers and wasting a tiny bit of review bandwidth. I'd consider it bad practice to have patterns like

void *something = kmalloc();
if (!something)

	return dev_err_probe(dev, -ENOMEM, ..);

and my assumption was people would take a similar view with devm_add_action_or_reset().

It is a bit nuanced to have some cases where we think prints are reasonable and others where they aren't so I get your point about consistency.

The code size reduction is nice so I'd not be against it as an extra if the reduction across a kernel builds is significant and enough people want to keep these non printing prints.

Jonathan

Quoted 83 lines

drivers/base/core.c +2 -2

index 3809baed42f3..58a2ab74f14c 100644
@@ -5062,7 +5062,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
  *
  * Returns @err.
  */
-int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 {
 	va_list vargs;

@@ -5075,7 +5075,7 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(dev_err_probe);
+EXPORT_SYMBOL_GPL(_dev_err_probe);
 
 /**
  * dev_warn_probe - probe error check and log helper

include/linux/dev_printk.h +7 -1

index eb2094e43050..23ef250727f1 100644
@@ -275,7 +275,13 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
-__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+__printf(3, 4) int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
+#define dev_err_probe(dev, err, ...) (								\
+	(__builtin_constant_p(err) && err == -ENOMEM) ? err : _dev_err_probe(dev, err, __VA_ARGS__)	\
+)
+
+
 __printf(3, 4) int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...);
 
 /* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */

include/linux/device/devres.h +4 -1

index ae696d10faff..abfa5152b5a7 100644
@@ -157,8 +157,11 @@ static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(
 	int ret;
 
 	ret = __devm_add_action(dev, action, data, name);
-	if (ret)
+	if (ret) {
+		if (ret != -ENOMEM)
+			__builtin_unreachable();
 		action(data);
+	}
 
 	return ret;
 }

With that

ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
meson->channels[i].clk);
if (ret)
return dev_err_probe(dev, ret,
"Failed to add clk_put action\n");

from drivers/pwm/pwm-meson.c is optimized to

        ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
                                       meson->channels[i].clk);
        if (ret)
                return ret;

Best regards
Uwe

Uwe Kleine-König

maintainer

2025-07-02 06:54 UTC #8 lore raw

Hello Jonathan,

Quoted 18 lines · Jonathan Cameron

Quoted 4 lines · On Tue, 1 Jul 2025 19:44:17 +0200 Uwe Kleine-König <[email protected]> wrote:
Quoted 1 line · Waqar Hameed
 drivers/pwm/pwm-meson.c                          | 3 +--  
Looking at this driver I tried the following:
I'm not sure what we actually want here.
My thought when suggesting removing instances of this particular combination wasn't saving on code size, but rather just general removal of pointless code that was getting cut and paste into new drivers and wasting a tiny bit of review bandwidth. I'd consider it bad practice to have patterns like
void *something = kmalloc();
if (!something)

Quoted 8 lines

	return dev_err_probe(dev, -ENOMEM, ..);
and my assumption was people would take a similar view with devm_add_action_or_reset().
It is a bit nuanced to have some cases where we think prints are reasonable and others where they aren't so I get your point about consistency.

The problem I see is that there are two classes of functions: a) Those that require an error message and b) those that don't. Class b) consists of the functions that can only return success or -ENOMEM and the functions that emit an error message themselves. (And another problem I see is that for the latter the error message is usually non-optimal because the function doesn't know the all details of the request. See my reply to Andy for more details about that rant.)

IMHO what takes away the review bandwidth is that the reviewer has to check which class the failing function is part of. If this effort results in more driver authors not adding an error message after devm_add_action_or_reset() that's nice, but in two months I have forgotten the details of this discussion and I have to recheck if devm_add_action_or_reset() is part of a) or b) and so the burden is still on me.

So to give my answer on your question "What do we actually want here?":
Please let us get rid of the need to care for a) or b).

Quoted 3 lines

The code size reduction is nice so I'd not be against it as an extra if the reduction across a kernel builds is significant and enough people want to keep these non printing prints.

To complete implementing my wish all API functions would need to stop to emit an error message. Unfortunately that isn't without downsides because the result is that there are more error strings and so the kernel size is increased. So you have to weight if you prefer individual error messages and easier review/maintenance at the cost of a bigger binary size and more dev_err_probe() calls in drivers eating vertical space in your editor.

I know on which side I am, but I bet we won't find agreement about that in the kernel community ...

Best regards
Uwe

Jonathan Cameron

2025-07-02 09:58 UTC #9 lore raw

Quoted 38 lines · On Wed, 2 Jul 2025 08:54:48 +0200 Uwe Kleine-König <[email protected]> wrote:

Hello Jonathan,
Quoted 18 lines · Jonathan Cameron
Quoted 4 lines · On Tue, 1 Jul 2025 19:44:17 +0200 Uwe Kleine-König <[email protected]> wrote:
Quoted 1 line · Waqar Hameed
 drivers/pwm/pwm-meson.c                          | 3 +--    
Looking at this driver I tried the following:
I'm not sure what we actually want here.
My thought when suggesting removing instances of this particular combination wasn't saving on code size, but rather just general removal of pointless code that was getting cut and paste into new drivers and wasting a tiny bit of review bandwidth. I'd consider it bad practice to have patterns like
void *something = kmalloc();
if (!something)
Quoted 8 lines
	return dev_err_probe(dev, -ENOMEM, ..);
and my assumption was people would take a similar view with devm_add_action_or_reset().
It is a bit nuanced to have some cases where we think prints are reasonable and others where they aren't so I get your point about consistency.
The problem I see is that there are two classes of functions: a) Those that require an error message and b) those that don't. Class b) consists of the functions that can only return success or -ENOMEM and the functions that emit an error message themselves. (And another problem I see is that for the latter the error message is usually non-optimal because the function doesn't know the all details of the request. See my reply to Andy for more details about that rant.)

Quoted 7 lines

IMHO what takes away the review bandwidth is that the reviewer has to check which class the failing function is part of. If this effort results in more driver authors not adding an error message after devm_add_action_or_reset() that's nice, but in two months I have forgotten the details of this discussion and I have to recheck if devm_add_action_or_reset() is part of a) or b) and so the burden is still on me.

Maybe this is a job for checkpatch, at least for the common cases.

There is already a check for kmalloc etc.
https://elixir.bootlin.com/linux/v6.16-rc4/source/scripts/checkpatch.pl#L6442

+CC Joe (who wrote the allocation functions test years ago) and other checkpatch folk.

Quoted 24 lines

So to give my answer on your question "What do we actually want here?":
Please let us get rid of the need to care for a) or b).
Quoted 3 lines
The code size reduction is nice so I'd not be against it as an extra if the reduction across a kernel builds is significant and enough people want to keep these non printing prints.
To complete implementing my wish all API functions would need to stop to emit an error message. Unfortunately that isn't without downsides because the result is that there are more error strings and so the kernel size is increased. So you have to weight if you prefer individual error messages and easier review/maintenance at the cost of a bigger binary size and more dev_err_probe() calls in drivers eating vertical space in your editor.
I know on which side I am, but I bet we won't find agreement about that in the kernel community ...
Best regards
Uwe

Andy Shevchenko

2025-07-01 17:57 UTC #10 lore raw

In reply to Uwe Kleine-König

Quoted 1 line · Uwe Kleine-König

On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:

...

Quoted 15 lines

With that
ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
meson->channels[i].clk);
if (ret)
return dev_err_probe(dev, ret,
"Failed to add clk_put action\n");
from drivers/pwm/pwm-meson.c is optimized to
        ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
                                       meson->channels[i].clk);
        if (ret)
                return ret;

Quoted 6 lines

.
I would prefer this approach, because a) there is no need to drop all dev_err_probe()s after devm_add_action_or_reset() and b) the dev_err_probe()s could stay for consistency in the error paths of a driver.

Why do we need a dev_err_probe() after devm_add_action*()? I would expect that the original call (if needed) can spit out a message.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Uwe Kleine-König

maintainer

2025-07-02 06:10 UTC #11 lore raw

Hello Andy,

Quoted 22 lines · Andy Shevchenko

Quoted 1 line · Uwe Kleine-König
On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
...
Quoted 17 lines
With that
ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
meson->channels[i].clk);
if (ret)
return dev_err_probe(dev, ret,
"Failed to add clk_put action\n");
from drivers/pwm/pwm-meson.c is optimized to
        ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
                                       meson->channels[i].clk);
        if (ret)
                return ret;
.

Quoted 7 lines

Quoted 4 lines
I would prefer this approach, because a) there is no need to drop all dev_err_probe()s after devm_add_action_or_reset() and b) the dev_err_probe()s could stay for consistency in the error paths of a driver.
Why do we need a dev_err_probe() after devm_add_action*()? I would expect that the original call (if needed) can spit out a message.

I'm not a big fan of API functions that emit an error message. In general the caller knows better what went wrong (here:
devm_add_action_or_reset() doesn't know this to be about the clk_put action), so the error message can be more expressive.

Also in general an API function doesn't know if a failure is fatal or if the consumer handles the failure just well and if the call is part of a driver's .probe() so it's unclear if dev_err_probe() can/should be used. (I admit that the last two probably don't apply to devm_add_action_or_reset() but that's not a good enough reason to make this function special. Every special case is a maintanance burden.)

My two ¢,
Uwe

Andy Shevchenko

2025-07-02 07:54 UTC #12 lore raw

Quoted 3 lines · Uwe Kleine-König

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Uwe Kleine-König
On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:

...

Quoted 22 lines

Quoted 22 lines
Quoted 21 lines
With that
ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
meson->channels[i].clk);
if (ret)
return dev_err_probe(dev, ret,
"Failed to add clk_put action\n");
from drivers/pwm/pwm-meson.c is optimized to
        ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
                                       meson->channels[i].clk);
        if (ret)
                return ret;
.
I would prefer this approach, because a) there is no need to drop all dev_err_probe()s after devm_add_action_or_reset() and b) the dev_err_probe()s could stay for consistency in the error paths of a driver.

Quoted 4 lines

Quoted 2 lines
Why do we need a dev_err_probe() after devm_add_action*()? I would expect that the original call (if needed) can spit out a message.
I'm not a big fan of API functions that emit an error message.

We do have that in devm_ioremap*() family. Just saying...

Quoted 3 lines

In general the caller knows better what went wrong (here:
devm_add_action_or_reset() doesn't know this to be about the clk_put action), so the error message can be more expressive.

I'm not sure I was clear about my suggestion. What I argued is something like this

devm_foo_alloc()

{
	ret = foo_alloc();
	if (ret)
		return dev_err_probe();

	return devm_add_action_or_reset();
}

foo_alloc() in my example is left untouched.

Quoted 6 lines

Also in general an API function doesn't know if a failure is fatal or if the consumer handles the failure just well and if the call is part of a driver's .probe() so it's unclear if dev_err_probe() can/should be used. (I admit that the last two probably don't apply to devm_add_action_or_reset() but that's not a good enough reason to make this function special. Every special case is a maintanance burden.)

devm_*() are only supposed to be called in the probe phase. So using dev_err_probe() there (implementations) is natural thing to do, if required. And see above, we have such cases already.

Signature · 2 lines

With Best Regards,
Andy Shevchenko