[PATCH] nvme: fix return type of nvme_poll()

[PATCH] nvme: fix return type of nvme_poll()

[Yongsoo Joo]

Change the type of the return variable 'found' from bool to int so
that any interested caller can see not only whether at least one
CQE was processed, but also the total number of CQEs processed.

Signed-off-by: Yongsoo Joo <ysjoo@kookmin.ac.kr>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1a5ba80f1811..a51869899385 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1209,7 +1209,7 @@ static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 static int nvme_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 {
 	struct nvme_queue *nvmeq = hctx->driver_data;
-	bool found;
+	int found;
 
 	if (!nvme_cqe_pending(nvmeq))
 		return 0;
-- 
2.34.1


-- 

Re: [PATCH] nvme: fix return type of nvme_poll()

[Keith Busch]

On Sat, Dec 07, 2024 at 04:55:46AM +0000, Yongsoo Joo wrote:
> Change the type of the return variable 'found' from bool to int so
> that any interested caller can see not only whether at least one
> CQE was processed, but also the total number of CQEs processed.

It looks like all the interested callers only care if it's not 0, so I'm
not sure this is a "fix" as the subject says.

Re: [PATCH] nvme: fix return type of nvme_poll()

[Yongsoo Joo]

On Sat, Dec 07, 2024 at 08:13:12PM -0700, Keith Busch wrote:
> On Sat, Dec 07, 2024 at 04:55:46AM +0000, Yongsoo Joo wrote:
> > Change the type of the return variable 'found' from bool to int so
> > that any interested caller can see not only whether at least one
> > CQE was processed, but also the total number of CQEs processed.
> 
> It looks like all the interested callers only care if it's not 0, so I'm
> not sure this is a "fix" as the subject says.

I agree that all the "current" interested callers seem to expect 
boolean-like return values from nvme_poll(). However, the return 
type of both nvme_poll() and nvme_poll_cq() is int, and 
nvme_poll_cq() actually returns the number of processed CQEs.

The inconsistency between the two functions is, in my view, worth 
addressing to prevent potential confusion for potential callers who 
are interested the actual numbers. And this patch is 
backward-compatible with the current usage, so no changes are 
required for the existing callers.

I’d like to ask for your opinion on the followings:
- Do you think rephrasing the patch title to “nvme: change return 
  type of nvme_poll()” would be more appropriate?
- Do you believe it would make more sense to change the return 
  type of nvme_poll() from int to bool instead?
- Or, do you think this inconsistency is not a big issue, and the 
  current implementation is sufficient as it stands?

Thanks for your time and feedback. I look forward to your thoughts.


-- 

Re: [PATCH] nvme: fix return type of nvme_poll()

[Christoph Hellwig]

On Sun, Dec 08, 2024 at 06:24:30AM +0000, Yongsoo Joo wrote:
> I agree that all the "current" interested callers seem to expect 
> boolean-like return values from nvme_poll(). However, the return 
> type of both nvme_poll() and nvme_poll_cq() is int, and 
> nvme_poll_cq() actually returns the number of processed CQEs.

Maybe add a patch to switch the poll method to return bool instead?

Re: [PATCH] nvme: fix return type of nvme_poll()

[Yongsoo Joo]

On Mon, Dec 09, 2024 at 08:58:36AM +0100, Christoph Hellwig wrote:
> On Sun, Dec 08, 2024 at 06:24:30AM +0000, Yongsoo Joo wrote:
> > I agree that all the "current" interested callers seem to expect 
> > boolean-like return values from nvme_poll(). However, the return 
> > type of both nvme_poll() and nvme_poll_cq() is int, and 
> > nvme_poll_cq() actually returns the number of processed CQEs.
> 
> Maybe add a patch to switch the poll method to return bool instead?
>
Thank you for your suggestion. I will submit a revised patch soon,
addressing the concern using the word 'fix' and the suggestion to 
return bool.


--