qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] hw/block/nvme: align with existing style


From: Klaus Jensen
Subject: Re: [PATCH v3] hw/block/nvme: align with existing style
Date: Fri, 16 Apr 2021 07:28:58 +0200

On Apr 16 09:22, Gollu Appalanaidu wrote:
Use lower case hexadecimal format for the constants and in
comments use the same format as used in Spec. ("FFFFFFFFh")

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
-v3: Add Suggestions (Philippe)
Describe the NVMe subsystem style in nvme.c header

-v2: Address review comments (Klaus)
use lower case hexa format for the code and in comments
use the same format as used in Spec. ("FFFFFFFFh")

hw/block/nvme-ns.c   |  2 +-
hw/block/nvme.c      | 46 +++++++++++++++++++++++++-------------------
include/block/nvme.h | 10 +++++-----
3 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 7bb618f182..a0895614d9 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -303,7 +303,7 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)

    id_ns_z = g_malloc0(sizeof(NvmeIdNsZoned));

-    /* MAR/MOR are zeroes-based, 0xffffffff means no limit */
+    /* MAR/MOR are zeroes-based, FFFFFFFFFh means no limit */
    id_ns_z->mar = cpu_to_le32(ns->params.max_active_zones - 1);
    id_ns_z->mor = cpu_to_le32(ns->params.max_open_zones - 1);
    id_ns_z->zoc = 0;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 624a1431d0..cbe7f33daa 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -134,6 +134,12 @@
 *         Setting this property to true enables Read Across Zone Boundaries.
 */

+/**
+ * While QEMU coding style prefers lowercase hexadecimal in constants,
+ * the NVMe subsystem use the format from the NVMe specifications in
+ * the comments: no '0x' prefix, uppercase, 'h' hexadecimal suffix
+ */
+
#include "qemu/osdep.h"
#include "qemu/units.h"
#include "qemu/error-report.h"
@@ -3607,18 +3613,18 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest 
*req)

    /*
     * In the base NVM command set, Flush may apply to all namespaces
-     * (indicated by NSID being set to 0xFFFFFFFF). But if that feature is used
+     * (indicated by NSID being set to FFFFFFFFh). But if that feature is used
     * along with TP 4056 (Namespace Types), it may be pretty screwed up.
     *
-     * If NSID is indeed set to 0xFFFFFFFF, we simply cannot associate the
+     * If NSID is indeed set to FFFFFFFFh, we simply cannot associate the
     * opcode with a specific command since we cannot determine a unique I/O
     * command set. Opcode 0x0 could have any other meaning than something
     * equivalent to flushing and say it DOES have completely different
-     * semantics in some other command set - does an NSID of 0xFFFFFFFF then
+     * semantics in some other command set - does an NSID of FFFFFFFFh then
     * mean "for all namespaces, apply whatever command set specific command
     * that uses the 0x0 opcode?" Or does it mean "for all namespaces, apply
     * whatever command that uses the 0x0 opcode if, and only if, it allows
-     * NSID to be 0xFFFFFFFF"?
+     * NSID to be FFFFFFFFh"?
     *
     * Anyway (and luckily), for now, we do not care about this since the
     * device only supports namespace types that includes the NVM Flush command
@@ -3934,7 +3940,7 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
            NVME_CHANGED_NSID_SIZE) {
        /*
         * If more than 1024 namespaces, the first entry in the log page should
-         * be set to 0xffffffff and the others to 0 as spec.
+         * be set to FFFFFFFFh and the others to 0 as spec.
         */
        if (i == ARRAY_SIZE(nslist)) {
            memset(nslist, 0x0, sizeof(nslist));
@@ -4332,7 +4338,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeRequest *req,
    trace_pci_nvme_identify_nslist(min_nsid);

    /*
-     * Both 0xffffffff (NVME_NSID_BROADCAST) and 0xfffffffe are invalid values
+     * Both FFFFFFFFh (NVME_NSID_BROADCAST) and FFFFFFFFEh are invalid values
     * since the Active Namespace ID List should return namespaces with ids
     * *higher* than the NSID specified in the command. This is also specified
     * in the spec (NVM Express v1.3d, Section 5.15.4).
@@ -4379,7 +4385,7 @@ static uint16_t nvme_identify_nslist_csi(NvmeCtrl *n, 
NvmeRequest *req,
    trace_pci_nvme_identify_nslist_csi(min_nsid, c->csi);

    /*
-     * Same as in nvme_identify_nslist(), 0xffffffff/0xfffffffe are invalid.
+     * Same as in nvme_identify_nslist(), FFFFFFFFh/FFFFFFFFEh are invalid.
     */
    if (min_nsid >= NVME_NSID_BROADCAST - 1) {
        return NVME_INVALID_NSID | NVME_DNR;
@@ -4595,7 +4601,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest 
*req)
            /*
             * The Reservation Notification Mask and Reservation Persistence
             * features require a status code of Invalid Field in Command when
-             * NSID is 0xFFFFFFFF. Since the device does not support those
+             * NSID is FFFFFFFFh. Since the device does not support those
             * features we can always return Invalid Namespace or Format as we
             * should do for all other features.
             */
@@ -4854,8 +4860,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest 
*req)
            return NVME_INVALID_FIELD | NVME_DNR;
        }

-        trace_pci_nvme_setfeat_numq((dw11 & 0xFFFF) + 1,
-                                    ((dw11 >> 16) & 0xFFFF) + 1,
+        trace_pci_nvme_setfeat_numq((dw11 & 0xffff) + 1,
+                                    ((dw11 >> 16) & 0xffff) + 1,
                                    n->params.max_ioqpairs,
                                    n->params.max_ioqpairs);
        req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
@@ -5493,7 +5499,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
            n->bar.cc = data;
        }
        break;
-    case 0x1C:  /* CSTS */
+    case 0x1c:  /* CSTS */
        if (data & (1 << 4)) {
            NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
                           "attempted to W1C CSTS.NSSRO"
@@ -5505,7 +5511,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
        }
        break;
    case 0x20:  /* NSSR */
-        if (data == 0x4E564D65) {
+        if (data == 0x4e564d65) {
            trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
        } else {
            /* The spec says that writes of other values have no effect */
@@ -5575,11 +5581,11 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
        n->bar.cmbmsc = (n->bar.cmbmsc & 0xffffffff) | (data << 32);
        return;

-    case 0xE00: /* PMRCAP */
+    case 0xe00: /* PMRCAP */
        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrcap_readonly,
                       "invalid write to PMRCAP register, ignored");
        return;
-    case 0xE04: /* PMRCTL */
+    case 0xe04: /* PMRCTL */
        n->bar.pmrctl = data;
        if (NVME_PMRCTL_EN(data)) {
            memory_region_set_enabled(&n->pmr.dev->mr, true);
@@ -5590,19 +5596,19 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
            n->pmr.cmse = false;
        }
        return;
-    case 0xE08: /* PMRSTS */
+    case 0xe08: /* PMRSTS */
        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrsts_readonly,
                       "invalid write to PMRSTS register, ignored");
        return;
-    case 0xE0C: /* PMREBS */
+    case 0xe0C: /* PMREBS */
        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrebs_readonly,
                       "invalid write to PMREBS register, ignored");
        return;
-    case 0xE10: /* PMRSWTP */
+    case 0xe10: /* PMRSWTP */
        NVME_GUEST_ERR(pci_nvme_ub_mmiowr_pmrswtp_readonly,
                       "invalid write to PMRSWTP register, ignored");
        return;
-    case 0xE14: /* PMRMSCL */
+    case 0xe14: /* PMRMSCL */
        if (!NVME_CAP_PMRS(n->bar.cap)) {
            return;
        }
@@ -5622,7 +5628,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
uint64_t data,
        }

        return;
-    case 0xE18: /* PMRMSCU */
+    case 0xe18: /* PMRMSCU */
        if (!NVME_CAP_PMRS(n->bar.cap)) {
            return;
        }
@@ -5664,7 +5670,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, 
unsigned size)
         * from PMRSTS should ensure prior writes
         * made it to persistent media
         */
-        if (addr == 0xE08 &&
+        if (addr == 0xe08 &&
            (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
            memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
        }
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 4ac926fbc6..0739e0d665 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -848,8 +848,8 @@ enum NvmeStatusCodes {
    NVME_FW_REQ_SUSYSTEM_RESET  = 0x0110,
    NVME_NS_ALREADY_ATTACHED    = 0x0118,
    NVME_NS_PRIVATE             = 0x0119,
-    NVME_NS_NOT_ATTACHED        = 0x011A,
-    NVME_NS_CTRL_LIST_INVALID   = 0x011C,
+    NVME_NS_NOT_ATTACHED        = 0x011a,
+    NVME_NS_CTRL_LIST_INVALID   = 0x011c,
    NVME_CONFLICTING_ATTRS      = 0x0180,
    NVME_INVALID_PROT_INFO      = 0x0181,
    NVME_WRITE_TO_RO            = 0x0182,
@@ -1409,9 +1409,9 @@ typedef enum NvmeZoneState {
    NVME_ZONE_STATE_IMPLICITLY_OPEN  = 0x02,
    NVME_ZONE_STATE_EXPLICITLY_OPEN  = 0x03,
    NVME_ZONE_STATE_CLOSED           = 0x04,
-    NVME_ZONE_STATE_READ_ONLY        = 0x0D,
-    NVME_ZONE_STATE_FULL             = 0x0E,
-    NVME_ZONE_STATE_OFFLINE          = 0x0F,
+    NVME_ZONE_STATE_READ_ONLY        = 0x0d,
+    NVME_ZONE_STATE_FULL             = 0x0e,
+    NVME_ZONE_STATE_OFFLINE          = 0x0f,
} NvmeZoneState;

static inline void _nvme_check_size(void)
--
2.17.1


Looks good. I'll fixup a couple of missing constants in the comments (e.g. 0x0 -> 0h) when merged.

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]