[PATCH v2 0/4] platform/x86: int3472: A few cleanups

[PATCH v2 0/4] platform/x86: int3472: A few cleanups

[Andy Shevchenko]

A few cleanups to the driver. It includes an amendment to
dev_err_probe() to ignore 0 error code. The patches 1 and 2 are
dependent, while patches 2 and 3 may be applied in any order.

Assumed to go via PDx86 tree if no objections to the code.

v2:
- amended code example in the documentation (Ilpo, Greg)
- added note about -ENOMEM and 0 to the kernel-doc
- fixed kernel-doc warning (no "Return" section)
- added tags (Ilpo)

Andy Shevchenko (4):
  driver core: Ignore 0 in dev_err_probe()
  platform/x86: int3472: Simplify dev_err_probe() usage
  platform/x86: int3472: Use GPIO_LOOKUP() macro
  platform/x86: int3472: Use str_high_low()

 drivers/base/core.c                           | 13 +++++++++++--
 drivers/platform/x86/intel/int3472/discrete.c | 14 ++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

In the similar way, ignore 0 error code (AKA "success") in
dev_err_probe(). This helps to simplify a code such as

  if (ret < 0)
    return dev_err_probe(int3472->dev, ret, err_msg);

  return ret;

to

  return dev_err_probe(int3472->dev, ret, err_msg);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4bc8b88d697e..830a14084bf6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4987,13 +4987,14 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  * This helper implements common pattern present in probe functions for error
  * checking: print debug or error message depending if the error value is
  * -EPROBE_DEFER and propagate error upwards.
+ *
  * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
  * checked later by reading devices_deferred debugfs attribute.
  * It replaces code sequence::
  *
  * 	if (err != -EPROBE_DEFER)
  * 		dev_err(dev, ...);
- * 	else
+ * 	else if (err)
  * 		dev_dbg(dev, ...);
  * 	return err;
  *
@@ -5003,12 +5004,16 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * Using this helper in your probe function is totally fine even if @err is
  * known to never be -EPROBE_DEFER.
+ *
+ * NOTE: The message is not going to be printed or saved in cases when @err
+ * is equal to -ENOMEM or 0.
+ *
  * The benefit compared to a normal dev_err() is the standardized format
  * of the error code, it being emitted symbolically (i.e. you get "EAGAIN"
  * instead of "-35") and the fact that the error code is returned which allows
  * more compact error paths.
  *
- * Returns @err.
+ * Return: the value of @err.
  */
 int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 {
@@ -5032,6 +5037,10 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 		 */
 		break;
 
+	case 0:
+		/* Success, no need to issue an error message */
+		break;
+
 	default:
 		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
 		break;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Andy Shevchenko]

Since dev_err_probe() ignores success, i.e. 0. we may call
it unconditionally.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 34db7d0381fd..29df19379464 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -299,10 +299,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	int3472->ngpios++;
 	ACPI_FREE(obj);
 
-	if (ret < 0)
-		return dev_err_probe(int3472->dev, ret, err_msg);
-
-	return ret;
+	return dev_err_probe(int3472->dev, ret, err_msg);
 }
 
 static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 3/4] platform/x86: int3472: Use GPIO_LOOKUP() macro

[Andy Shevchenko]

Use GPIO_LOOKUP() macro which provides a compound literal
and can be used with dynamic data.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 29df19379464..a360901d03f0 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -69,11 +69,7 @@ static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 	if (!adev)
 		return -ENODEV;
 
-	table_entry->key = acpi_dev_name(adev);
-	table_entry->chip_hwnum = agpio->pin_table[0];
-	table_entry->con_id = func;
-	table_entry->idx = 0;
-	table_entry->flags = polarity;
+	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, polarity);
 
 	return 0;
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 4/4] platform/x86: int3472: Use str_high_low()

[Andy Shevchenko]

Use str_high_low() rather than open coding.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index a360901d03f0..0559295dfb27 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/string_choices.h>
 #include <linux/uuid.h>
 
 #include "common.h"
@@ -240,7 +241,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
 		agpio->resource_source.string_ptr, agpio->pin_table[0],
-		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
+		str_high_low(polarity == GPIO_ACTIVE_HIGH));
 
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Greg Kroah-Hartman]

On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> In the similar way, ignore 0 error code (AKA "success") in
> dev_err_probe(). This helps to simplify a code such as
> 
>   if (ret < 0)
>     return dev_err_probe(int3472->dev, ret, err_msg);
> 
>   return ret;
> 
> to
> 
>   return dev_err_probe(int3472->dev, ret, err_msg);
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Sat, Aug 24, 2024 at 11:08:53AM +0800, Greg Kroah-Hartman wrote:
> On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > In the similar way, ignore 0 error code (AKA "success") in
> > dev_err_probe(). This helps to simplify a code such as
> > 
> >   if (ret < 0)
> >     return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> >   return ret;
> > 
> > to
> > 
> >   return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thank you!

Hans, I think we all set to proceed with this. Do you have any comments?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Dan Carpenter]

On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> In the similar way, ignore 0 error code (AKA "success") in
> dev_err_probe(). This helps to simplify a code such as
> 
>   if (ret < 0)
>     return dev_err_probe(int3472->dev, ret, err_msg);
> 
>   return ret;
> 
> to
> 
>   return dev_err_probe(int3472->dev, ret, err_msg);
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is a terrible idea because currently Smatch is able to detect about one
bug per month where someone unintentionally passes the wrong error variable
to dev_err_probe().

I really hate this.

NAKed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Dan Carpenter]

Hi Andy,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/driver-core-Ignore-0-in-dev_err_probe/20240826-113856
base:   driver-core/driver-core-testing
patch link:    https://lore.kernel.org/r/20240822130722.1261891-3-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage
config: i386-randconfig-141-20240830 (https://download.01.org/0day-ci/archive/20240831/202408310807.sNPe5Mr2-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408310807.sNPe5Mr2-lkp@intel.com/

smatch warnings:
drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() error: uninitialized symbol 'err_msg'.
drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() warn: passing zero to 'dev_err_probe'

vim +/err_msg +292 drivers/platform/x86/intel/int3472/discrete.c

5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  270  		case INT3472_GPIO_TYPE_POWER_ENABLE:
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  271  			ret = skl_int3472_register_regulator(int3472, gpio);
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  272  			if (ret)
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  273  				err_msg = "Failed to map regulator to sensor\n";
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  274  
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  275  			break;
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  276  		default: /* Never reached */
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  277  			ret = -EINVAL;
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  278  			break;
53c5f7f6e7930f drivers/platform/x86/intel/int3472/discrete.c                   Hans de Goede   2023-10-04  279  		}
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  280  		break;
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  281  	default:
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  282  		dev_warn(int3472->dev,
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  283  			 "GPIO type 0x%02x unknown; the sensor may not work\n",
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  284  			 type);
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  285  		ret = 1;
                                                                                                                                ^^^^^^^^

5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  286  		break;
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  287  	}
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  288  
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  289  	int3472->ngpios++;
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  290  	ACPI_FREE(obj);
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03  291  
5de691bffe57fd drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c Daniel Scally   2021-06-03 @292  	return dev_err_probe(int3472->dev, ret, err_msg);

This is the success path.  "err_msg" is only set for the error path.  "ret" is 1
so it will use the uninitialized data.  But even if ret were zero, it's illegal
to pass uninitialized variables to functions which aren't inlined.

1) It's undefined behavior
2) Linus said so
3) It causes UBSan warnings at runtime.

regards,
dan carpenter

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Dan Carpenter]

On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > In the similar way, ignore 0 error code (AKA "success") in
> > dev_err_probe(). This helps to simplify a code such as
> > 
> >   if (ret < 0)
> >     return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> >   return ret;
> > 
> > to
> > 
> >   return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This is a terrible idea because currently Smatch is able to detect about one
> bug per month where someone unintentionally passes the wrong error variable
> to dev_err_probe().

Here are the stats since Jan 2023.  All these bugs are impossible to detect now.

2024-08-12 d3bde2243d42 iio: proximity: hx9023s: Fix error code in hx9023s_property_get()
2024-07-08 101e5c5c4e76 PCI: qcom: Fix missing error code in qcom_pcie_probe()
2024-02-22 debabbb1f272 iio: adc: ti-ads1298: Fix error code in probe()
2024-01-08 9c46e3a5232d iio: adc: ad7091r8: Fix error code in ad7091r8_gpio_setup()
2023-12-04 35ddd61cf023 platform/x86: x86-android-tablets: Fix an IS_ERR() vs NULL check in probe
2023-11-20 2d37b3649c41 hwrng: starfive - Fix dev_err_probe return error
2023-11-30 03219a3aa6c8 drm/imagination: Fix error codes in pvr_device_clk_init()
2023-09-07 4b2b39f9395b watchdog: marvell_gti_wdt: Fix error code in probe()
2023-08-24 8886e1b03669 ASoC: codecs: Fix error code in aw88261_i2c_probe()
2023-06-23 ad5152b85e8b leds: aw200xx: Fix error code in probe()
2023-07-18 86fe3e9f4c63 phy: starfive: fix error code in probe
2023-07-12 0b64150c3429 Input: bcm-keypad - correct dev_err_probe() error
2023-06-26 5fb2864cbd50 OPP: Properly propagate error along when failing to get icc_path
2023-06-19 02474880e8fd ASoC: max98388: fix error code in probe()
2023-05-25 cc5f2eb7ce11 mfd: tps6594: Fix an error code in probe()
2023-05-22 1ca04f21b204 remoteproc: stm32: Fix error code in stm32_rproc_parse_dt()
2023-05-15 46f5dd7439e3 fbdev: omapfb: panel-tpo-td043mtea1: fix error code in probe()
2023-05-13 3530167c6fe8 soc: qcom: icc-bwmon: fix incorrect error code passed to dev_err_probe()
2023-05-12 bc97139ff135 power: supply: rt9467: Fix passing zero to 'dev_err_probe'
2023-03-23 c4351b646123 iio: adc: ti-ads1100: fix error code in probe()
2023-02-27 65609d3206f7 i2c: gxp: fix an error code in probe
2023-02-15 76f5aaabce49 ASoC: soc-ac97: Return correct error codes

regards,
dan carpenter

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote:
> On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > > In the similar way, ignore 0 error code (AKA "success") in
> > > dev_err_probe(). This helps to simplify a code such as
> > > 
> > >   if (ret < 0)
> > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > 
> > >   return ret;
> > > 
> > > to
> > > 
> > >   return dev_err_probe(int3472->dev, ret, err_msg);
> > > 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > This is a terrible idea because currently Smatch is able to detect about one
> > bug per month where someone unintentionally passes the wrong error variable
> > to dev_err_probe().

How many cases you know where smatch is false positive about this?

I believe the number is only a few at most, which means that you may easily
detect this still with this change being applied, i.e. "anything that
terminates function flow with code 0, passed to dev_err_probe(), is
suspicious".

So, I don't see how it prevents you from detecting that.

> Here are the stats since Jan 2023.  All these bugs are impossible to detect now.
> 
> 2024-08-12 d3bde2243d42 iio: proximity: hx9023s: Fix error code in hx9023s_property_get()

Just looked at this one.
Whoever writes something like

if (ret || foo)
	return ...ret...;

should be punished immediately. I.o.w. has nothing to do with the API.

> 2024-07-08 101e5c5c4e76 PCI: qcom: Fix missing error code in qcom_pcie_probe()
> 2024-02-22 debabbb1f272 iio: adc: ti-ads1298: Fix error code in probe()
> 2024-01-08 9c46e3a5232d iio: adc: ad7091r8: Fix error code in ad7091r8_gpio_setup()
> 2023-12-04 35ddd61cf023 platform/x86: x86-android-tablets: Fix an IS_ERR() vs NULL check in probe
> 2023-11-20 2d37b3649c41 hwrng: starfive - Fix dev_err_probe return error
> 2023-11-30 03219a3aa6c8 drm/imagination: Fix error codes in pvr_device_clk_init()
> 2023-09-07 4b2b39f9395b watchdog: marvell_gti_wdt: Fix error code in probe()
> 2023-08-24 8886e1b03669 ASoC: codecs: Fix error code in aw88261_i2c_probe()
> 2023-06-23 ad5152b85e8b leds: aw200xx: Fix error code in probe()
> 2023-07-18 86fe3e9f4c63 phy: starfive: fix error code in probe
> 2023-07-12 0b64150c3429 Input: bcm-keypad - correct dev_err_probe() error
> 2023-06-26 5fb2864cbd50 OPP: Properly propagate error along when failing to get icc_path
> 2023-06-19 02474880e8fd ASoC: max98388: fix error code in probe()
> 2023-05-25 cc5f2eb7ce11 mfd: tps6594: Fix an error code in probe()
> 2023-05-22 1ca04f21b204 remoteproc: stm32: Fix error code in stm32_rproc_parse_dt()
> 2023-05-15 46f5dd7439e3 fbdev: omapfb: panel-tpo-td043mtea1: fix error code in probe()
> 2023-05-13 3530167c6fe8 soc: qcom: icc-bwmon: fix incorrect error code passed to dev_err_probe()
> 2023-05-12 bc97139ff135 power: supply: rt9467: Fix passing zero to 'dev_err_probe'
> 2023-03-23 c4351b646123 iio: adc: ti-ads1100: fix error code in probe()
> 2023-02-27 65609d3206f7 i2c: gxp: fix an error code in probe
> 2023-02-15 76f5aaabce49 ASoC: soc-ac97: Return correct error codes

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Andy Shevchenko]

On Sat, Aug 31, 2024 at 11:31:53AM +0300, Dan Carpenter wrote:
> Hi Andy,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/driver-core-Ignore-0-in-dev_err_probe/20240826-113856
> base:   driver-core/driver-core-testing
> patch link:    https://lore.kernel.org/r/20240822130722.1261891-3-andriy.shevchenko%40linux.intel.com
> patch subject: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage
> config: i386-randconfig-141-20240830 (https://download.01.org/0day-ci/archive/20240831/202408310807.sNPe5Mr2-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202408310807.sNPe5Mr2-lkp@intel.com/
> 
> smatch warnings:
> drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() error: uninitialized symbol 'err_msg'.
> drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() warn: passing zero to 'dev_err_probe'

Okay, I might agree on the err_msg, which is good to have to be passed anyway.
In such a case it might be good to have a dev_dbg() in the dev_err_probe() to
say that it is likely a bug in the code.

Would you accept that approach?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] platform/x86: int3472: A few cleanups

[Andy Shevchenko]

On Thu, Aug 22, 2024 at 04:05:37PM +0300, Andy Shevchenko wrote:
> A few cleanups to the driver. It includes an amendment to
> dev_err_probe() to ignore 0 error code. The patches 1 and 2 are
> dependent, while patches 2 and 3 may be applied in any order.
> 
> Assumed to go via PDx86 tree if no objections to the code.

Ilpo, Hans, the patches 3 and 4 are independent from 1&2 and may be applied
separately, if you have no objections.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Dan Carpenter]

On Mon, Sep 02, 2024 at 01:16:17PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote:
> > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > > > In the similar way, ignore 0 error code (AKA "success") in
> > > > dev_err_probe(). This helps to simplify a code such as
> > > > 
> > > >   if (ret < 0)
> > > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > > 
> > > >   return ret;
> > > > 
> > > > to
> > > > 
> > > >   return dev_err_probe(int3472->dev, ret, err_msg);
> > > > 
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > This is a terrible idea because currently Smatch is able to detect about one
> > > bug per month where someone unintentionally passes the wrong error variable
> > > to dev_err_probe().
> 
> How many cases you know where smatch is false positive about this?
> 

This check has a very low false positive rate.  There is only one potential
false positive in the current linux-next.  Let me add Baolin Wang to get his
take on this.  I never mentioned reported this warning because the code was old
when I wrote the check.

drivers/spi/spi-sprd-adi.c
   550          ret = of_hwspin_lock_get_id(np, 0);
   551          if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) {

Is it possible for the CONFIG_ to not be enabled but ret is zero?

   552                  sadi->hwlock =
   553                          devm_hwspin_lock_request_specific(&pdev->dev, ret);
   554                  if (!sadi->hwlock) {
   555                          ret = -ENXIO;
   556                          goto put_ctlr;
   557                  }
   558          } else {
   559                  switch (ret) {
   560                  case -ENOENT:
   561                          dev_info(&pdev->dev, "no hardware spinlock supplied\n");
   562                          break;
   563                  default:
   564                          dev_err_probe(&pdev->dev, ret, "failed to find hwlock id\n");
                                                          ^^^

   565                          goto put_ctlr;
   566                  }
   567          }


> I believe the number is only a few at most, which means that you may easily
> detect this still with this change being applied, i.e. "anything that
> terminates function flow with code 0, passed to dev_err_probe(), is
> suspicious".
> 

I think you mean the opposite of what you wrote?  That if we're passing zero to
dev_err_probe() and it's the last line in a function it's *NOT* suspicious?
Otherwise, I don't really understand the heuristic you're proposing.

regards,
dan carpenter

Re: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Dan Carpenter]

On Mon, Sep 02, 2024 at 01:17:51PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 31, 2024 at 11:31:53AM +0300, Dan Carpenter wrote:
> > Hi Andy,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/driver-core-Ignore-0-in-dev_err_probe/20240826-113856
> > base:   driver-core/driver-core-testing
> > patch link:    https://lore.kernel.org/r/20240822130722.1261891-3-andriy.shevchenko%40linux.intel.com
> > patch subject: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage
> > config: i386-randconfig-141-20240830 (https://download.01.org/0day-ci/archive/20240831/202408310807.sNPe5Mr2-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202408310807.sNPe5Mr2-lkp@intel.com/
> > 
> > smatch warnings:
> > drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() error: uninitialized symbol 'err_msg'.
> > drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() warn: passing zero to 'dev_err_probe'
> 
> Okay, I might agree on the err_msg, which is good to have to be passed anyway.
> In such a case it might be good to have a dev_dbg() in the dev_err_probe() to
> say that it is likely a bug in the code.
> 
> Would you accept that approach?

ret is 1.  We have to revert this patch.

regards,
dan carpenter

Re: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Hans de Goede]

Hi,

On 9/2/24 1:23 PM, Dan Carpenter wrote:
> On Mon, Sep 02, 2024 at 01:17:51PM +0300, Andy Shevchenko wrote:
>> On Sat, Aug 31, 2024 at 11:31:53AM +0300, Dan Carpenter wrote:
>>> Hi Andy,
>>>
>>> kernel test robot noticed the following build warnings:
>>>
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/driver-core-Ignore-0-in-dev_err_probe/20240826-113856
>>> base:   driver-core/driver-core-testing
>>> patch link:    https://lore.kernel.org/r/20240822130722.1261891-3-andriy.shevchenko%40linux.intel.com
>>> patch subject: [PATCH v2 2/4] platform/x86: int3472: Simplify dev_err_probe() usage
>>> config: i386-randconfig-141-20240830 (https://download.01.org/0day-ci/archive/20240831/202408310807.sNPe5Mr2-lkp@intel.com/config)
>>> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> | Closes: https://lore.kernel.org/r/202408310807.sNPe5Mr2-lkp@intel.com/
>>>
>>> smatch warnings:
>>> drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() error: uninitialized symbol 'err_msg'.
>>> drivers/platform/x86/intel/int3472/discrete.c:292 skl_int3472_handle_gpio_resources() warn: passing zero to 'dev_err_probe'
>>
>> Okay, I might agree on the err_msg, which is good to have to be passed anyway.
>> In such a case it might be good to have a dev_dbg() in the dev_err_probe() to
>> say that it is likely a bug in the code.
>>
>> Would you accept that approach?
> 
> ret is 1.  We have to revert this patch.

Note this was never applied, so no need to revert.

I'll apply patch 3/4 when I get around to it.

If some agreement is reached on patch 1/2 the new version of those can
be applied later.

Regards,

Hans

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Mon, Sep 02, 2024 at 02:10:41PM +0300, Dan Carpenter wrote:
> On Mon, Sep 02, 2024 at 01:16:17PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote:
> > > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> > > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > > > > In the similar way, ignore 0 error code (AKA "success") in
> > > > > dev_err_probe(). This helps to simplify a code such as
> > > > > 
> > > > >   if (ret < 0)
> > > > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > > > 
> > > > >   return ret;
> > > > > 
> > > > > to
> > > > > 
> > > > >   return dev_err_probe(int3472->dev, ret, err_msg);
> > > > > 
> > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > 
> > > > This is a terrible idea because currently Smatch is able to detect about one
> > > > bug per month where someone unintentionally passes the wrong error variable
> > > > to dev_err_probe().
> > 
> > How many cases you know where smatch is false positive about this?
> 
> This check has a very low false positive rate.

Yes, that's my expectation as well.

> There is only one potential
> false positive in the current linux-next.  Let me add Baolin Wang to get his
> take on this.  I never mentioned reported this warning because the code was old
> when I wrote the check.
> 
> drivers/spi/spi-sprd-adi.c
>    550          ret = of_hwspin_lock_get_id(np, 0);
>    551          if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) {
> 
> Is it possible for the CONFIG_ to not be enabled but ret is zero?
> 
>    552                  sadi->hwlock =
>    553                          devm_hwspin_lock_request_specific(&pdev->dev, ret);
>    554                  if (!sadi->hwlock) {
>    555                          ret = -ENXIO;
>    556                          goto put_ctlr;
>    557                  }
>    558          } else {
>    559                  switch (ret) {
>    560                  case -ENOENT:
>    561                          dev_info(&pdev->dev, "no hardware spinlock supplied\n");
>    562                          break;
>    563                  default:
>    564                          dev_err_probe(&pdev->dev, ret, "failed to find hwlock id\n");
>                                                           ^^^
> 
>    565                          goto put_ctlr;
>    566                  }
>    567          }
> 
> 
> > I believe the number is only a few at most, which means that you may easily
> > detect this still with this change being applied, i.e. "anything that
> > terminates function flow with code 0, passed to dev_err_probe(), is
> > suspicious".
> 
> I think you mean the opposite of what you wrote?  That if we're passing zero to
> dev_err_probe() and it's the last line in a function it's *NOT* suspicious?

Yes, sorry, I meant "...terminates function flow _in the middle_...".

> Otherwise, I don't really understand the heuristic you're proposing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Dan Carpenter]

On Mon, Sep 02, 2024 at 02:38:18PM +0300, Andy Shevchenko wrote:
> > > I believe the number is only a few at most, which means that you may easily
> > > detect this still with this change being applied, i.e. "anything that
> > > terminates function flow with code 0, passed to dev_err_probe(), is
> > > suspicious".
> > 
> > I think you mean the opposite of what you wrote?  That if we're passing zero to
> > dev_err_probe() and it's the last line in a function it's *NOT* suspicious?
> 
> Yes, sorry, I meant "...terminates function flow _in the middle_...".
> 

I don't think that works.  There are lots of success paths in the middle of
functions.  Smatch already has code to determine whether we should return an
error code or not.

1) Was there a function that returned NULL
2) Was there a function that returned an erorr code/error pointer
3) Was there a bounds check where x >= y?
4) Did we print an error code?
Etc..

I'd end up re-using this code.  This heuristic is more error prone, so there
would be false positives and missed bugs but I can't predict the future so I
don't know how bad it would be.  Looking through the warnings, we still would be
able to detected a number of these because Smatch warnings when you pass NULL to
IS_ERR() or PTR_ERR().

Probably the worse thing from a Smatch perspective is that now I can't just
assume that dev_err_probe() is always an error path.  So for example, we know
that *foo is always initialized on success so we can eliminate all the
"return dev_err_probe();" paths because those are failure path.

I'm never going to like this patch because I always want to make the error paths
more separate and more clear.

regards,
dan carpenter

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Johan Hovold]

On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > In the similar way, ignore 0 error code (AKA "success") in
> > dev_err_probe(). This helps to simplify a code such as
> > 
> >   if (ret < 0)
> >     return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> >   return ret;
> > 
> > to
> > 
> >   return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This is a terrible idea because currently Smatch is able to detect about one
> bug per month where someone unintentionally passes the wrong error variable
> to dev_err_probe().
> 
> I really hate this.
> 
> NAKed-by: Dan Carpenter <dan.carpenter@linaro.org>

Regardless of any issues this may cause for tooling, I fully agree that
this is a terrible idea that will only result in unreadable code.

	return dev_err_probe(dev, ret, "registration failed\n");

Except it did not fail...

NAK

Johan

Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Mon, Sep 02, 2024 at 05:58:18PM +0200, Johan Hovold wrote:
> On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > > In the similar way, ignore 0 error code (AKA "success") in
> > > dev_err_probe(). This helps to simplify a code such as
> > > 
> > >   if (ret < 0)
> > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > 
> > >   return ret;
> > > 
> > > to
> > > 
> > >   return dev_err_probe(int3472->dev, ret, err_msg);
> > > 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > This is a terrible idea because currently Smatch is able to detect about one
> > bug per month where someone unintentionally passes the wrong error variable
> > to dev_err_probe().
> > 
> > I really hate this.
> > 
> > NAKed-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Regardless of any issues this may cause for tooling, I fully agree that
> this is a terrible idea that will only result in unreadable code.
> 
> 	return dev_err_probe(dev, ret, "registration failed\n");
> 
> Except it did not fail...
> 
> NAK

Fair enough. Thank you, guys, for reviewing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] platform/x86: int3472: A few cleanups

[Hans de Goede]

Hi,

On 9/2/24 12:19 PM, Andy Shevchenko wrote:
> On Thu, Aug 22, 2024 at 04:05:37PM +0300, Andy Shevchenko wrote:
>> A few cleanups to the driver. It includes an amendment to
>> dev_err_probe() to ignore 0 error code. The patches 1 and 2 are
>> dependent, while patches 2 and 3 may be applied in any order.
>>
>> Assumed to go via PDx86 tree if no objections to the code.
> 
> Ilpo, Hans, the patches 3 and 4 are independent from 1&2 and may be applied
> separately, if you have no objections.

I've applied patches 3 and 4 to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
now.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans