Page MenuHomeFreeBSD

nvmecontrol: Add Samsung Extended SMART Information logpage support.
ClosedPublic

Authored by wanpengqian_gmail.com on Jan 5 2022, 7:26 AM.
Tags
None
Referenced Files
F102115047: D33749.diff
Thu, Nov 7, 7:28 PM
Unknown Object (File)
Sat, Oct 26, 8:13 PM
Unknown Object (File)
Oct 2 2024, 11:25 AM
Unknown Object (File)
Oct 2 2024, 11:25 AM
Unknown Object (File)
Oct 2 2024, 11:25 AM
Unknown Object (File)
Sep 30 2024, 1:40 PM
Unknown Object (File)
Sep 30 2024, 1:33 PM
Unknown Object (File)
Sep 30 2024, 1:31 PM

Details

Summary

Samsung PM983 SSD has a 0xca logpage. It has more information compared to Intel's
this patch tested on PM983 M2 SSD and works as expected.

Product Datasheet can be found here:
https://gzhls.at/blob/ldb/2/b/4/9/d8f8e423e0134a3f116548621be52b49a323.pdf

Signed-off-by: Wanpeng Qian <wanpengqian@gmail.com>

Test Plan

Currently only tested on PM983 SSDs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43714
Build 40602: arc lint + arc unit

Event Timeline

Generally this looks good. I know you've used the datasheet names for these items, but since the datasheet doesn't actually contain what they mean, it might make sense to tweak the descriptions slightly to give a better clue.

sbin/nvmecontrol/modules/samsung/samsung.c
129

All the printfs from here on down need to be converted from le to host.

141

that's a quite odd name for erase block reserve

144

That's an odd name for number of sectors reallocated....

147

NPO? That's an odd name for the number of clean shutdowns. Number of Power Off spelled out would be slightly less bad, but still obscure.

150

That's an odd name for number of unclean shutdowns, though I see the data sheet uses sudden power off here.

data is converted from le to host
use more accurate description

You are right. the datasheet is not so clear for what it means. I correct some descriptions for better meaning.

sbin/nvmecontrol/modules/samsung/samsung.c
129

For 128bit values, do I need to convert two 64bits first? I am not sure.

141

I use Reserved Erase Block Count to match others' style

144

I change UECC to Uncorrectable ECC here. seems reasonable.

147

is Normal Power Off Count seems odd?

150

use Sudden Power Off Count to match the datasheet's description.

With the revisions, I think this is ready to go based on the posted datasheet pointer.

sbin/nvmecontrol/modules/samsung/samsung.c
129

Yes. However, we should have a le128dec function that we can use here. We don't and now that I'm looking at a lot of the rest of the code, I see that printing the other log pages isn't quite endian correct (Which is why we don't have a le128dec). For now, leave the 128 bit stuff as is and I'll do a pass to fix them all (if someone else does not do it first).

141

That sounds fine.

144

That's fine. UECC or Uncorrectable ECC are both reasonable, and the latter is a little better.

147

That's fine. I think it's what NPO is short for and it will save others having to puzzle it out.

150

Great!

This revision is now accepted and ready to land.Jan 6 2022, 6:28 AM
This revision now requires review to proceed.Jan 7 2022, 5:46 AM

Output SMART ID for better classification

pauamma_gundo.com added inline comments.
sbin/nvmecontrol/nvmecontrol.8
247 ↗(On Diff #101335)

Also, "Western Digital" may be clearer than WDC.

Who's going to commit this? Seems like it's as complete as it can get.

In D33749#835893, @bcr wrote:

Who's going to commit this? Seems like it's as complete as it can get.

Yes, please.

sbin/nvmecontrol/nvmecontrol.8
247 ↗(On Diff #101335)

Also, "Western Digital" may be clearer than WDC.

I think it is well-known for WDC nowadays. Since HGST is a short for Hitachi Global Storage Technologies, use of WDC will match HGST. what do you think?

Explicitly approve the man page change.
@imp: Can you take care of the commit? I associate nvmecontrol mainly with you and you have a src bit to do the commit. Thank you!

pauamma_gundo.com added inline comments.
sbin/nvmecontrol/nvmecontrol.8
247 ↗(On Diff #101335)

Also, "Western Digital" may be clearer than WDC.

I think it is well-known for WDC nowadays. Since HGST is a short for Hitachi Global Storage Technologies, use of WDC will match HGST. what do you think?

I think you know the audience for this and I don't. If you say they will understand WDC the way you mean it, that's good enough for it.

This revision is now accepted and ready to land.Oct 5 2022, 12:55 PM