[PATCH] usb: ohci: remove unreachable platform_driver_unregister() call

[PATCH] usb: ohci: remove unreachable platform_driver_unregister() call

[Jay Fang]

From: Zihao Tang <tangzihao1@hisilicon.com>

Fix the following smatch warnings:

drivers/usb/host/ohci-hcd.c:1318 ohci_hcd_mod_init() warn:
ignoring unreachable code.

platform_driver_register(&TMIO_OHCI_DRIVER) is the last
platform_driver_register() call in ohci_hcd_mod_init(), so if it
failed, there's no need to unregister it, but just goto error_tmio.

So remove the unreachable platform_driver_unregister(&TMIO_OHCI_DRIVER).
No functionality change.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Zihao Tang <tangzihao1@hisilicon.com>
Signed-off-by: Jay Fang <f.fangjian@huawei.com>
---
 drivers/usb/host/ohci-hcd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 1f5e693..2d09ef2 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1319,7 +1319,6 @@ static int __init ohci_hcd_mod_init(void)
 
 	/* Error path */
 #ifdef TMIO_OHCI_DRIVER
-	platform_driver_unregister(&TMIO_OHCI_DRIVER);
  error_tmio:
 #endif
 #ifdef SM501_OHCI_DRIVER
-- 
2.7.4

Re: [PATCH] usb: ohci: remove unreachable platform_driver_unregister() call

[Alan Stern]

On Sat, Mar 20, 2021 at 09:31:51AM +0800, Jay Fang wrote:
> From: Zihao Tang <tangzihao1@hisilicon.com>
> 
> Fix the following smatch warnings:
> 
> drivers/usb/host/ohci-hcd.c:1318 ohci_hcd_mod_init() warn:
> ignoring unreachable code.
> 
> platform_driver_register(&TMIO_OHCI_DRIVER) is the last
> platform_driver_register() call in ohci_hcd_mod_init(), so if it
> failed, there's no need to unregister it, but just goto error_tmio.
> 
> So remove the unreachable platform_driver_unregister(&TMIO_OHCI_DRIVER).
> No functionality change.

Doesn't the compiler realize that the call is unreachable, and 
therefore avoid generating any object for it?

It's true that the function call is, strictly speaking, unnecessary.  
However, it provides a pleasing symmetry and it acts as a guide in the 
unlikely event that anyone wants to add another platform-specific 
driver in the future.

Also, consider what would happen if somebody ever removes the TMIO_OHCI 
platform driver entirely.  They certainly would remove the corresponding
#ifdef...#endif block.  But by your way of thinking, they should remove 
as well the following platform_driver_unregister(&SM501_OHCI_DRIVER) 
call -- something that is not immediately obvious and is likely to be 
overlooked.

On the whole, I prefer to keep the code as it is.  However, if you would 
like to submit a different patch, adding a comment which explains that 
the call is known to be unreachable but is retained nonetheless because 
the maintainer is a crotchety old formalist, I would be willing to apply 
that.

Alan Stern

PS: A #ifdef...#endif block containing nothing but a statement label 
looks a little weird, don't you agree?

> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Zihao Tang <tangzihao1@hisilicon.com>
> Signed-off-by: Jay Fang <f.fangjian@huawei.com>
> ---
>  drivers/usb/host/ohci-hcd.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index 1f5e693..2d09ef2 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -1319,7 +1319,6 @@ static int __init ohci_hcd_mod_init(void)
>  
>  	/* Error path */
>  #ifdef TMIO_OHCI_DRIVER
> -	platform_driver_unregister(&TMIO_OHCI_DRIVER);
>   error_tmio:
>  #endif
>  #ifdef SM501_OHCI_DRIVER
> -- 
> 2.7.4
> 

Re: [PATCH] usb: ohci: remove unreachable platform_driver_unregister() call

[Dan Carpenter]

On Fri, Mar 19, 2021 at 10:24:43PM -0400, Alan Stern wrote:
> On Sat, Mar 20, 2021 at 09:31:51AM +0800, Jay Fang wrote:
> > From: Zihao Tang <tangzihao1@hisilicon.com>
> > 
> > Fix the following smatch warnings:
> > 
> > drivers/usb/host/ohci-hcd.c:1318 ohci_hcd_mod_init() warn:
> > ignoring unreachable code.
> > 
> > platform_driver_register(&TMIO_OHCI_DRIVER) is the last
> > platform_driver_register() call in ohci_hcd_mod_init(), so if it
> > failed, there's no need to unregister it, but just goto error_tmio.
> > 
> > So remove the unreachable platform_driver_unregister(&TMIO_OHCI_DRIVER).
> > No functionality change.
> 
> Doesn't the compiler realize that the call is unreachable, and 
> therefore avoid generating any object for it?
>

This is a static checker warning.  For example, Heart Bleed bug was an
ignored unreachable code static checker warning.

> It's true that the function call is, strictly speaking, unnecessary.  
> However, it provides a pleasing symmetry and it acts as a guide in the 
> unlikely event that anyone wants to add another platform-specific 
> driver in the future.

Hopefully future programmers can figure out basic stuff like that.

This code doesn't trigger a Smatch warning on my .config because the
Smatch check doesn't warn if the previous line was an #endif.  On the
other hand, the ifdefs are also why I forwarded the email when I saw the
warning from kbuild.  Normally kbuild is better at picking the person
to blame but because this is a .config thing that confused it.  Anyway,
I glanced at the warning and thought it looked suspicious enough to
warrant a further look.

When I first wrote the Smatch unreachable code warning there were a
handful of places which used that style of code:

	return 0;

	unreachable_release();
err_release:
	release_something();

I just left those as-is because it was obvious to me that it was done
intentionally.  However, it seems that other people have removed all of
those behind my back so I can't find an example of this now except for
in ohci_hcd_mod_init().

Anyway, I would have put a special case to silence these false positives
but it wasn't common practice in 2014 and no one does it these days.

regards,
dan carpenter

Re: [PATCH] usb: ohci: remove unreachable platform_driver_unregister() call

[Alan Stern]

On Mon, Mar 22, 2021 at 07:58:50AM +0300, Dan Carpenter wrote:
> On Fri, Mar 19, 2021 at 10:24:43PM -0400, Alan Stern wrote:
> > On Sat, Mar 20, 2021 at 09:31:51AM +0800, Jay Fang wrote:
> > > From: Zihao Tang <tangzihao1@hisilicon.com>
> > > 
> > > Fix the following smatch warnings:
> > > 
> > > drivers/usb/host/ohci-hcd.c:1318 ohci_hcd_mod_init() warn:
> > > ignoring unreachable code.
> > > 
> > > platform_driver_register(&TMIO_OHCI_DRIVER) is the last
> > > platform_driver_register() call in ohci_hcd_mod_init(), so if it
> > > failed, there's no need to unregister it, but just goto error_tmio.
> > > 
> > > So remove the unreachable platform_driver_unregister(&TMIO_OHCI_DRIVER).
> > > No functionality change.
> > 
> > Doesn't the compiler realize that the call is unreachable, and 
> > therefore avoid generating any object for it?
> >
> 
> This is a static checker warning.  For example, Heart Bleed bug was an
> ignored unreachable code static checker warning.
> 
> > It's true that the function call is, strictly speaking, unnecessary.  
> > However, it provides a pleasing symmetry and it acts as a guide in the 
> > unlikely event that anyone wants to add another platform-specific 
> > driver in the future.
> 
> Hopefully future programmers can figure out basic stuff like that.
> 
> This code doesn't trigger a Smatch warning on my .config because the
> Smatch check doesn't warn if the previous line was an #endif.  On the
> other hand, the ifdefs are also why I forwarded the email when I saw the
> warning from kbuild.  Normally kbuild is better at picking the person
> to blame but because this is a .config thing that confused it.  Anyway,
> I glanced at the warning and thought it looked suspicious enough to
> warrant a further look.
> 
> When I first wrote the Smatch unreachable code warning there were a
> handful of places which used that style of code:
> 
> 	return 0;
> 
> 	unreachable_release();
> err_release:
> 	release_something();
> 
> I just left those as-is because it was obvious to me that it was done
> intentionally.  However, it seems that other people have removed all of
> those behind my back so I can't find an example of this now except for
> in ohci_hcd_mod_init().
> 
> Anyway, I would have put a special case to silence these false positives
> but it wasn't common practice in 2014 and no one does it these days.

Well, I guess it would be okay to convert the code to a comment.  Just 
so long as it isn't removed entirely.

Alan Stern