[PATCH] Remove error prints for devm_add_action_or_reset()

[PATCH] Remove error prints for devm_add_action_or_reset()

[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 <waqar.hameed@axis.com>
---
This patch was inspired from a discussion from another patch:

  https://lore.kernel.org/all/pndqzz07v0j.fsf@axis.com/

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(-)

diff --git a/drivers/counter/ti-ecap-capture.c b/drivers/counter/ti-ecap-capture.c
index 3faaf7f60539..114f2d33f193 100644
--- a/drivers/counter/ti-ecap-capture.c
+++ b/drivers/counter/ti-ecap-capture.c
@@ -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)
diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index 0d17985a5fdc..172e7a6283dc 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -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;
diff --git a/drivers/i2c/muxes/i2c-mux-mule.c b/drivers/i2c/muxes/i2c-mux-mule.c
index 284ff4afeeac..617ca058d2c9 100644
--- a/drivers/i2c/muxes/i2c-mux-mule.c
+++ b/drivers/i2c/muxes/i2c-mux-mule.c
@@ -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) {
diff --git a/drivers/iio/accel/msa311.c b/drivers/iio/accel/msa311.c
index c31c53abc3d0..f18b06832c0a 100644
--- a/drivers/iio/accel/msa311.c
+++ b/drivers/iio/accel/msa311.c
@@ -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)
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index 6cf790ff3eb5..ff778a44323f 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -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)
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 92596f15e797..fd86470bd838 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -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");
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index b3e6bd2a55d7..a7f67d8ee9e0 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -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,
diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
index 3343b54e8e44..fe9e3ece3fda 100644
--- a/drivers/iio/adc/mt6577_auxadc.c
+++ b/drivers/iio/adc/mt6577_auxadc.c
@@ -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)
diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index 72aa4ca2e5a4..35433250b008 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -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);
 
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 325e3747a134..02d7bf137f2e 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -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)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
index d280c949cf47..bf23b7beb5f8 100644
--- a/drivers/iio/adc/ti-ads1119.c
+++ b/drivers/iio/adc/ti-ads1119.c
@@ -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);
 }
diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
index b1f745f75dbe..46255c530cb4 100644
--- a/drivers/iio/adc/ti-ads7924.c
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -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,
diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index 47f1c7e9efa9..6665409a9a87 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -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);
 }
diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
index 6f301c067045..9871096cbab3 100644
--- a/drivers/iio/light/al3000a.c
+++ b/drivers/iio/light/al3000a.c
@@ -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)
diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
index e9b237de180a..7d2f3738b967 100644
--- a/drivers/iio/light/apds9306.c
+++ b/drivers/iio/light/apds9306.c
@@ -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)
diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
index 56ab5fe90ff9..613561dd4083 100644
--- a/drivers/iio/light/bh1745.c
+++ b/drivers/iio/light/bh1745.c
@@ -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;
 }
diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
index ba4eb82d9bc2..95167273bb90 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -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);
 }
diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
index f4085020e03e..e2f42d87760a 100644
--- a/drivers/iio/light/opt4060.c
+++ b/drivers/iio/light/opt4060.c
@@ -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)
diff --git a/drivers/iio/magnetometer/als31300.c b/drivers/iio/magnetometer/als31300.c
index f72af829715f..928e1378304c 100644
--- a/drivers/iio/magnetometer/als31300.c
+++ b/drivers/iio/magnetometer/als31300.c
@@ -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;
diff --git a/drivers/iio/magnetometer/tmag5273.c b/drivers/iio/magnetometer/tmag5273.c
index 2ca5c26f0091..bdb656b031b1 100644
--- a/drivers/iio/magnetometer/tmag5273.c
+++ b/drivers/iio/magnetometer/tmag5273.c
@@ -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)
diff --git a/drivers/iio/proximity/vl53l0x-i2c.c b/drivers/iio/proximity/vl53l0x-i2c.c
index ef4aa7b2835e..b92fbd27755b 100644
--- a/drivers/iio/proximity/vl53l0x-i2c.c
+++ b/drivers/iio/proximity/vl53l0x-i2c.c
@@ -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;
diff --git a/drivers/iio/temperature/mlx90635.c b/drivers/iio/temperature/mlx90635.c
index f7f88498ba0e..1175c7887ae1 100644
--- a/drivers/iio/temperature/mlx90635.c
+++ b/drivers/iio/temperature/mlx90635.c
@@ -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)
diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index df42fdf36ae3..4d000b5b3ae6 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c
@@ -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));
diff --git a/drivers/mmc/host/litex_mmc.c b/drivers/mmc/host/litex_mmc.c
index b338ccfa8f33..08764cb8aa3d 100644
--- a/drivers/mmc/host/litex_mmc.c
+++ b/drivers/mmc/host/litex_mmc.c
@@ -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;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
index b3dc1afeefd1..38fb81db48c2 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
@@ -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);
diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
index 917a76d584f0..28ba11e42823 100644
--- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
+++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
@@ -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;
diff --git a/drivers/power/supply/mt6370-charger.c b/drivers/power/supply/mt6370-charger.c
index 98579998b300..29510af4e595 100644
--- a/drivers/power/supply/mt6370-charger.c
+++ b/drivers/power/supply/mt6370-charger.c
@@ -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)
diff --git a/drivers/power/supply/rt9467-charger.c b/drivers/power/supply/rt9467-charger.c
index e9aba9ad393c..e2ff9c4609ef 100644
--- a/drivers/power/supply/rt9467-charger.c
+++ b/drivers/power/supply/rt9467-charger.c
@@ -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)
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 8c6bf3d49753..e90d37d4f956 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -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;
diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index e63c77e41823..1bdc8a51befb 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -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);
 }
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 50adfb8b335b..3be165080a15 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -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);
diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 83079c414b4f..67832790acad 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -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))
diff --git a/sound/soc/sof/imx/imx-common.c b/sound/soc/sof/imx/imx-common.c
index 62bf707aa909..0ea232a38533 100644
--- a/sound/soc/sof/imx/imx-common.c
+++ b/sound/soc/sof/imx/imx-common.c
@@ -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)
-- 
2.39.5

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[David Lechner]

On 7/1/25 10:03 AM, Waqar Hameed wrote:
> 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 <waqar.hameed@axis.com>
> ---
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.

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Waqar Hameed]

On Tue, Jul 01, 2025 at 10:16 -0500 David Lechner <dlechner@baylibre.com> wrote:

> On 7/1/25 10:03 AM, Waqar Hameed wrote:
>> 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 <waqar.hameed@axis.com>
>> ---
> 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.

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.)

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Geraldo Nascimento]

On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> 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

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Waqar Hameed]

On Tue, Jul 01, 2025 at 12:25 -0300 Geraldo Nascimento <geraldogabriel@gmail.com> wrote:

> [Some people who received this message don't often get email from geraldogabriel@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
>> 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 for the suggestion Geraldo! I will do that (as also answered
to David).

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Geraldo Nascimento]

On Tue, Jul 01, 2025 at 06:15:51PM +0200, Waqar Hameed wrote:
> On Tue, Jul 01, 2025 at 12:25 -0300 Geraldo Nascimento <geraldogabriel@gmail.com> wrote:
> 
> > [Some people who received this message don't often get email from geraldogabriel@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> >> 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 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

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

Hello,

On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
>  drivers/pwm/pwm-meson.c                          | 3 +--

Looking at this driver I tried the following:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3809baed42f3..58a2ab74f14c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index eb2094e43050..23ef250727f1 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -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. */
diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index ae696d10faff..abfa5152b5a7 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Jonathan Cameron]

On Tue, 1 Jul 2025 19:44:17 +0200
Uwe Kleine-König <ukleinek@kernel.org> wrote:

> Hello,
> 
> On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> >  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


> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3809baed42f3..58a2ab74f14c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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
> diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
> index eb2094e43050..23ef250727f1 100644
> --- a/include/linux/dev_printk.h
> +++ b/include/linux/dev_printk.h
> @@ -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. */
> diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
> index ae696d10faff..abfa5152b5a7 100644
> --- a/include/linux/device/devres.h
> +++ b/include/linux/device/devres.h
> @@ -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

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

Hello Jonathan,

On Tue, Jul 01, 2025 at 06:55:19PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Jul 2025 19:44:17 +0200
> Uwe Kleine-König <ukleinek@kernel.org> wrote:
> 
> > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> > >  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)
> 	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).

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Jonathan Cameron]

On Wed, 2 Jul 2025 08:54:48 +0200
Uwe Kleine-König <ukleinek@kernel.org> wrote:

> Hello Jonathan,
> 
> On Tue, Jul 01, 2025 at 06:55:19PM +0100, Jonathan Cameron wrote:
> > On Tue, 1 Jul 2025 19:44:17 +0200
> > Uwe Kleine-König <ukleinek@kernel.org> wrote:
> >   
> > > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:  
> > > >  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)
> > 	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.

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.


> 
> 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).
> 
> > 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
> 

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Andy Shevchenko]

On Tue, Jul 1, 2025 at 8:44 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:

...

> 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.

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.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

Hello Andy,

On Tue, Jul 01, 2025 at 08:57:02PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 1, 2025 at 8:44 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> 
> ...
> 
> > 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.
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 08:10:28AM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 01, 2025 at 08:57:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 1, 2025 at 8:44 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> > > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:

...

> > > 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.
> > 
> > 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...

> 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.

> 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.

-- 
With Best Regards,
Andy Shevchenko