[PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[Vincent Fu]

Return BLK_STS_OK instead of 0 on success. Since BLK_STS_OK is defined
as 0 this patch results in no functional change but to avoid confusion
and to be consistent with nvme_setup_write_zeroes as well as
nvme_setup_discard we should have nvme_setup_rw also return BLK_STS_OK.

Signed-off-by: Vincent Fu <vincent.fu@samsung.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b9b0fbde9..f45665e36 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -895,7 +895,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 
 	cmnd->rw.control = cpu_to_le16(control);
 	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
-	return 0;
+	return BLK_STS_OK;
 }
 
 void nvme_cleanup_cmd(struct request *req)
-- 
2.25.1

Re: [PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[Pankaj Raghav]

On Mon, Apr 18, 2022 at 07:43:50PM +0000, Vincent Fu wrote:
> Return BLK_STS_OK instead of 0 on success. Since BLK_STS_OK is defined
> as 0 this patch results in no functional change but to avoid confusion
> and to be consistent with nvme_setup_write_zeroes as well as
> nvme_setup_discard we should have nvme_setup_rw also return BLK_STS_OK.
> 
> Signed-off-by: Vincent Fu <vincent.fu@samsung.com>
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b9b0fbde9..f45665e36 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -895,7 +895,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
>  
>  	cmnd->rw.control = cpu_to_le16(control);
>  	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
> -	return 0;
> +	return BLK_STS_OK;
>  }
>  
>  void nvme_cleanup_cmd(struct request *req)
> -- 
> 2.25.1
> 
Small nit:
The commit line could be changed to:
nvme: return BLK_STS_OK on success in nvme_setup_rw

Otherwise, Looks good.
Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>

-- 
Pankaj Raghav

Re: [PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[Christoph Hellwig]

On Mon, Apr 18, 2022 at 07:43:50PM +0000, Vincent Fu wrote:
> Return BLK_STS_OK instead of 0 on success. Since BLK_STS_OK is defined
> as 0 this patch results in no functional change but to avoid confusion
> and to be consistent with nvme_setup_write_zeroes as well as
> nvme_setup_discard we should have nvme_setup_rw also return BLK_STS_OK.

I really don't see the point.  blk_status_t like all __bitwise types
is designed to be able to cope with a plain 0 just fine.

RE: [PATCH] nvme: have nvme_setup_rw return BLK_STS_OK

[Vincent Fu]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Thursday, April 28, 2022 11:08 AM
> To: Vincent Fu <vincent.fu@samsung.com>
> Cc: linux-nvme@lists.infradead.org
> Subject: Re: [PATCH] nvme: have nvme_setup_rw return BLK_STS_OK
> 
> On Mon, Apr 18, 2022 at 07:43:50PM +0000, Vincent Fu wrote:
> > Return BLK_STS_OK instead of 0 on success. Since BLK_STS_OK is
> defined
> > as 0 this patch results in no functional change but to avoid confusion
> > and to be consistent with nvme_setup_write_zeroes as well as
> > nvme_setup_discard we should have nvme_setup_rw also return
> BLK_STS_OK.
> 
> I really don't see the point.  blk_status_t like all __bitwise types
> is designed to be able to cope with a plain 0 just fine.

Thank you for the feedback. This is obviously a small cleanup but I found it
confusing that the other nvme_setup_* functions returned BLK_STS_OK whereas
nvme_setup_rw returned 0. The code is easier to understand if all of these
functions are consistent and this change saves people the trouble of making
sure that BLK_STS_OK is defined as 0.