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

2021-04-15 Thread Gollu Appalanaidu

On Thu, Apr 15, 2021 at 07:18:50PM +0200, Klaus Jensen wrote:

On Apr 15 15:13, Philippe Mathieu-Daudé wrote:

On 4/15/21 2:00 PM, Gollu Appalanaidu wrote:

Make uniform hexadecimal numbers format.

Signed-off-by: Gollu Appalanaidu 
---
-v2: Address review comments (Klaus)
use lower case hexa format for the code and in comments
use the same format as used in Spec. ("h")


^ This comment is relevant to the commit message.

Also it would be nice if the subsystem could describe somewhere
what is its style. Not sure where... The file header is probably
the simplest place.

Something like:

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



+1 for that suggestion.



Sure, will add it and send v3.


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








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

2021-04-15 Thread Klaus Jensen

On Apr 15 15:13, Philippe Mathieu-Daudé wrote:

On 4/15/21 2:00 PM, Gollu Appalanaidu wrote:

Make uniform hexadecimal numbers format.

Signed-off-by: Gollu Appalanaidu 
---
-v2: Address review comments (Klaus)
use lower case hexa format for the code and in comments
use the same format as used in Spec. ("h")


^ This comment is relevant to the commit message.

Also it would be nice if the subsystem could describe somewhere
what is its style. Not sure where... The file header is probably
the simplest place.

Something like:

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



+1 for that suggestion.


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





signature.asc
Description: PGP signature


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

2021-04-15 Thread Philippe Mathieu-Daudé
On 4/15/21 2:00 PM, Gollu Appalanaidu wrote:
> Make uniform hexadecimal numbers format.
> 
> Signed-off-by: Gollu Appalanaidu 
> ---
> -v2: Address review comments (Klaus)
> use lower case hexa format for the code and in comments 
> use the same format as used in Spec. ("h")

^ This comment is relevant to the commit message.

Also it would be nice if the subsystem could describe somewhere
what is its style. Not sure where... The file header is probably
the simplest place.

Something like:

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

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




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

2021-04-15 Thread Gollu Appalanaidu
Make uniform hexadecimal numbers format.

Signed-off-by: Gollu Appalanaidu 
---
-v2: Address review comments (Klaus)
use lower case hexa format for the code and in comments 
use the same format as used in Spec. ("h")


 hw/block/nvme-ns.c   |  2 +-
 hw/block/nvme.c  | 40 
 include/block/nvme.h | 10 +-
 3 files changed, 26 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, 0x means no limit */
+/* MAR/MOR are zeroes-based, Fh 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..f50e25c501 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3607,18 +3607,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 0x). But if that feature is used
+ * (indicated by NSID being set to h). But if that feature is used
  * along with TP 4056 (Namespace Types), it may be pretty screwed up.
  *
- * If NSID is indeed set to 0x, we simply cannot associate the
+ * If NSID is indeed set to h, 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 0x then
+ * semantics in some other command set - does an NSID of h 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 0x"?
+ * NSID to be h"?
  *
  * 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 +3934,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 0x and the others to 0 as spec.
+ * be set to h and the others to 0 as spec.
  */
 if (i == ARRAY_SIZE(nslist)) {
 memset(nslist, 0x0, sizeof(nslist));
@@ -4332,7 +4332,7 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeRequest *req,
 trace_pci_nvme_identify_nslist(min_nsid);
 
 /*
- * Both 0x (NVME_NSID_BROADCAST) and 0xfffe are invalid values
+ * Both h (NVME_NSID_BROADCAST) and Eh 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 +4379,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(), 0x/0xfffe are invalid.
+ * Same as in nvme_identify_nslist(), h/Eh are invalid.
  */
 if (min_nsid >= NVME_NSID_BROADCAST - 1) {
 return NVME_INVALID_NSID | NVME_DNR;
@@ -4595,7 +4595,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 0x. Since the device does not support those
+ * NSID is h. 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 +4854,8 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest 
*req)
 return NVME_INVALID_FIELD | NVME_DNR;
 }
 
-trace_pci_nvme_setfeat_numq((dw11 & 0x) + 1,
-((dw11 >> 16) & 0x) + 1,
+trace_pci_nvme_setfeat_numq((dw11 & 0x) + 1,
+((dw11 >> 16) & 0x) + 1,
 n->params.max_ioqpairs,
 n->params.max_ioqpairs);