| Message ID | 20240909231139.2367576-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improve the UFS driver link hibernation code | expand |
On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Simplify __ufshcd_send_uic_cmd() by always initializing the > uic_cmd::done completion. This is fine since the time required to > initialize a completion is small compared to the time required to > process an UIC command. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Hi Bart, The time to compare should not be with the UIC command process time. It should be compared with the time for the "if (completion)" conditional expression. We cannot justify increasing the latency of sending a UIC command just because the UIC command process time is longer. Thanks. Peter
On 9/10/24 5:34 AM, Peter Wang (王信友) wrote: > On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: >> Simplify __ufshcd_send_uic_cmd() by always initializing the >> uic_cmd::done completion. This is fine since the time required to >> initialize a completion is small compared to the time required to >> process an UIC command. > > The time to compare should not be with the UIC command process time. > It should be compared with the time for the "if (completion)" > conditional > expression. We cannot justify increasing the latency of sending a UIC > command just because the UIC command process time is longer. Although I appreciate your concern about performance, I don't think that the above feedback is appropriate. On my test setup UIC commands take between 20 and 1500 microseconds to complete. A single init_completion() call takes about 0.01 microseconds. So I don't think that the time spent in init_completion() matters for UIC commands. Additionally, this patch only introduces an init_completion() call for power management commands and not any other type of UIC command. Thanks, Bart.
On Tue, 2024-09-10 at 09:52 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 9/10/24 5:34 AM, Peter Wang (王信友) wrote: > > On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > >> Simplify __ufshcd_send_uic_cmd() by always initializing the > >> uic_cmd::done completion. This is fine since the time required to > >> initialize a completion is small compared to the time required to > >> process an UIC command. > > > > The time to compare should not be with the UIC command process > time. > > It should be compared with the time for the "if (completion)" > > conditional > > expression. We cannot justify increasing the latency of sending a > UIC > > command just because the UIC command process time is longer. > > Although I appreciate your concern about performance, I don't think > that > the above feedback is appropriate. On my test setup UIC commands take > between 20 and 1500 microseconds to complete. A single > init_completion() > call takes about 0.01 microseconds. So I don't think that the time > spent > in init_completion() matters for UIC commands. Additionally, this > patch > only introduces an init_completion() call for power management > commands > and not any other type of UIC command. > > Thanks, > > Bart. > Hi Bart, I have concerns about adding latency just for power management commands. This is because when DVFS needs performance and wants to scale up, it will take extra time to complete the scaling up process. However, I agree that such a 0.01ms delay may not have a significant impact on the overall scale-up and read/write process. It might be helpful to add this information to the patch description. Thank you. Peter Reviewed-by: Peter Wang <peter.wang@mediatek.com>
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 134cba0ff512..063fb66c6719 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2537,13 +2537,11 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result * @hba: per adapter instance * @uic_cmd: UIC command - * @completion: initialize the completion only if this is set to true * * Return: 0 only if success. */ static int -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd, - bool completion) +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) { lockdep_assert_held(&hba->uic_cmd_mutex); @@ -2553,8 +2551,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd, return -EIO; } - if (completion) - init_completion(&uic_cmd->done); + init_completion(&uic_cmd->done); uic_cmd->cmd_active = 1; ufshcd_dispatch_uic_cmd(hba, uic_cmd); @@ -2580,7 +2577,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) mutex_lock(&hba->uic_cmd_mutex); ufshcd_add_delay_before_dme_cmd(hba); - ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true); + ret = __ufshcd_send_uic_cmd(hba, uic_cmd); if (!ret) ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); @@ -4270,7 +4267,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) reenable_intr = true; } spin_unlock_irqrestore(hba->host->host_lock, flags); - ret = __ufshcd_send_uic_cmd(hba, cmd, false); + ret = __ufshcd_send_uic_cmd(hba, cmd); if (ret) { dev_err(hba->dev, "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
Simplify __ufshcd_send_uic_cmd() by always initializing the uic_cmd::done completion. This is fine since the time required to initialize a completion is small compared to the time required to process an UIC command. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)