Re: [Openipmi-developer] [PATCH v2] ipmi: make ipmi_destroy_user() return void

[Corey M. <co...@mi...>]

On Wed, Dec 25, 2024 at 01:45:30AM +0000, Vitaliy Shevtsov wrote:
> Return value of ipmi_destroy_user() has no meaning, because it's always
> zero and callers can do nothing with it. And in most cases it's not
> checked. So make this function return void. This also will eliminate static
> code analyzer warnings such as unreachable code/redundant comparison when
> the return value is checked against non-zero value.

This is applied to my next tree, thank you.

-corey

> 
> Found by Linux Verification Center (linuxtesting.org) with Svace.
> 
> Signed-off-by: Vitaliy Shevtsov <v.s...@ma...>
> ---
> v2: Add changes in drivers/char/ipmi/ipmi_poweroff.c missed by chance
> 
>  drivers/char/ipmi/ipmi_devintf.c    | 5 +----
>  drivers/char/ipmi/ipmi_msghandler.c | 4 +---
>  drivers/char/ipmi/ipmi_poweroff.c   | 6 +-----
>  drivers/char/ipmi/ipmi_watchdog.c   | 5 +----
>  include/linux/ipmi.h                | 2 +-
>  5 files changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> index 332082e02ea5..e6ba35b71f10 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -122,12 +122,9 @@ static int ipmi_open(struct inode *inode, struct file *file)
>  static int ipmi_release(struct inode *inode, struct file *file)
>  {
>  	struct ipmi_file_private *priv = file->private_data;
> -	int                      rv;
>  	struct ipmi_recv_msg *msg, *next;
>  
> -	rv = ipmi_destroy_user(priv->user);
> -	if (rv)
> -		return rv;
> +	ipmi_destroy_user(priv->user);
>  
>  	list_for_each_entry_safe(msg, next, &priv->recv_msgs, link)
>  		ipmi_free_recv_msg(msg);
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index e12b531f5c2f..1e5313748f8b 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -1398,13 +1398,11 @@ static void _ipmi_destroy_user(struct ipmi_user *user)
>  	module_put(owner);
>  }
>  
> -int ipmi_destroy_user(struct ipmi_user *user)
> +void ipmi_destroy_user(struct ipmi_user *user)
>  {
>  	_ipmi_destroy_user(user);
>  
>  	kref_put(&user->refcount, free_user);
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL(ipmi_destroy_user);
>  
> diff --git a/drivers/char/ipmi/ipmi_poweroff.c b/drivers/char/ipmi/ipmi_poweroff.c
> index 941d2dcc8c9d..05f17e3e6207 100644
> --- a/drivers/char/ipmi/ipmi_poweroff.c
> +++ b/drivers/char/ipmi/ipmi_poweroff.c
> @@ -699,8 +699,6 @@ static int __init ipmi_poweroff_init(void)
>  #ifdef MODULE
>  static void __exit ipmi_poweroff_cleanup(void)
>  {
> -	int rv;
> -
>  #ifdef CONFIG_PROC_FS
>  	unregister_sysctl_table(ipmi_table_header);
>  #endif
> @@ -708,9 +706,7 @@ static void __exit ipmi_poweroff_cleanup(void)
>  	ipmi_smi_watcher_unregister(&smi_watcher);
>  
>  	if (ready) {
> -		rv = ipmi_destroy_user(ipmi_user);
> -		if (rv)
> -			pr_err("could not cleanup the IPMI user: 0x%x\n", rv);
> +		ipmi_destroy_user(ipmi_user);
>  		pm_power_off = old_poweroff_func;
>  	}
>  }
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
> index 335eea80054e..f1875b2bebbc 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -1064,7 +1064,6 @@ static void ipmi_register_watchdog(int ipmi_intf)
>  
>  static void ipmi_unregister_watchdog(int ipmi_intf)
>  {
> -	int rv;
>  	struct ipmi_user *loc_user = watchdog_user;
>  
>  	if (!loc_user)
> @@ -1089,9 +1088,7 @@ static void ipmi_unregister_watchdog(int ipmi_intf)
>  	mutex_lock(&ipmi_watchdog_mutex);
>  
>  	/* Disconnect from IPMI. */
> -	rv = ipmi_destroy_user(loc_user);
> -	if (rv)
> -		pr_warn("error unlinking from IPMI: %d\n",  rv);
> +	ipmi_destroy_user(loc_user);
>  
>  	/* If it comes back, restart it properly. */
>  	ipmi_start_timer_on_heartbeat = 1;
> diff --git a/include/linux/ipmi.h b/include/linux/ipmi.h
> index a1c9c0d48ebf..2f74dd90c271 100644
> --- a/include/linux/ipmi.h
> +++ b/include/linux/ipmi.h
> @@ -126,7 +126,7 @@ int ipmi_create_user(unsigned int          if_num,
>   * the users before you destroy the callback structures, it should be
>   * safe, too.
>   */
> -int ipmi_destroy_user(struct ipmi_user *user);
> +void ipmi_destroy_user(struct ipmi_user *user);
>  
>  /* Get the IPMI version of the BMC we are talking to. */
>  int ipmi_get_version(struct ipmi_user *user,
> -- 
> 2.47.1
>