Project

General

Profile

Actions

Bug #14765

closed

SATL could decode page 83 for device IDs

Added by Garrett D'Amore about 2 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Category:
driver - device drivers
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

The LUN device IDs are generally available on modern SATA devices (required by T13 specifications), but our SATL doesn't decode them.

The recommendation in the latest SAT spec is to translate requests to obtain VPD page 0x83 correctly. The best way for us to do that (safest) is to obtain the inquiry information at startup, and store it, and then return it in software.

We have an implementation of this at RackTop.

There is a low risk on upgrade that devices may upon reboot may start enumerating under a /dev/ link with a WWN, instead of a simple cXtYdZ link. ZFS does seem to cope well with this, but folks using legacy filesystems may have some difficulties, particularly if the /dev directory is not persistent (as is the case with some non-persistent root distributions.)

Still, we feel this offers better interoperability if the disk is moved to another adapter, as the names will be preserved, and the use of page 83 data is becoming more and more common in various situations.

Modern hardware based SATLs do translate this already.

Actions #1

Updated by Garrett D'Amore about 2 months ago

Here's the delta I implemented for RackTop. Note that this code assumes that the device is a "modern" SATA drive. Probably we should update it to better accommodate drives that simply lack this page; I've not been consciously aware of any using any such drive, and I have to believe any such drives are long past their manufacturer warranties/expected life times (I don't think anyone has made one in over a decade):

Note that these should carry a RackTop copyright (2021), but they aren't in the diffs because of other changes that we have made in this driver as well.

commit d6784f114e629c6d0f7647cb47b6cdfdb932544a
Author: Garrett D'Amore <garrett@damore.org>
Date:   Thu Oct 14 12:40:17 2021 -0700

    BSR-9997 SATL should translate page 83

diff --git a/usr/src/uts/common/io/sata/impl/sata.c b/usr/src/uts/common/io/sata/impl/sata.c
index dbeed36833..69f0ff201d 100644
--- a/usr/src/uts/common/io/sata/impl/sata.c
+++ b/usr/src/uts/common/io/sata/impl/sata.c
@@ -3647,7 +3647,7 @@ sata_txlt_nodata_cmd_immediate(sata_pkt_txlate_t *spx)
 #define    INQUIRY_BDC_PAGE    0xB1    /* Block Device Characteristics Page */
                     /* Code */
 #define    INQUIRY_ATA_INFO_PAGE    0x89    /* ATA Information Page Code */
-#define    INQUIRY_DEV_IDENTIFICATION_PAGE 0x83 /* Not needed yet */
+#define    INQUIRY_DEV_IDENTIFICATION_PAGE 0x83 /* Device identifiers */

 static int
 sata_txlt_inquiry(sata_pkt_txlate_t *spx)
@@ -3750,13 +3750,15 @@ sata_txlt_inquiry(sata_pkt_txlate_t *spx)
         page_buf[0] = peripheral_device_type;
         page_buf[1] = INQUIRY_SUP_VPD_PAGE;
         page_buf[2] = 0;
-        page_buf[3] = 4; /* page length */
+        page_buf[3] = 5; /* page length */
         page_buf[4] = INQUIRY_SUP_VPD_PAGE;
         page_buf[5] = INQUIRY_USN_PAGE;
         page_buf[6] = INQUIRY_BDC_PAGE;
-        page_buf[7] = INQUIRY_ATA_INFO_PAGE;
+        page_buf[7] = INQUIRY_DEV_IDENTIFICATION_PAGE;
+        page_buf[8] = INQUIRY_ATA_INFO_PAGE;
+
         /* Copy no more than requested */
-        count = MIN(bp->b_bcount, 8);
+        count = MIN(bp->b_bcount, 9);
         bcopy(page_buf, bp->b_un.b_addr, count);
         break;

@@ -3923,11 +3925,25 @@ sata_txlt_inquiry(sata_pkt_txlate_t *spx)

     case INQUIRY_DEV_IDENTIFICATION_PAGE:
         /*
-         * We may want to implement this page, when
-         * identifiers are common for SATA devices
-         * But not now.
+         * Page 83; SAT-5 requires this, and modern
+         * SATA devices all support a WWN.
          */
-        /*FALLTHROUGH*/
+        page_buf[0] = peripheral_device_type;
+        page_buf[1] = INQUIRY_DEV_IDENTIFICATION_PAGE;
+        page_buf[2] = 0;
+        page_buf[3] = 12; /* remaining length */
+        page_buf[4] = 0x01; /* protocol 0, code set 1 */
+        page_buf[5] = 0x03; /* LUN, NAA type */
+        page_buf[6] = 0;
+        page_buf[7] = 0x08; /* length (64-bit WWN) */
+#ifdef    _LITTLE_ENDIAN
+        swab(&sdinfo->satadrv_id.ai_naa_ieee_oui, &page_buf[8], 8);
+#else
+        bcopy(&sdinfo->statadrv_id.ai_naa_ieee_oui, &page_buf[8], 8);
+#endif
+        count = MIN(bp->b_bcount, 12 + 4); /* header + designator */
+        bcopy(page_buf, bp->b_un.b_addr, count);
+        break;

     default:
         /* Request for unsupported VPD page */

Actions #2

Updated by Garrett D'Amore about 2 months ago

  • Description updated (diff)
Actions #3

Updated by Garrett D'Amore about 2 months ago

A little more information.

ATA-8/ACS makes the NAA identifier mandatory.

However, if you look at SATA-1.0, it defines the identify device data per ATA/ATAPI-5. In that specification, these bytes are reserved, and the specification requires that reserved bits are zero.

It would be illegal for the first byte, or the first ushort_t to be all zero, because the NAA nibble must be a valid value and zero is not legal. (Legal NAA format types are 1, 2, 5, 6 (would be 128-bit WWN), and 0xC, 0xD, 0xE, 0xF.

We can (and probably should) examine that first byte and not translate if it is zero.

Actions #4

Updated by Garrett D'Amore about 1 month ago

  • % Done changed from 0 to 80
Actions #5

Updated by Electric Monk about 1 month ago

  • Gerrit CR set to 2212
Actions #6

Updated by Garrett D'Amore about 1 month ago

Testing:

We've been using this code to collect VPD page 83, which is used in our stack for a variety of purposes, for some months now. It has been used in with two different generations of Intel motherboards utilizing the AHCI controller.

Obviously there are some older generation AHCI controllers that we have not tested, as well as some truly ancient SATA drives that we no longer have access to. However, any drive manufactured in the past decade is going to support the WWN information in the DEVICE INFORMATION response for SATA drives.

Actions #7

Updated by Joshua M. Clulow about 1 month ago

To clarify, our expectations are that this will not result in any changes to /dev or /devices paths for disks. As per Garrett's mail:

I think my concern was misguided. I don’t think this causes any of the links or device paths to change, because the SATA drives don’t enumerate using DDI_NT_BLOCK_FABRIC or DDI_NT_BLOCK_SAS … they just use DDI_NT_BLOCK_CHAN.

A little more context – I know that our Intel platforms still enumerate sata drives using this driver using cXtYdZ – where Y is a small integer like 0 or 1, instead of a large WWN based target number – which is what I was afraid would happen.

Actions #8

Updated by Garrett D'Amore about 1 month ago

  • Status changed from In Progress to Pending RTI
Actions #9

Updated by Electric Monk about 1 month ago

  • Status changed from Pending RTI to Closed
  • % Done changed from 80 to 100

git commit 8118bef4ce6388ad51cc4ab94dbedc03621ee1e3

commit  8118bef4ce6388ad51cc4ab94dbedc03621ee1e3
Author: Garrett D'Amore <garrett@damore.org>
Date:   2022-07-07T21:43:42.000Z

    14765 SATL could decode page 83 for device IDs
    Reviewed by: Hans Rosenfeld <rosenfeld@grumpf.hope-2000.org>
    Reviewed by: Andy Fiddaman <andy@omnios.org>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Jerry Jelinek <gjelinek@gmail.com>
    Reviewed by: Gordon Ross <gordon.w.ross@gmail.com>
    Reviewed by: Albert Lee <trisk@forkgnu.org>
    Reviewed by: Andrew Stormont <andyjstormont@gmail.com>
    Approved by: Approved by: Joshua M. Clulow <josh@sysmgr.org>

Actions

Also available in: Atom PDF