From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751330AbdFXHrF (ORCPT ); Sat, 24 Jun 2017 03:47:05 -0400 Received: from mail-wr0-f179.google.com ([209.85.128.179]:35037 "EHLO mail-wr0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751237AbdFXHrD (ORCPT ); Sat, 24 Jun 2017 03:47:03 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170623061942.5225-1-kai.heng.feng@canonical.com> From: Kai-Heng Feng Date: Sat, 24 Jun 2017 15:47:01 +0800 Message-ID: Subject: Re: [PATCH] nvme: explicitly disable APST on quirked devices To: Andy Lutomirski Cc: Christoph Hellwig , linux-nvme , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jun 24, 2017 at 1:17 AM, Andy Lutomirski wrote: > On Thu, Jun 22, 2017 at 11:19 PM, Kai-Heng Feng > wrote: >> A user reports APST is enabled, even when the NVMe is quirked or with >> option "default_ps_max_latency_us=0". >> >> The current logic will not set APST if the device is quirked. But the >> NVMe in question will enable APST automatically. >> >> Separate the logic "apst is supported" and "to enable apst", so we can >> use the latter one to explicitly disable APST at initialiaztion. >> >> BugLink: https://bugs.launchpad.net/bugs/1699004 >> Signed-off-by: Kai-Heng Feng >> --- >> drivers/nvme/host/core.c | 25 +++++++++++++++++-------- >> drivers/nvme/host/nvme.h | 1 + >> 2 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 0ddd6b9af7fc..c459d15d18f5 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1477,6 +1477,14 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl) >> if (!ctrl->apsta) >> return; >> >> + if (!ctrl->apst_enabled) { >> + if (ctrl->state == NVME_CTRL_NEW || >> + ctrl->state == NVME_CTRL_RESETTING) >> + dev_info(ctrl->device, "Disable APST at initialization\n"); >> + else >> + return; >> + } >> + > > Is this change really necessary? ISTM that, if we want to optimize > the case where we're not changing anything, we should do it more > generally. Do you mean combining the check on ctrl->apsta and ctrl->apst_enabled if we do nothing and just want to return? > >> if (ctrl->npss > 31) { >> dev_warn(ctrl->device, "NPSS is invalid; not using APST\n"); >> return; >> @@ -1486,7 +1494,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl) >> if (!table) >> return; >> >> - if (ctrl->ps_max_latency_us == 0) { >> + if (ctrl->ps_max_latency_us == 0 || !ctrl->apst_enabled) { >> /* Turn off APST. */ >> apste = 0; >> dev_dbg(ctrl->device, "APST disabled\n"); >> @@ -1653,7 +1661,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) >> u64 cap; >> int ret, page_shift; >> u32 max_hw_sectors; >> - u8 prev_apsta; >> + bool prev_apst_enabled; >> >> ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs); >> if (ret) { >> @@ -1721,16 +1729,17 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) >> ctrl->kas = le16_to_cpu(id->kas); >> >> ctrl->npss = id->npss; >> - prev_apsta = ctrl->apsta; >> + ctrl->apsta = id->apsta; > > So ctrl->apsta now means, literally, is APSTA set in the features. > This seems good. > >> + prev_apst_enabled = ctrl->apst_enabled; >> if (ctrl->quirks & NVME_QUIRK_NO_APST) { >> if (force_apst && id->apsta) { >> dev_warn(ctrl->device, "forcibly allowing APST due to nvme_core.force_apst -- use at your own risk\n"); >> - ctrl->apsta = 1; >> + ctrl->apst_enabled = true; >> } else { >> - ctrl->apsta = 0; >> + ctrl->apst_enabled = false; >> } >> } else { >> - ctrl->apsta = id->apsta; >> + ctrl->apst_enabled = true; > > Shouldn't this be ctrl->apst_enabled = id->apsta? > > The way you have it could cause us to do the wrong thing if id->apsta > somehow changes between identifications. You are right. It should be initialized with id->apsta. I am curious though, when does NVMe do multiple identifications? > > >> memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd)); >> >> @@ -1760,9 +1769,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) >> >> kfree(id); >> >> - if (ctrl->apsta && !prev_apsta) >> + if (ctrl->apst_enabled && !prev_apst_enabled) >> dev_pm_qos_expose_latency_tolerance(ctrl->device); >> - else if (!ctrl->apsta && prev_apsta) >> + else if (!ctrl->apst_enabled && prev_apst_enabled) >> dev_pm_qos_hide_latency_tolerance(ctrl->device); > > This is also wrong unless you make the change above, I think. Thanks, I'll address these issues on later version. > > --Andy