linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] nvme: explicitly disable APST on quirked devices
@ 2017-06-23  6:19 Kai-Heng Feng
  2017-06-23 17:17 ` Andy Lutomirski
  0 siblings, 1 reply; 4+ messages in thread
From: Kai-Heng Feng @ 2017-06-23  6:19 UTC (permalink / raw)
  To: hch; +Cc: luto, linux-nvme, linux-kernel, Kai-Heng Feng

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 <kai.heng.feng@canonical.com>
---
 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;
+	}
+
 	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;
+	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;
 	}
 	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);
 
 	nvme_configure_apst(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ec8c7363934d..95d0cdafe0d5 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -165,6 +165,7 @@ struct nvme_ctrl {
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
+	bool apst_enabled;
 
 	u32 hmpre;
 	u32 hmmin;
-- 
2.13.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: explicitly disable APST on quirked devices
  2017-06-23  6:19 [PATCH] nvme: explicitly disable APST on quirked devices Kai-Heng Feng
@ 2017-06-23 17:17 ` Andy Lutomirski
  2017-06-24  7:47   ` Kai-Heng Feng
  0 siblings, 1 reply; 4+ messages in thread
From: Andy Lutomirski @ 2017-06-23 17:17 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Christoph Hellwig, Andrew Lutomirski, linux-nvme, linux-kernel

On Thu, Jun 22, 2017 at 11:19 PM, Kai-Heng Feng
<kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com>
> ---
>  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.

>         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.


>         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.

--Andy

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: explicitly disable APST on quirked devices
  2017-06-23 17:17 ` Andy Lutomirski
@ 2017-06-24  7:47   ` Kai-Heng Feng
  2017-06-24 16:25     ` Andy Lutomirski
  0 siblings, 1 reply; 4+ messages in thread
From: Kai-Heng Feng @ 2017-06-24  7:47 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: Christoph Hellwig, linux-nvme, linux-kernel

On Sat, Jun 24, 2017 at 1:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Thu, Jun 22, 2017 at 11:19 PM, Kai-Heng Feng
> <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com>
>> ---
>>  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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: explicitly disable APST on quirked devices
  2017-06-24  7:47   ` Kai-Heng Feng
@ 2017-06-24 16:25     ` Andy Lutomirski
  0 siblings, 0 replies; 4+ messages in thread
From: Andy Lutomirski @ 2017-06-24 16:25 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Andy Lutomirski, Christoph Hellwig, linux-nvme, linux-kernel

On Sat, Jun 24, 2017 at 12:47 AM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> On Sat, Jun 24, 2017 at 1:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Thu, Jun 22, 2017 at 11:19 PM, Kai-Heng Feng
>> <kai.heng.feng@canonical.com> 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 <kai.heng.feng@canonical.com>
>>> ---
>>>  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?

No, I mean just not adding that code.  Isn't the intended behavior that:

 - !apsta: do nothing
 - apsta && !apst_enabled: do the set_feature call to turn off APST
 - apsta && apst_enabled: do what the max_latency says to do

The block of code you're adding adds a dev_info() that seems to be
unnecessary and skips re-disabling APST when not in NVME_CTRL_NEW or
NVME_CTRL_RESETTING, which also seems unnecessary.

>>>         } 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?

Suspend/resume, I think, and probably at other times.

>
>>
>>
>>>         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.

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-24 16:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-23  6:19 [PATCH] nvme: explicitly disable APST on quirked devices Kai-Heng Feng
2017-06-23 17:17 ` Andy Lutomirski
2017-06-24  7:47   ` Kai-Heng Feng
2017-06-24 16:25     ` Andy Lutomirski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).