diff mbox series

[17/25] block/nvme: Simplify nvme_cmd_sync()

Message ID 20201027135547.374946-18-philmd@redhat.com
State New
Headers show
Series block/nvme: Fix Aarch64 host | expand

Commit Message

Philippe Mathieu-Daudé Oct. 27, 2020, 1:55 p.m. UTC 
As all commands use the ADMIN queue, it is pointless to pass
it as argument each time. Remove the argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Eric Auger Oct. 28, 2020, 2:27 p.m. UTC | #1 
On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote:
> As all commands use the ADMIN queue, it is pointless to pass
> it as argument each time. Remove the argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  block/nvme.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2d3648694b0..68f0c3f7959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>      aio_wait_kick();
>  }
>  
> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> -                         NvmeCmd *cmd)
> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
>  {
> +    BDRVNVMeState *s = bs->opaque;
> +    NVMeQueuePair *q = s->queues[INDEX_ADMIN];
>      AioContext *aio_context = bdrv_get_aio_context(bs);
>      NVMeRequest *req;
>      int ret = -EINPROGRESS;
> @@ -534,7 +535,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  
>      memset(id, 0, sizeof(*id));
>      cmd.dptr.prp1 = cpu_to_le64(iova);
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to identify controller");
>          goto out;
>      }
> @@ -557,7 +558,7 @@ static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      memset(id, 0, sizeof(*id));
>      cmd.cdw10 = 0;
>      cmd.nsid = cpu_to_le32(namespace);
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to identify namespace");
>          goto out;
>      }
> @@ -663,7 +664,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
>          .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to create CQ io queue [%u]", n);
>          goto out_error;
>      }
> @@ -673,7 +674,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
>          .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> +    if (nvme_cmd_sync(bs, &cmd)) {
>          error_setg(errp, "Failed to create SQ io queue [%u]", n);
>          goto out_error;
>      }
> @@ -889,7 +890,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
>          .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
>      };
>  
> -    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
> +    ret = nvme_cmd_sync(bs, &cmd);
>      if (ret) {
>          error_setg(errp, "Failed to configure NVMe write cache");
>      }
>
Stefan Hajnoczi Oct. 28, 2020, 3:21 p.m. UTC | #2 
On Tue, Oct 27, 2020 at 02:55:39PM +0100, Philippe Mathieu-Daudé wrote:
> As all commands use the ADMIN queue, it is pointless to pass
> it as argument each time. Remove the argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2d3648694b0..68f0c3f7959 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>      aio_wait_kick();
>  }
>  
> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> -                         NvmeCmd *cmd)
> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)

Please rename the function to nvme_admin_cmd_sync() so it's behavior is
clear.
Philippe Mathieu-Daudé Oct. 29, 2020, 7:35 a.m. UTC | #3 
On 10/28/20 4:21 PM, Stefan Hajnoczi wrote:
> On Tue, Oct 27, 2020 at 02:55:39PM +0100, Philippe Mathieu-Daudé wrote:
>> As all commands use the ADMIN queue, it is pointless to pass
>> it as argument each time. Remove the argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 2d3648694b0..68f0c3f7959 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -487,9 +487,10 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>>      aio_wait_kick();
>>  }
>>  
>> -static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>> -                         NvmeCmd *cmd)
>> +static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
> 
> Please rename the function to nvme_admin_cmd_sync() so it's behavior is
> clear.

Good idea :)
diff mbox series

Patch

diff --git a/block/nvme.c b/block/nvme.c
index 2d3648694b0..68f0c3f7959 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -487,9 +487,10 @@  static void nvme_cmd_sync_cb(void *opaque, int ret)
     aio_wait_kick();
 }
 
-static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
-                         NvmeCmd *cmd)
+static int nvme_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
 {
+    BDRVNVMeState *s = bs->opaque;
+    NVMeQueuePair *q = s->queues[INDEX_ADMIN];
     AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     int ret = -EINPROGRESS;
@@ -534,7 +535,7 @@  static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 
     memset(id, 0, sizeof(*id));
     cmd.dptr.prp1 = cpu_to_le64(iova);
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
     }
@@ -557,7 +558,7 @@  static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     memset(id, 0, sizeof(*id));
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to identify namespace");
         goto out;
     }
@@ -663,7 +664,7 @@  static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
         .cdw11 = cpu_to_le32(NVME_CQ_IEN | NVME_CQ_PC),
     };
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to create CQ io queue [%u]", n);
         goto out_error;
     }
@@ -673,7 +674,7 @@  static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | n),
         .cdw11 = cpu_to_le32(NVME_SQ_PC | (n << 16)),
     };
-    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
+    if (nvme_cmd_sync(bs, &cmd)) {
         error_setg(errp, "Failed to create SQ io queue [%u]", n);
         goto out_error;
     }
@@ -889,7 +890,7 @@  static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
         .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
     };
 
-    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
+    ret = nvme_cmd_sync(bs, &cmd);
     if (ret) {
         error_setg(errp, "Failed to configure NVMe write cache");
     }