From: Can Guo <[email protected]>
In UFS host reset and restore path, before probe, we stop and start the
host controller once. After host controller is stopped, the pending
requests, if any, are cleared from the doorbell, but no completion IRQ
would be raised due to the hba is stopped.
These pending requests shall be completed along with the first NOP_OUT
command(as it is the first command which can raise a transfer completion
IRQ) sent during probe.
Since the OCSs of these pending requests are not SUCCESS(because they are
not yet literally finished), their UPIUs shall be dumped. When there are
multiple pending requests, the UPIU dump can be overwhelming and may lead
to stability issues because it is in atomic context.
Therefore, before probe, complete these pending requests right after host
controller is stopped and silence the UPIU dump from them.
Signed-off-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
drivers/scsi/ufs/ufshcd.h | 2 ++
2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5950a7c..b92a3f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
break;
} /* end of switch */
- if (host_byte(result) != DID_OK)
+ if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
return result;
}
@@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct work_struct *work)
/*
* if host reset is required then skip clearing the pending
- * transfers forcefully because they will automatically get
- * cleared after link startup.
+ * transfers forcefully because they will get cleared during
+ * host reset and restore
*/
if (needs_reset)
goto skip_pending_xfer_clear;
@@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
int err;
unsigned long flags;
- /* Reset the host controller */
+ /*
+ * Stop the host controller and complete the requests
+ * cleared by h/w
+ */
spin_lock_irqsave(hba->host->host_lock, flags);
ufshcd_hba_stop(hba, false);
+ hba->silence_err_logs = true;
+ ufshcd_complete_requests(hba);
+ hba->silence_err_logs = false;
spin_unlock_irqrestore(hba->host->host_lock, flags);
/* scale up clocks to max frequency before full reinitialization */
@@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
static int ufshcd_reset_and_restore(struct ufs_hba *hba)
{
int err = 0;
- unsigned long flags;
int retries = MAX_HOST_RESET_RETRIES;
do {
@@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
err = ufshcd_host_reset_and_restore(hba);
} while (err && --retries);
- /*
- * After reset the door-bell might be cleared, complete
- * outstanding requests in s/w here.
- */
- spin_lock_irqsave(hba->host->host_lock, flags);
- ufshcd_transfer_req_compl(hba);
- ufshcd_tmc_handler(hba);
- spin_unlock_irqrestore(hba->host->host_lock, flags);
-
return err;
}
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e0fe247..1e51034 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -513,6 +513,7 @@ struct ufs_stats {
* @uic_error: UFS interconnect layer error status
* @saved_err: sticky error mask
* @saved_uic_err: sticky UIC error mask
+ * @silence_err_logs: flag to silence error logs
* @dev_cmd: ufs device management command information
* @last_dme_cmd_tstamp: time stamp of the last completed DME command
* @auto_bkops_enabled: to track whether bkops is enabled in device
@@ -670,6 +671,7 @@ struct ufs_hba {
u32 saved_err;
u32 saved_uic_err;
struct ufs_stats ufs_stats;
+ bool silence_err_logs;
/* Device management request data */
struct ufs_dev_cmd dev_cmd;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
>
>
> From: Can Guo <[email protected]>
>
> In UFS host reset and restore path, before probe, we stop and start the host
> controller once. After host controller is stopped, the pending requests, if any,
> are cleared from the doorbell, but no completion IRQ would be raised due to the
> hba is stopped.
> These pending requests shall be completed along with the first NOP_OUT
> command(as it is the first command which can raise a transfer completion
> IRQ) sent during probe.
> Since the OCSs of these pending requests are not SUCCESS(because they are not
> yet literally finished), their UPIUs shall be dumped. When there are multiple
> pending requests, the UPIU dump can be overwhelming and may lead to
> stability issues because it is in atomic context.
> Therefore, before probe, complete these pending requests right after host
> controller is stopped and silence the UPIU dump from them.
>
> Signed-off-by: Can Guo <[email protected]>
Already reviewed by Alim & Bean, and tested by Bean.
Please keep those tag updated - will make it clear what is still required to merge this series.
Thanks,
Avri
On Mon, Nov 18, 2019 at 9:22 AM Can Guo <[email protected]> wrote:
>
> From: Can Guo <[email protected]>
>
> In UFS host reset and restore path, before probe, we stop and start the
> host controller once. After host controller is stopped, the pending
> requests, if any, are cleared from the doorbell, but no completion IRQ
> would be raised due to the hba is stopped.
> These pending requests shall be completed along with the first NOP_OUT
> command(as it is the first command which can raise a transfer completion
> IRQ) sent during probe.
> Since the OCSs of these pending requests are not SUCCESS(because they are
> not yet literally finished), their UPIUs shall be dumped. When there are
> multiple pending requests, the UPIU dump can be overwhelming and may lead
> to stability issues because it is in atomic context.
> Therefore, before probe, complete these pending requests right after host
> controller is stopped and silence the UPIU dump from them.
>
> Signed-off-by: Can Guo <[email protected]>
> ---
Tested-by: Alim Akhtar <[email protected]>
Please add all previous Ack/reviewed and tested-by tags so that we are
aware what need to be done for this patch.
Thanks
> drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5950a7c..b92a3f4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
> break;
> } /* end of switch */
>
> - if (host_byte(result) != DID_OK)
> + if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
> ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
> return result;
> }
> @@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct work_struct *work)
>
> /*
> * if host reset is required then skip clearing the pending
> - * transfers forcefully because they will automatically get
> - * cleared after link startup.
> + * transfers forcefully because they will get cleared during
> + * host reset and restore
> */
> if (needs_reset)
> goto skip_pending_xfer_clear;
> @@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
> int err;
> unsigned long flags;
>
> - /* Reset the host controller */
> + /*
> + * Stop the host controller and complete the requests
> + * cleared by h/w
> + */
> spin_lock_irqsave(hba->host->host_lock, flags);
> ufshcd_hba_stop(hba, false);
> + hba->silence_err_logs = true;
> + ufshcd_complete_requests(hba);
> + hba->silence_err_logs = false;
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> /* scale up clocks to max frequency before full reinitialization */
> @@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
> static int ufshcd_reset_and_restore(struct ufs_hba *hba)
> {
> int err = 0;
> - unsigned long flags;
> int retries = MAX_HOST_RESET_RETRIES;
>
> do {
> @@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
> err = ufshcd_host_reset_and_restore(hba);
> } while (err && --retries);
>
> - /*
> - * After reset the door-bell might be cleared, complete
> - * outstanding requests in s/w here.
> - */
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - ufshcd_transfer_req_compl(hba);
> - ufshcd_tmc_handler(hba);
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> -
> return err;
> }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e0fe247..1e51034 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -513,6 +513,7 @@ struct ufs_stats {
> * @uic_error: UFS interconnect layer error status
> * @saved_err: sticky error mask
> * @saved_uic_err: sticky UIC error mask
> + * @silence_err_logs: flag to silence error logs
> * @dev_cmd: ufs device management command information
> * @last_dme_cmd_tstamp: time stamp of the last completed DME command
> * @auto_bkops_enabled: to track whether bkops is enabled in device
> @@ -670,6 +671,7 @@ struct ufs_hba {
> u32 saved_err;
> u32 saved_uic_err;
> struct ufs_stats ufs_stats;
> + bool silence_err_logs;
>
> /* Device management request data */
> struct ufs_dev_cmd dev_cmd;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Regards,
Alim
On 2019-11-18 21:23, Alim Akhtar wrote:
> On Mon, Nov 18, 2019 at 9:22 AM Can Guo <[email protected]> wrote:
>>
>> From: Can Guo <[email protected]>
>>
>> In UFS host reset and restore path, before probe, we stop and start
>> the
>> host controller once. After host controller is stopped, the pending
>> requests, if any, are cleared from the doorbell, but no completion IRQ
>> would be raised due to the hba is stopped.
>> These pending requests shall be completed along with the first NOP_OUT
>> command(as it is the first command which can raise a transfer
>> completion
>> IRQ) sent during probe.
>> Since the OCSs of these pending requests are not SUCCESS(because they
>> are
>> not yet literally finished), their UPIUs shall be dumped. When there
>> are
>> multiple pending requests, the UPIU dump can be overwhelming and may
>> lead
>> to stability issues because it is in atomic context.
>> Therefore, before probe, complete these pending requests right after
>> host
>> controller is stopped and silence the UPIU dump from them.
>>
>> Signed-off-by: Can Guo <[email protected]>
>> ---
>
> Tested-by: Alim Akhtar <[email protected]>
>
> Please add all previous Ack/reviewed and tested-by tags so that we are
> aware what need to be done for this patch.
> Thanks
>
Hi Alim,
Thanks for pointing out it. I updated the patch a little bit so I think
the
prevoius tags are not valid any more.
Best Regards,
Can Guo.
>> drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
>> drivers/scsi/ufs/ufshcd.h | 2 ++
>> 2 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 5950a7c..b92a3f4 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct
>> scsi_device *sdev)
>> break;
>> } /* end of switch */
>>
>> - if (host_byte(result) != DID_OK)
>> + if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
>> ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
>> return result;
>> }
>> @@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct
>> work_struct *work)
>>
>> /*
>> * if host reset is required then skip clearing the pending
>> - * transfers forcefully because they will automatically get
>> - * cleared after link startup.
>> + * transfers forcefully because they will get cleared during
>> + * host reset and restore
>> */
>> if (needs_reset)
>> goto skip_pending_xfer_clear;
>> @@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct
>> ufs_hba *hba)
>> int err;
>> unsigned long flags;
>>
>> - /* Reset the host controller */
>> + /*
>> + * Stop the host controller and complete the requests
>> + * cleared by h/w
>> + */
>> spin_lock_irqsave(hba->host->host_lock, flags);
>> ufshcd_hba_stop(hba, false);
>> + hba->silence_err_logs = true;
>> + ufshcd_complete_requests(hba);
>> + hba->silence_err_logs = false;
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>>
>> /* scale up clocks to max frequency before full
>> reinitialization */
>> @@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct
>> ufs_hba *hba)
>> static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>> {
>> int err = 0;
>> - unsigned long flags;
>> int retries = MAX_HOST_RESET_RETRIES;
>>
>> do {
>> @@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct
>> ufs_hba *hba)
>> err = ufshcd_host_reset_and_restore(hba);
>> } while (err && --retries);
>>
>> - /*
>> - * After reset the door-bell might be cleared, complete
>> - * outstanding requests in s/w here.
>> - */
>> - spin_lock_irqsave(hba->host->host_lock, flags);
>> - ufshcd_transfer_req_compl(hba);
>> - ufshcd_tmc_handler(hba);
>> - spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -
>> return err;
>> }
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index e0fe247..1e51034 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -513,6 +513,7 @@ struct ufs_stats {
>> * @uic_error: UFS interconnect layer error status
>> * @saved_err: sticky error mask
>> * @saved_uic_err: sticky UIC error mask
>> + * @silence_err_logs: flag to silence error logs
>> * @dev_cmd: ufs device management command information
>> * @last_dme_cmd_tstamp: time stamp of the last completed DME command
>> * @auto_bkops_enabled: to track whether bkops is enabled in device
>> @@ -670,6 +671,7 @@ struct ufs_hba {
>> u32 saved_err;
>> u32 saved_uic_err;
>> struct ufs_stats ufs_stats;
>> + bool silence_err_logs;
>>
>> /* Device management request data */
>> struct ufs_dev_cmd dev_cmd;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>