On 8/21/2024 11:29 AM, Bart Van Assche wrote:
Accessing a host controller register after the host controller has
entered the hibernation state may cause the host controller to exit the
hibernation state. Hence rework the hibernation entry code such that it
does not modify the interrupt enabled status. This patch relies on the
following:
* If an UIC command is submitted that should be completed by the UIC
command completion interrupt, hba->uic_async_done == NULL.
* If an UIC command is submitted that should be completed by the power
mode change interrupt or by a hibernation state change interrupt,
hba->uic_async_done != NULL.
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
drivers/ufs/core/ufshcd.c | 22 ++++++----------------
include/ufs/ufshcd.h | 7 ++++---
2 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d0ae6e50becc..e12f30b8a83c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2585,6 +2585,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
ufshcd_hold(hba);
mutex_lock(&hba->uic_cmd_mutex);
ufshcd_add_delay_before_dme_cmd(hba);
+ WARN_ON(hba->uic_async_done);
ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
if (!ret)
@@ -4255,7 +4256,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
unsigned long flags;
u8 status;
int ret;
- bool reenable_intr = false;
mutex_lock(&hba->uic_cmd_mutex);
ufshcd_add_delay_before_dme_cmd(hba);
@@ -4266,15 +4266,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
goto out_unlock;
}
hba->uic_async_done = &uic_async_done;
- if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
- ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
- /*
- * Make sure UIC command completion interrupt is disabled before
- * issuing UIC command.
- */
- ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
- reenable_intr = true;
- }
spin_unlock_irqrestore(hba->host->host_lock, flags);
ret = __ufshcd_send_uic_cmd(hba, cmd, false);
if (ret) {
@@ -4318,8 +4309,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
spin_lock_irqsave(hba->host->host_lock, flags);
hba->active_uic_cmd = NULL;
hba->uic_async_done = NULL;
- if (reenable_intr)
- ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
if (ret) {
ufshcd_set_link_broken(hba);
ufshcd_schedule_eh_work(hba);
@@ -5472,11 +5461,12 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
if (intr_status & UIC_COMMAND_COMPL && cmd) {
Let's consider this corner case. I am not sure if it's a valid concern,
but it is a corner case that the extra interrupt (new behavior
introduced by this patch) may bring.
Let's say you are sending a ufshcd_uic_pwr_ctrl() command. You will get
2 uic completion interrupts:
[1] ufshcd_uic_cmd_compl() is called for the first interrupt which
happens to be UFSHCD_UIC_PWR_MASK only. At the end of the
ufshcd_uic_pwr_ctrl(), you would set the hba->active_uic_cmd to NULL.
[2]The second uic completion interrupt for UIC_COMMAND_COMP is delayed.
This interrupt is newly introduced by this patch.
Now let's say you have a new UIC command coming via
ufshcd_send_uic_cmd(). The ufshcd_dispatch_uic_cmd() will update your
hba->active_uic_cmd to the new uic_cmd.
The delayed interrupt mentioned in [2] arrives late. The
ufshcd_uic_cmd_compl() is called. In this scenario, this check here may
return true "if (intr_status & UIC_COMMAND_COMPL && cmd) {..}"
Now, the ufshcd_uic_cmd_compl() would proceed to complete the
UIC_COMMAND_COMPL interrupt for the new command intended for the
previous "old" uic command.
Thanks, Bao
- cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
- cmd->argument3 = ufshcd_get_dme_attr_val(hba);
- if (!hba->uic_async_done)
+ if (!hba->uic_async_done) {
+ cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
+ cmd->argument3 = ufshcd_get_dme_attr_val(hba);
cmd->cmd_active = 0;
- complete(&cmd->done);
+ complete(&cmd->done);
+ }
retval = IRQ_HANDLED;
}
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a43b14276bc3..0577013a4611 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -868,9 +868,10 @@ enum ufshcd_mcq_opr {
* @tmf_tag_set: TMF tag set.
* @tmf_queue: Used to allocate TMF tags.
* @tmf_rqs: array with pointers to TMF requests while these are in progress.
- * @active_uic_cmd: handle of active UIC command
- * @uic_cmd_mutex: mutex for UIC command
- * @uic_async_done: completion used during UIC processing
+ * @active_uic_cmd: active UIC command pointer.
+ * @uic_cmd_mutex: mutex used to serialize UIC command processing.
+ * @uic_async_done: completion used to wait for power mode or hibernation state
+ * changes.
* @ufshcd_state: UFSHCD state
* @eh_flags: Error handling flags
* @intr_mask: Interrupt Mask Bits
