[PATCH v1 1/2] driver core: add a wrapper to device probe log helper to return pointer

[PATCH v1 1/2] driver core: add a wrapper to device probe log helper to return pointer

[Andy Shevchenko]

Sometimes the ->probe() function can be split to the core and actual probe
parts. In such cases the core one may return a pointer to the allocated
resource, or error pointer in unsuccessful scenario. Allow that kind of
core function to use dev_err_probe_ptr(), so the following excerpt

	ret = bar(...);
	if (ret) {
		dev_err_probe(dev, ret, ...);
		return ERR_PTR(ret);
	}

can be replaced with

	ret = bar(...);
	if (ret)
		return dev_err_probe_ptr(dev, ret, ...);

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/device.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 93459724dcde..8650d3afbe7c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -14,6 +14,7 @@
 
 #include <linux/dev_printk.h>
 #include <linux/energy_model.h>
+#include <linux/err.h>
 #include <linux/ioport.h>
 #include <linux/kobject.h>
 #include <linux/klist.h>
@@ -982,6 +983,13 @@ void device_links_supplier_sync_state_resume(void);
 extern __printf(3, 4)
 int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
 
+/* As above, but returns error pointer */
+static inline __printf(3, 0)
+void *dev_err_probe_ptr(const struct device *dev, int err, const char *fmt, va_list args)
+{
+	return ERR_PTR(dev_err_probe(dev, err, fmt, args));
+}
+
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
-- 
2.34.1

[PATCH v1 2/2] serial: sh-sci: Switch to use dev_err_probe_ptr()

[Andy Shevchenko]

Instead of
	return ERR_PTR(dev_err_probe(...));
call
	return dev_err_probe_ptr(...);

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/sh-sci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index b610b27893a8..0fce09c13847 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3199,8 +3199,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 
 	rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(rstc))
-		return ERR_PTR(dev_err_probe(&pdev->dev, PTR_ERR(rstc),
-					     "failed to get reset ctrl\n"));
+		return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");
 
 	ret = reset_control_deassert(rstc);
 	if (ret) {
-- 
2.34.1

Re: [PATCH v1 2/2] serial: sh-sci: Switch to use dev_err_probe_ptr()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 14, 2022 at 3:33 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Instead of
>         return ERR_PTR(dev_err_probe(...));
> call
>         return dev_err_probe_ptr(...);
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3199,8 +3199,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>
>         rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
>         if (IS_ERR(rstc))
> -               return ERR_PTR(dev_err_probe(&pdev->dev, PTR_ERR(rstc),
> -                                            "failed to get reset ctrl\n"));
> +               return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");

I think the joined line is too long, so please keep it split.

drivers/tty/serial/sh-sci.c:3824:10: error: too few arguments to
function ‘dev_err_probe_ptr’

Indeed, dev_err_probe_ptr() is not a varargs function.
BTW, I do like the general idea.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 2/2] serial: sh-sci: Switch to use dev_err_probe_ptr()

[kernel test robot]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on usb/usb-testing linux/master linus/master v5.17-rc4 next-20220214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/driver-core-add-a-wrapper-to-device-probe-log-helper-to-return-pointer/20220214-223425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: hexagon-buildonly-randconfig-r001-20220214 (https://download.01.org/0day-ci/archive/20220215/202202150314.3Ybl4jns-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project ea071884b0cc7210b3cc5fe858f0e892a779a23b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/810910d324cc80b092207d043651de696d293cbd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/driver-core-add-a-wrapper-to-device-probe-log-helper-to-return-pointer/20220214-223425
        git checkout 810910d324cc80b092207d043651de696d293cbd
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/tty/serial/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/tty/serial/sh-sci.c:3205:83: error: too few arguments to function call, expected 4, have 3
                   return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");
                          ~~~~~~~~~~~~~~~~~                                                        ^
   include/linux/device.h:988:7: note: 'dev_err_probe_ptr' declared here
   void *dev_err_probe_ptr(const struct device *dev, int err, const char *fmt, va_list args)
         ^
   1 error generated.


vim +3205 drivers/tty/serial/sh-sci.c

  3187	
  3188	static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
  3189						  unsigned int *dev_id)
  3190	{
  3191		struct device_node *np = pdev->dev.of_node;
  3192		struct reset_control *rstc;
  3193		struct plat_sci_port *p;
  3194		struct sci_port *sp;
  3195		const void *data;
  3196		int id, ret;
  3197	
  3198		if (!IS_ENABLED(CONFIG_OF) || !np)
  3199			return ERR_PTR(-EINVAL);
  3200	
  3201		data = of_device_get_match_data(&pdev->dev);
  3202	
  3203		rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
  3204		if (IS_ERR(rstc))
> 3205			return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");
  3206	
  3207		ret = reset_control_deassert(rstc);
  3208		if (ret) {
  3209			dev_err(&pdev->dev, "failed to deassert reset %d\n", ret);
  3210			return ERR_PTR(ret);
  3211		}
  3212	
  3213		ret = devm_add_action_or_reset(&pdev->dev, sci_reset_control_assert, rstc);
  3214		if (ret) {
  3215			dev_err(&pdev->dev, "failed to register assert devm action, %d\n",
  3216				ret);
  3217			return ERR_PTR(ret);
  3218		}
  3219	
  3220		p = devm_kzalloc(&pdev->dev, sizeof(struct plat_sci_port), GFP_KERNEL);
  3221		if (!p)
  3222			return ERR_PTR(-ENOMEM);
  3223	
  3224		/* Get the line number from the aliases node. */
  3225		id = of_alias_get_id(np, "serial");
  3226		if (id < 0 && ~sci_ports_in_use)
  3227			id = ffz(sci_ports_in_use);
  3228		if (id < 0) {
  3229			dev_err(&pdev->dev, "failed to get alias id (%d)\n", id);
  3230			return ERR_PTR(-EINVAL);
  3231		}
  3232		if (id >= ARRAY_SIZE(sci_ports)) {
  3233			dev_err(&pdev->dev, "serial%d out of range\n", id);
  3234			return ERR_PTR(-EINVAL);
  3235		}
  3236	
  3237		sp = &sci_ports[id];
  3238		*dev_id = id;
  3239	
  3240		p->type = SCI_OF_TYPE(data);
  3241		p->regtype = SCI_OF_REGTYPE(data);
  3242	
  3243		sp->has_rtscts = of_property_read_bool(np, "uart-has-rtscts");
  3244	
  3245		return p;
  3246	}
  3247	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 2/2] serial: sh-sci: Switch to use dev_err_probe_ptr()

[kernel test robot]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on usb/usb-testing linux/master linus/master v5.17-rc4 next-20220214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/driver-core-add-a-wrapper-to-device-probe-log-helper-to-return-pointer/20220214-223425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220215/202202150928.sqyjprfB-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/810910d324cc80b092207d043651de696d293cbd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/driver-core-add-a-wrapper-to-device-probe-log-helper-to-return-pointer/20220214-223425
        git checkout 810910d324cc80b092207d043651de696d293cbd
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/tty/serial/sh-sci.c: In function 'sci_parse_dt':
>> drivers/tty/serial/sh-sci.c:3205:24: error: too few arguments to function 'dev_err_probe_ptr'
    3205 |                 return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");
         |                        ^~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from include/linux/cpufreq.h:12,
                    from drivers/tty/serial/sh-sci.c:23:
   include/linux/device.h:988:7: note: declared here
     988 | void *dev_err_probe_ptr(const struct device *dev, int err, const char *fmt, va_list args)
         |       ^~~~~~~~~~~~~~~~~


vim +/dev_err_probe_ptr +3205 drivers/tty/serial/sh-sci.c

  3187	
  3188	static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
  3189						  unsigned int *dev_id)
  3190	{
  3191		struct device_node *np = pdev->dev.of_node;
  3192		struct reset_control *rstc;
  3193		struct plat_sci_port *p;
  3194		struct sci_port *sp;
  3195		const void *data;
  3196		int id, ret;
  3197	
  3198		if (!IS_ENABLED(CONFIG_OF) || !np)
  3199			return ERR_PTR(-EINVAL);
  3200	
  3201		data = of_device_get_match_data(&pdev->dev);
  3202	
  3203		rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
  3204		if (IS_ERR(rstc))
> 3205			return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");
  3206	
  3207		ret = reset_control_deassert(rstc);
  3208		if (ret) {
  3209			dev_err(&pdev->dev, "failed to deassert reset %d\n", ret);
  3210			return ERR_PTR(ret);
  3211		}
  3212	
  3213		ret = devm_add_action_or_reset(&pdev->dev, sci_reset_control_assert, rstc);
  3214		if (ret) {
  3215			dev_err(&pdev->dev, "failed to register assert devm action, %d\n",
  3216				ret);
  3217			return ERR_PTR(ret);
  3218		}
  3219	
  3220		p = devm_kzalloc(&pdev->dev, sizeof(struct plat_sci_port), GFP_KERNEL);
  3221		if (!p)
  3222			return ERR_PTR(-ENOMEM);
  3223	
  3224		/* Get the line number from the aliases node. */
  3225		id = of_alias_get_id(np, "serial");
  3226		if (id < 0 && ~sci_ports_in_use)
  3227			id = ffz(sci_ports_in_use);
  3228		if (id < 0) {
  3229			dev_err(&pdev->dev, "failed to get alias id (%d)\n", id);
  3230			return ERR_PTR(-EINVAL);
  3231		}
  3232		if (id >= ARRAY_SIZE(sci_ports)) {
  3233			dev_err(&pdev->dev, "serial%d out of range\n", id);
  3234			return ERR_PTR(-EINVAL);
  3235		}
  3236	
  3237		sp = &sci_ports[id];
  3238		*dev_id = id;
  3239	
  3240		p->type = SCI_OF_TYPE(data);
  3241		p->regtype = SCI_OF_REGTYPE(data);
  3242	
  3243		sp->has_rtscts = of_property_read_bool(np, "uart-has-rtscts");
  3244	
  3245		return p;
  3246	}
  3247	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 2/2] serial: sh-sci: Switch to use dev_err_probe_ptr()

[Andi Shyti]

Hi Andy,

On Mon, Feb 14, 2022 at 04:32:48PM +0200, Andy Shevchenko wrote:
> Instead of
> 	return ERR_PTR(dev_err_probe(...));
> call
> 	return dev_err_probe_ptr(...);
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/sh-sci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index b610b27893a8..0fce09c13847 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3199,8 +3199,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>  
>  	rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
>  	if (IS_ERR(rstc))
> -		return ERR_PTR(dev_err_probe(&pdev->dev, PTR_ERR(rstc),
> -					     "failed to get reset ctrl\n"));
> +		return dev_err_probe_ptr(&pdev->dev, PTR_ERR(rstc), "failed to get reset ctrl\n");

I think this is a great idea. Like Geert, however, I believe that
this could be more effective as a vararg function. Furthermore,
wouldn't it be easier if the PTR_ERR conversion was carried out
directly within dev_err_probe_ptr?

Wouldn't this approach be more compact and convenient?

	return dev_err_probe_ptr(&pdev->dev, rstc,
				 "failed to get reset ctrl\n");


Now, things start to get a bit more complicated as we will have
four different combinations of dev_err_probes. They could either
take a pointer with an error or an error integer, and they could
either return an error or a pointer with an error. All four cases
are utilized.

Please include me in your next batch of patches as I am also
working on something similar in the background.

Thanks,
Andi

>  
>  	ret = reset_control_deassert(rstc);
>  	if (ret) {
> -- 
> 2.34.1
> 

Re: [PATCH v1 1/2] driver core: add a wrapper to device probe log helper to return pointer

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 14, 2022 at 3:32 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Sometimes the ->probe() function can be split to the core and actual probe
> parts. In such cases the core one may return a pointer to the allocated
> resource, or error pointer in unsuccessful scenario. Allow that kind of
> core function to use dev_err_probe_ptr(), so the following excerpt
>
>         ret = bar(...);
>         if (ret) {
>                 dev_err_probe(dev, ret, ...);
>                 return ERR_PTR(ret);
>         }
>
> can be replaced with
>
>         ret = bar(...);
>         if (ret)
>                 return dev_err_probe_ptr(dev, ret, ...);
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -14,6 +14,7 @@
>
>  #include <linux/dev_printk.h>
>  #include <linux/energy_model.h>
> +#include <linux/err.h>
>  #include <linux/ioport.h>
>  #include <linux/kobject.h>
>  #include <linux/klist.h>
> @@ -982,6 +983,13 @@ void device_links_supplier_sync_state_resume(void);
>  extern __printf(3, 4)
>  int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
>
> +/* As above, but returns error pointer */
> +static inline __printf(3, 0)
> +void *dev_err_probe_ptr(const struct device *dev, int err, const char *fmt, va_list args)

Shouldn't this be a varargs function, like dev_err_probe()?

> +{
> +       return ERR_PTR(dev_err_probe(dev, err, fmt, args));
> +}
> +
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
>         MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds