[bug report] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

From: Dan Carpenter <dan.carpenter@oracle.com>
To: cang@codeaurora.org
Cc: linux-scsi@vger.kernel.org
Subject: [bug report] scsi: ufs: Try to save power mode change and UIC cmd completion timeout
Date: Mon, 23 May 2022 10:06:20 +0300	[thread overview]
Message-ID: <YosybAfpXHiso+22@kili> (raw)

Hello Can Guo,

The patch 0f52fcb99ea2: "scsi: ufs: Try to save power mode change and
UIC cmd completion timeout" from Nov 2, 2020, leads to the following
Smatch static checker warning:

	drivers/ufs/core/ufshcd.c:4048 ufshcd_uic_pwr_ctrl()
	warn: missing error code here? '_dev_err()' failed. 'ret' = '0'

drivers/ufs/core/ufshcd.c
    4004 static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
    4005 {
    4006         DECLARE_COMPLETION_ONSTACK(uic_async_done);
    4007         unsigned long flags;
    4008         u8 status;
    4009         int ret;
    4010         bool reenable_intr = false;
    4011 
    4012         mutex_lock(&hba->uic_cmd_mutex);
    4013         ufshcd_add_delay_before_dme_cmd(hba);
    4014 
    4015         spin_lock_irqsave(hba->host->host_lock, flags);
    4016         if (ufshcd_is_link_broken(hba)) {
    4017                 ret = -ENOLINK;
    4018                 goto out_unlock;
    4019         }
    4020         hba->uic_async_done = &uic_async_done;
    4021         if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
    4022                 ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
    4023                 /*
    4024                  * Make sure UIC command completion interrupt is disabled before
    4025                  * issuing UIC command.
    4026                  */
    4027                 wmb();
    4028                 reenable_intr = true;
    4029         }
    4030         ret = __ufshcd_send_uic_cmd(hba, cmd, false);
    4031         spin_unlock_irqrestore(hba->host->host_lock, flags);
    4032         if (ret) {
    4033                 dev_err(hba->dev,
    4034                         "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
    4035                         cmd->command, cmd->argument3, ret);
    4036                 goto out;
    4037         }
    4038 
    4039         if (!wait_for_completion_timeout(hba->uic_async_done,
    4040                                          msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
    4041                 dev_err(hba->dev,
    4042                         "pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
    4043                         cmd->command, cmd->argument3);
    4044 
    4045                 if (!cmd->cmd_active) {
    4046                         dev_err(hba->dev, "%s: Power Mode Change operation has been completed, go check UPMCRS\n",
    4047                                 __func__);
--> 4048                         goto check_upmcrs;

Why is this a dev_err()?  It looks like a success path.  I suspect this
is debugging code which escaped into production.  Or alternatively, if
this is an error path the error code needs to be set and the error
message needs to be re-written to be more clear.

    4049                 }
    4050 
    4051                 ret = -ETIMEDOUT;
    4052                 goto out;
    4053         }
    4054 
    4055 check_upmcrs:
    4056         status = ufshcd_get_upmcrs(hba);
    4057         if (status != PWR_LOCAL) {
    4058                 dev_err(hba->dev,
    4059                         "pwr ctrl cmd 0x%x failed, host upmcrs:0x%x\n",
    4060                         cmd->command, status);
    4061                 ret = (status != PWR_OK) ? status : -1;
    4062         }
    4063 out:
    4064         if (ret) {
    4065                 ufshcd_print_host_state(hba);
    4066                 ufshcd_print_pwr_info(hba);
    4067                 ufshcd_print_evt_hist(hba);
    4068         }
    4069 
    4070         spin_lock_irqsave(hba->host->host_lock, flags);
    4071         hba->active_uic_cmd = NULL;
    4072         hba->uic_async_done = NULL;
    4073         if (reenable_intr)
    4074                 ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
    4075         if (ret) {
    4076                 ufshcd_set_link_broken(hba);
    4077                 ufshcd_schedule_eh_work(hba);
    4078         }
    4079 out_unlock:
    4080         spin_unlock_irqrestore(hba->host->host_lock, flags);
    4081         mutex_unlock(&hba->uic_cmd_mutex);
    4082 
    4083         return ret;
    4084 }

regards,
dan carpenter