Page MenuHomeFreeBSD

nvmecontrol: Add APST feature support
Needs ReviewPublic

Authored by sap_eseipi.net on Nov 1 2024, 10:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Aug 2, 5:01 AM
Unknown Object (File)
Sat, Aug 1, 8:01 AM
Unknown Object (File)
Fri, Jul 24, 5:33 PM
Unknown Object (File)
Thu, Jul 23, 6:51 AM
Unknown Object (File)
Jul 4 2026, 2:51 PM
Unknown Object (File)
Jul 3 2026, 7:18 AM
Unknown Object (File)
May 30 2026, 8:45 AM
Unknown Object (File)
May 30 2026, 5:21 AM

Details

Reviewers
imp
Summary

Add -a, -d and -m options to the power subcommand to manage APST,
along with additional status output when invoked without args.

Signed-off-by: Alexey Sukhoguzov <aps@eseipi.net>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75358
Build 72241: arc lint + arc unit

Event Timeline

What has to be done to get this committed? If it's about testing:
I have this stuff running on 14 for 3/2 years now and it works great...

This looks good. I had intended to commit this when submitted, but it fall off my radar.
I did have one question: when validating the power states, you compare against 32 instead of the number of power states in the card. It seems like you should do the latter, is there any reason, other than the max possible power state, that you did this? I suppose the drive will reject bad states when the set feature is sent, but maybe it's better to validate where I flagged.

sbin/nvmecontrol/power.c
193

Shouldn't this check against cdata->npss for the high end?

This revision is now accepted and ready to land.Sun, Aug 2, 1:46 PM
sbin/nvmecontrol/power.c
193

I've staged this with that one change.

About ITPS upper boundary: I'm not sure why I did it this way, so yeah, I think it'd be correct to check the value against NPSS. Thanks!

Also, after I took a fresh look at the code, I've found some more issues (please correct me if I'm wrong somewhere, I haven't touched NVME stuff since this patch):

  1. We can't really transition to a lower power state (from ITPS description: "[...] This field should not specify a power state with higher reported idle power than the current power state. [...]"), and it doesn't make sense to autonomously switch to the same PS, so I think ITPS should actually be (i < ITPS < NPSS), not (0 <= ITPS < NPSS).
  2. Furthermore, ITPS must be NOPS. I've added a check for that, too.
  3. In ITPT error message, the upper boundary was incorrectly specified as 1 << 24, not as (1 << 24) - 1. Maybe I should have defined a constant for this value somewhere, I'm not sure.
  4. We should clear ITPS if ITPT was set to 0 (from the same ITPS description: "[...] If the ITPT field is cleared to 0h, then this field should be cleared to 0h.").
  5. I messed up a cast in htole64(itpt << 8 | i << 3) for ITPT >= 8388608 (that is, 1 << 23). If such a value is provided, the 31th bit will become set after the shift, as well as bits 32..63 after the conversion, which are reserved by the spec. Sorry for that, my fault.
  6. And finally, while at it, I've also did an attempt to make error messages slightly more informative. This way I think it'll be more clear where the problem is, if there is one.
This revision now requires review to proceed.Mon, Aug 3, 1:42 AM
sbin/nvmecontrol/power.c
217–218

I forgot to explain this change. In the previous diff, entry >> 8 had the same issue because of signed right shift as in #5 of my previous comment. However, instead of casting, here it was easier and more correct to just switch to uint64_t. And while I was at it, I also introduced separate itps and itpt for clarity.

228

I also forgot to change this bitmask to 0xFF for consistency with the ITPT bitmask below. Instead, I could've used 0xFFFFFF00 there, but I think it's slightly more readable and maintainable this way.