[PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Andy Shevchenko]

We have got already new users of this API which interpret it differently
and miss the opportunity to optimize their code.

In order to avoid similar cases in the future, annotate dev_err_probe()
with __must_check.

Fixes: a787e5400a1c ("driver core: add device probe log helper")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index ca18da4768e3..f9d2e5703bbf 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -978,7 +978,7 @@ void device_links_supplier_sync_state_pause(void);
 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, ...);
+int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
 
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
-- 
2.28.0

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Andrzej Hajda]

Hi Andy,

On 26.08.2020 12:44, Andy Shevchenko wrote:
> We have got already new users of this API which interpret it differently
> and miss the opportunity to optimize their code.
>
> In order to avoid similar cases in the future, annotate dev_err_probe()
> with __must_check.


There are many cases where __must_check can be annoying, for example:

ret = ...;

if (ret < 0) {

     dev_err_probe(...);

     goto cleanup;

}


Or (less frequently):

ptr = ...;

if (IS_ERR(ptr)) {

     dev_err_probe(...);

     return ptr;

}


Of course in both cases one can add workarounds, but I am not sure what 
is better.


Regards

Andrzej

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Andy Shevchenko]

On Wed, Aug 26, 2020 at 01:23:40PM +0200, Andrzej Hajda wrote:
> On 26.08.2020 12:44, Andy Shevchenko wrote:
> > We have got already new users of this API which interpret it differently
> > and miss the opportunity to optimize their code.
> >
> > In order to avoid similar cases in the future, annotate dev_err_probe()
> > with __must_check.
> 
> 
> There are many cases where __must_check can be annoying, for example:
> 
> ret = ...;
> 
> if (ret < 0) {
> 
>      dev_err_probe(...);
> 
>      goto cleanup;

Can be
	ret = dev_err_probe(...);

> }
> 
> 
> Or (less frequently):
> 
> ptr = ...;
> 
> if (IS_ERR(ptr)) {
> 
>      dev_err_probe(...);
> 
>      return ptr;

...which basically should be something like

	return dev_err_probe_ptr(...);

> }
> 
> 
> Of course in both cases one can add workarounds, but I am not sure what 
> is better.

Me neither, but definitely API in current state allows to make code suboptimal.
So, up to Greg and Rafael to decide.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Joe Perches]

On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> We have got already new users of this API which interpret it differently
> and miss the opportunity to optimize their code.
> 
> In order to avoid similar cases in the future, annotate dev_err_probe()
> with __must_check.
> 
> Fixes: a787e5400a1c ("driver core: add device probe log helper")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/device.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ca18da4768e3..f9d2e5703bbf 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -978,7 +978,7 @@ void device_links_supplier_sync_state_pause(void);
>  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, ...);
> +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);

Generally, the __must_check should go before the return type
and the extern isn't necessary and is also generally not used
in device.h, so I'd prefer:

__printf(3, 4)
__must_check int dev_err_probe(...);

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Andy Shevchenko]

On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:

...

> > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> 
> Generally, the __must_check should go before the return type
> and the extern isn't necessary and is also generally not used
> in device.h, so I'd prefer:
> 
> __printf(3, 4)
> __must_check int dev_err_probe(...);

I grepped the current code... I don't see support of your preference.
Maybe I missed something? (I'm talking about include/*)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Joe Perches]

On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > 
> > Generally, the __must_check should go before the return type
> > and the extern isn't necessary and is also generally not used
> > in device.h, so I'd prefer:
> > 
> > __printf(3, 4)
> > __must_check int dev_err_probe(...);
> 
> I grepped the current code... I don't see support of your preference.
> Maybe I missed something? (I'm talking about include/*)

Hey Andy.

The __must_check location is just personal belief
as it makes grep significantly easier

<qualifiers> type function(args...)

is a much easier grep pattern than

<qualifiers> type <qualifiers> function(args...)

extern function declarations are generally unnecessary
and should be avoided.

cheers, Joe

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Krzysztof Kozlowski]

On Wed, 26 Aug 2020 at 18:18, Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> > On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);

+Cc Stephen and Greg,

Hi Andy,

Did this patch ended up in next somehow? I am surprised because now I
got warnings for perfectly fine code:
https://lore.kernel.org/linux-next/20200909155654.76fe3bd6@canb.auug.org.au/T/#u

This creates simply false warnings instead of hints for "optimization".

Best regards,
Krzysztof

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Greg Kroah-Hartman]

On Wed, Sep 09, 2020 at 08:29:25AM +0200, Krzysztof Kozlowski wrote:
> On Wed, 26 Aug 2020 at 18:18, Joe Perches <joe@perches.com> wrote:
> >
> > On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > > > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> > >
> > > ...
> > >
> > > > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> 
> +Cc Stephen and Greg,
> 
> Hi Andy,
> 
> Did this patch ended up in next somehow? I am surprised because now I
> got warnings for perfectly fine code:
> https://lore.kernel.org/linux-next/20200909155654.76fe3bd6@canb.auug.org.au/T/#u
> 
> This creates simply false warnings instead of hints for "optimization".

Yes, it got merged into m y driver core tree.

I'll fix up the tty build warning, should be easy enough, the patch is
below.

thanks,

greg k-h
------------------

diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fd95860cd661..cd1880715bad 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -151,7 +151,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
 	/* register the port */
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
-		dev_err_probe(&pdev->dev, ret, "unable to register 8250 port\n");
+		ret = dev_err_probe(&pdev->dev, ret, "unable to register 8250 port\n");
 		goto dis_clk;
 	}
 	data->line = ret;

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Krzysztof Kozlowski]

On Wed, 9 Sep 2020 at 09:02, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 09, 2020 at 08:29:25AM +0200, Krzysztof Kozlowski wrote:
> > On Wed, 26 Aug 2020 at 18:18, Joe Perches <joe@perches.com> wrote:
> > >
> > > On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > > > > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> > > >
> > > > ...
> > > >
> > > > > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > > > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> >
> > +Cc Stephen and Greg,
> >
> > Hi Andy,
> >
> > Did this patch ended up in next somehow? I am surprised because now I
> > got warnings for perfectly fine code:
> > https://lore.kernel.org/linux-next/20200909155654.76fe3bd6@canb.auug.org.au/T/#u
> >
> > This creates simply false warnings instead of hints for "optimization".
>
> Yes, it got merged into m y driver core tree.
>
> I'll fix up the tty build warning, should be easy enough, the patch is
> below.

Yes, this fix suppresses the warning but the question is whether we
really want the warning?
Such fixes mean additional code which the compiler might not optimize
(unless it inlines the dev_err_probe()). This additional code is
purely for suppressing the warning, without any meaning on its own.
Actually it might be even confusing for someone to see:
if (ret)
  ret = dev_err_probe(ret);

warn_unused_result should point errors, not "optimization
opportunities". If you want to have opportunity, add a coccinelle
rule. Or a checkpatch rule. Not a compiler warning.

Best regards,
Krzysztof

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Greg Kroah-Hartman]

On Wed, Sep 09, 2020 at 09:08:14AM +0200, Krzysztof Kozlowski wrote:
> On Wed, 9 Sep 2020 at 09:02, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Sep 09, 2020 at 08:29:25AM +0200, Krzysztof Kozlowski wrote:
> > > On Wed, 26 Aug 2020 at 18:18, Joe Perches <joe@perches.com> wrote:
> > > >
> > > > On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> > > > > On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > > > > > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > > > > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > >
> > > +Cc Stephen and Greg,
> > >
> > > Hi Andy,
> > >
> > > Did this patch ended up in next somehow? I am surprised because now I
> > > got warnings for perfectly fine code:
> > > https://lore.kernel.org/linux-next/20200909155654.76fe3bd6@canb.auug.org.au/T/#u
> > >
> > > This creates simply false warnings instead of hints for "optimization".
> >
> > Yes, it got merged into m y driver core tree.
> >
> > I'll fix up the tty build warning, should be easy enough, the patch is
> > below.
> 
> Yes, this fix suppresses the warning but the question is whether we
> really want the warning?
> Such fixes mean additional code which the compiler might not optimize
> (unless it inlines the dev_err_probe()). This additional code is
> purely for suppressing the warning, without any meaning on its own.
> Actually it might be even confusing for someone to see:
> if (ret)
>   ret = dev_err_probe(ret);

Yeah, that is dumb, as the patch I made shows :(

> warn_unused_result should point errors, not "optimization
> opportunities". If you want to have opportunity, add a coccinelle
> rule. Or a checkpatch rule. Not a compiler warning.

Ok, I now agree, I'll go revert this patch and trust that driver authors
will "do the right thing" here...

thanks,

greg k-h

Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check

[Andy Shevchenko]

On Wed, Sep 9, 2020 at 10:40 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Sep 09, 2020 at 09:08:14AM +0200, Krzysztof Kozlowski wrote:
> > On Wed, 9 Sep 2020 at 09:02, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Wed, Sep 09, 2020 at 08:29:25AM +0200, Krzysztof Kozlowski wrote:
> > > > On Wed, 26 Aug 2020 at 18:18, Joe Perches <joe@perches.com> wrote:
> > > > > On Wed, 2020-08-26 at 18:55 +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Aug 26, 2020 at 08:44:30AM -0700, Joe Perches wrote:
> > > > > > > On Wed, 2020-08-26 at 13:44 +0300, Andy Shevchenko wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > > > > > > +int __must_check dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> > > >
> > > > +Cc Stephen and Greg,
> > > >
> > > > Hi Andy,
> > > >
> > > > Did this patch ended up in next somehow? I am surprised because now I
> > > > got warnings for perfectly fine code:
> > > > https://lore.kernel.org/linux-next/20200909155654.76fe3bd6@canb.auug.org.au/T/#u
> > > >
> > > > This creates simply false warnings instead of hints for "optimization".
> > >
> > > Yes, it got merged into m y driver core tree.
> > >
> > > I'll fix up the tty build warning, should be easy enough, the patch is
> > > below.
> >
> > Yes, this fix suppresses the warning but the question is whether we
> > really want the warning?
> > Such fixes mean additional code which the compiler might not optimize
> > (unless it inlines the dev_err_probe()). This additional code is
> > purely for suppressing the warning, without any meaning on its own.
> > Actually it might be even confusing for someone to see:
> > if (ret)
> >   ret = dev_err_probe(ret);

The problem here is that the dev_err_probe() returns int on purpose.
In your patch what I can see it seems another issue is that the driver
is semi converted to devm API and thus uses goto:s here and there.

> Yeah, that is dumb, as the patch I made shows :(

I agree.

> > warn_unused_result should point errors, not "optimization
> > opportunities". If you want to have opportunity, add a coccinelle
> > rule. Or a checkpatch rule. Not a compiler warning.
>
> Ok, I now agree, I'll go revert this patch and trust that driver authors
> will "do the right thing" here...

I'm fine (as I stated during a review of that patch) to go either way,
but I see it would be nice to have drivers be better thought about
using devm APIs.

-- 
With Best Regards,
Andy Shevchenko