[PATCH 0/4] nvmet pci-epf fixes

[PATCH 0/4] nvmet pci-epf fixes

[Damien Le Moal]

Four patches to fix and improve IRQ handling in the PCI endpoint target
driver. The issues fixed by the first 2 patches where discovered with
testing polling queues (set on the host side). The following 2 patches
are small improvements/cleanup in the same IRQ handling area.

Damien Le Moal (4):
  nvmet: pci-epf: Clear completion queue IRQ flag on delete
  nvmet: pci-epf: Do not fall back to using INTX if not supported
  nvmet: pci-epf: Cleanup nvmet_pci_epf_raise_irq()
  nvmet: pci-epf: Improve debug message

 drivers/nvme/target/pci-epf.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

-- 
2.49.0

[PATCH 1/4] nvmet: pci-epf: Clear completion queue IRQ flag on delete

[Damien Le Moal]

The function nvmet_pci_epf_delete_cq() unconditionnally calls
nvmet_pci_epf_remove_irq_vector() even for completion queues that do not
have interrupts enabled. Furthermore, for completion queues that do
have IRQ enabled, deleting and re-creating the completion queue leaves
the flag NVMET_PCI_EPF_Q_IRQ_ENABLED set, even if the completion queue
is being re-created with IRQ disabled.

Fix these issues by calling nvmet_pci_epf_remove_irq_vector() only if
NVMET_PCI_EPF_Q_IRQ_ENABLED is set and make sure to always clear that
flag.

Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index 7fab7f3d79b7..d5442991f2fb 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -1344,7 +1344,8 @@ static u16 nvmet_pci_epf_delete_cq(struct nvmet_ctrl *tctrl, u16 cqid)
 
 	cancel_delayed_work_sync(&cq->work);
 	nvmet_pci_epf_drain_queue(cq);
-	nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
+	if (test_and_clear_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
+		nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
 	nvmet_pci_epf_mem_unmap(ctrl->nvme_epf, &cq->pci_map);
 
 	return NVME_SC_SUCCESS;
-- 
2.49.0

[PATCH 2/4] nvmet: pci-epf: Do not fall back to using INTX if not supported

[Damien Le Moal]

Some endpoint PCIe controllers do not support raising legacy INTX
interrupts. This support is indicated by the intx_capable field of
struct pci_epc_features. Modify nvmet_pci_epf_raise_irq() to not
automatically fallback to trying raising an INTX interrupt after an MSI
or MSI-X error if the controller does not support INTX.

Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index d5442991f2fb..859953041da8 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -636,14 +636,16 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
 	switch (nvme_epf->irq_type) {
 	case PCI_IRQ_MSIX:
 	case PCI_IRQ_MSI:
+		/*
+		 * If we fail to raise an MSI or MSI-X interrupr, it is likely
+		 * because the host is using legacy INTX IRQs (e.g. BIOS,
+		 * grub). But we can fallback to the INTX type only if the
+		 * endpoint controller supports this type.
+		 */
 		ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
 					nvme_epf->irq_type, cq->vector + 1);
-		if (!ret)
+		if (!ret || !nvme_epf->epc_features->intx_capable)
 			break;
-		/*
-		 * If we got an error, it is likely because the host is using
-		 * legacy IRQs (e.g. BIOS, grub).
-		 */
 		fallthrough;
 	case PCI_IRQ_INTX:
 		ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
-- 
2.49.0

[PATCH 3/4] nvmet: pci-epf: Cleanup nvmet_pci_epf_raise_irq()

[Damien Le Moal]

There is no point in taking the controller irq_lock and calling
nvmet_pci_epf_should_raise_irq() for a completion queue which does not
have IRQ enabled (NVMET_PCI_EPF_Q_IRQ_ENABLED flag is not set).
Move the test for the NVMET_PCI_EPF_Q_IRQ_ENABLED flag out of
nvmet_pci_epf_should_raise_irq() to the top of nvmet_pci_epf_raise_irq()
to return early when no IRQ should be raised.

Also, use dev_err_ratelimited() to avoid a message storm under load when
raising IRQs is failing.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index 859953041da8..34dd73d6457c 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -596,9 +596,6 @@ static bool nvmet_pci_epf_should_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
 	struct nvmet_pci_epf_irq_vector *iv = cq->iv;
 	bool ret;
 
-	if (!test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
-		return false;
-
 	/* IRQ coalescing for the admin queue is not allowed. */
 	if (!cq->qid)
 		return true;
@@ -625,7 +622,8 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
 	struct pci_epf *epf = nvme_epf->epf;
 	int ret = 0;
 
-	if (!test_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags))
+	if (!test_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags) ||
+	    !test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
 		return;
 
 	mutex_lock(&ctrl->irq_lock);
@@ -658,7 +656,9 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
 	}
 
 	if (ret)
-		dev_err(ctrl->dev, "Failed to raise IRQ (err=%d)\n", ret);
+		dev_err_ratelimited(ctrl->dev,
+				    "CQ[%u]: Failed to raise IRQ (err=%d)\n",
+				    cq->qid, ret);
 
 unlock:
 	mutex_unlock(&ctrl->irq_lock);
-- 
2.49.0

[PATCH 4/4] nvmet: pci-epf: Improve debug message

[Damien Le Moal]

Improve the debug message of nvmet_pci_epf_create_cq() to indicate if a
completion queue IRQ is disabled.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/nvme/target/pci-epf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
index 34dd73d6457c..bd79e059e066 100644
--- a/drivers/nvme/target/pci-epf.c
+++ b/drivers/nvme/target/pci-epf.c
@@ -1321,8 +1321,14 @@ static u16 nvmet_pci_epf_create_cq(struct nvmet_ctrl *tctrl,
 
 	set_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags);
 
-	dev_dbg(ctrl->dev, "CQ[%u]: %u entries of %zu B, IRQ vector %u\n",
-		cqid, qsize, cq->qes, cq->vector);
+	if (test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
+		dev_dbg(ctrl->dev,
+			"CQ[%u]: %u entries of %zu B, IRQ vector %u\n",
+			cqid, qsize, cq->qes, cq->vector);
+	else
+		dev_dbg(ctrl->dev,
+			"CQ[%u]: %u entries of %zu B, IRQ disabled\n",
+			cqid, qsize, cq->qes);
 
 	return NVME_SC_SUCCESS;
 
-- 
2.49.0

Re: [PATCH 1/4] nvmet: pci-epf: Clear completion queue IRQ flag on delete

[Niklas Cassel]

On Thu, May 08, 2025 at 03:57:42PM +0900, Damien Le Moal wrote:
> The function nvmet_pci_epf_delete_cq() unconditionnally calls

s/unconditionnally/unconditionally/


> nvmet_pci_epf_remove_irq_vector() even for completion queues that do not
> have interrupts enabled. Furthermore, for completion queues that do
> have IRQ enabled, deleting and re-creating the completion queue leaves
> the flag NVMET_PCI_EPF_Q_IRQ_ENABLED set, even if the completion queue
> is being re-created with IRQ disabled.
> 
> Fix these issues by calling nvmet_pci_epf_remove_irq_vector() only if
> NVMET_PCI_EPF_Q_IRQ_ENABLED is set and make sure to always clear that
> flag.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 7fab7f3d79b7..d5442991f2fb 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1344,7 +1344,8 @@ static u16 nvmet_pci_epf_delete_cq(struct nvmet_ctrl *tctrl, u16 cqid)
>  
>  	cancel_delayed_work_sync(&cq->work);
>  	nvmet_pci_epf_drain_queue(cq);
> -	nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
> +	if (test_and_clear_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
> +		nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);

I agree that we should only call nvmet_pci_epf_remove_irq_vector() if
NVMET_PCI_EPF_Q_IRQ_ENABLED is set.

However, it looks a bit weird to explicitly only clear only this flag bit.
What about the other flag bits?

I would have expected a memset() of flags, or flags = 0; in either
.create_cq/.create_sq, or in .delete_cq/.delete_sq.

And it also seems that flag NVMET_PCI_EPF_Q_IS_SQ is unused, so that flag
can probably be dropped.


Kind regards,
Niklas

Re: [PATCH 2/4] nvmet: pci-epf: Do not fall back to using INTX if not supported

[Niklas Cassel]

On Thu, May 08, 2025 at 03:57:43PM +0900, Damien Le Moal wrote:
> Some endpoint PCIe controllers do not support raising legacy INTX
> interrupts. This support is indicated by the intx_capable field of
> struct pci_epc_features. Modify nvmet_pci_epf_raise_irq() to not
> automatically fallback to trying raising an INTX interrupt after an MSI
> or MSI-X error if the controller does not support INTX.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Hm..

0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver") was first
included in v6.14-rc1.

4b313c69a38e ("PCI: endpoint: Add intx_capable to epc_features struct") was first
included in v6.15-rc1.

Perhaps add a:
Cc: stable+noautosel@kernel.org # depends on patch introducing intx_capable flag

So that it is not automatically backported.


> ---
>  drivers/nvme/target/pci-epf.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index d5442991f2fb..859953041da8 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -636,14 +636,16 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
>  	switch (nvme_epf->irq_type) {
>  	case PCI_IRQ_MSIX:
>  	case PCI_IRQ_MSI:
> +		/*
> +		 * If we fail to raise an MSI or MSI-X interrupr, it is likely

s/interrupr/interrupt/


> +		 * because the host is using legacy INTX IRQs (e.g. BIOS,
> +		 * grub). But we can fallback to the INTX type only if the

s/. But/, but/


> +		 * endpoint controller supports this type.
> +		 */
>  		ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
>  					nvme_epf->irq_type, cq->vector + 1);
> -		if (!ret)
> +		if (!ret || !nvme_epf->epc_features->intx_capable)
>  			break;
> -		/*
> -		 * If we got an error, it is likely because the host is using
> -		 * legacy IRQs (e.g. BIOS, grub).
> -		 */
>  		fallthrough;
>  	case PCI_IRQ_INTX:
>  		ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
> -- 
> 2.49.0
> 
> 

Re: [PATCH 4/4] nvmet: pci-epf: Improve debug message

[Niklas Cassel]

On Thu, May 08, 2025 at 03:57:45PM +0900, Damien Le Moal wrote:
> Improve the debug message of nvmet_pci_epf_create_cq() to indicate if a
> completion queue IRQ is disabled.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 34dd73d6457c..bd79e059e066 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1321,8 +1321,14 @@ static u16 nvmet_pci_epf_create_cq(struct nvmet_ctrl *tctrl,
>  
>  	set_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags);
>  
> -	dev_dbg(ctrl->dev, "CQ[%u]: %u entries of %zu B, IRQ vector %u\n",
> -		cqid, qsize, cq->qes, cq->vector);
> +	if (test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
> +		dev_dbg(ctrl->dev,
> +			"CQ[%u]: %u entries of %zu B, IRQ vector %u\n",
> +			cqid, qsize, cq->qes, cq->vector);
> +	else
> +		dev_dbg(ctrl->dev,
> +			"CQ[%u]: %u entries of %zu B, IRQ disabled\n",
> +			cqid, qsize, cq->qes);
>  
>  	return NVME_SC_SUCCESS;
>  
> -- 
> 2.49.0
> 
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 3/4] nvmet: pci-epf: Cleanup nvmet_pci_epf_raise_irq()

[Niklas Cassel]

On Thu, May 08, 2025 at 03:57:44PM +0900, Damien Le Moal wrote:
> There is no point in taking the controller irq_lock and calling
> nvmet_pci_epf_should_raise_irq() for a completion queue which does not
> have IRQ enabled (NVMET_PCI_EPF_Q_IRQ_ENABLED flag is not set).
> Move the test for the NVMET_PCI_EPF_Q_IRQ_ENABLED flag out of
> nvmet_pci_epf_should_raise_irq() to the top of nvmet_pci_epf_raise_irq()
> to return early when no IRQ should be raised.
> 
> Also, use dev_err_ratelimited() to avoid a message storm under load when
> raising IRQs is failing.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 859953041da8..34dd73d6457c 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -596,9 +596,6 @@ static bool nvmet_pci_epf_should_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
>  	struct nvmet_pci_epf_irq_vector *iv = cq->iv;
>  	bool ret;
>  
> -	if (!test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
> -		return false;
> -
>  	/* IRQ coalescing for the admin queue is not allowed. */
>  	if (!cq->qid)
>  		return true;
> @@ -625,7 +622,8 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
>  	struct pci_epf *epf = nvme_epf->epf;
>  	int ret = 0;
>  
> -	if (!test_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags))
> +	if (!test_bit(NVMET_PCI_EPF_Q_LIVE, &cq->flags) ||
> +	    !test_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
>  		return;
>  
>  	mutex_lock(&ctrl->irq_lock);
> @@ -658,7 +656,9 @@ static void nvmet_pci_epf_raise_irq(struct nvmet_pci_epf_ctrl *ctrl,
>  	}
>  
>  	if (ret)
> -		dev_err(ctrl->dev, "Failed to raise IRQ (err=%d)\n", ret);
> +		dev_err_ratelimited(ctrl->dev,
> +				    "CQ[%u]: Failed to raise IRQ (err=%d)\n",
> +				    cq->qid, ret);
>  
>  unlock:
>  	mutex_unlock(&ctrl->irq_lock);
> -- 
> 2.49.0
> 
> 

Nit: since we no longer perform the NVMET_PCI_EPF_Q_IRQ_ENABLED check in
nvmet_pci_epf_should_raise_irq(), should we rename the function to something
else? nvmet_pci_epf_irq_allowed() ? Any ideas ?

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 1/4] nvmet: pci-epf: Clear completion queue IRQ flag on delete

[Damien Le Moal]

On 5/8/25 10:07 PM, Niklas Cassel wrote:
> On Thu, May 08, 2025 at 03:57:42PM +0900, Damien Le Moal wrote:
>> The function nvmet_pci_epf_delete_cq() unconditionnally calls
> 
> s/unconditionnally/unconditionally/
> 
> 
>> nvmet_pci_epf_remove_irq_vector() even for completion queues that do not
>> have interrupts enabled. Furthermore, for completion queues that do
>> have IRQ enabled, deleting and re-creating the completion queue leaves
>> the flag NVMET_PCI_EPF_Q_IRQ_ENABLED set, even if the completion queue
>> is being re-created with IRQ disabled.
>>
>> Fix these issues by calling nvmet_pci_epf_remove_irq_vector() only if
>> NVMET_PCI_EPF_Q_IRQ_ENABLED is set and make sure to always clear that
>> flag.
>>
>> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/nvme/target/pci-epf.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
>> index 7fab7f3d79b7..d5442991f2fb 100644
>> --- a/drivers/nvme/target/pci-epf.c
>> +++ b/drivers/nvme/target/pci-epf.c
>> @@ -1344,7 +1344,8 @@ static u16 nvmet_pci_epf_delete_cq(struct nvmet_ctrl *tctrl, u16 cqid)
>>  
>>  	cancel_delayed_work_sync(&cq->work);
>>  	nvmet_pci_epf_drain_queue(cq);
>> -	nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
>> +	if (test_and_clear_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
>> +		nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
> 
> I agree that we should only call nvmet_pci_epf_remove_irq_vector() if
> NVMET_PCI_EPF_Q_IRQ_ENABLED is set.
> 
> However, it looks a bit weird to explicitly only clear only this flag bit.
> What about the other flag bits?

NVMET_PCI_EPF_Q_LIVE is cleared in the same function in the same manner. The
only other flag is NVMET_PCI_EPF_Q_IRQ_ENABLED, which this patch clears.

> I would have expected a memset() of flags, or flags = 0; in either
> .create_cq/.create_sq, or in .delete_cq/.delete_sq.
> 
> And it also seems that flag NVMET_PCI_EPF_Q_IS_SQ is unused, so that flag
> can probably be dropped.

Good catch. Will add a patch to drop it.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/4] nvmet: pci-epf: Do not fall back to using INTX if not supported

[Damien Le Moal]

On 5/8/25 10:07 PM, Niklas Cassel wrote:
> On Thu, May 08, 2025 at 03:57:43PM +0900, Damien Le Moal wrote:
>> Some endpoint PCIe controllers do not support raising legacy INTX
>> interrupts. This support is indicated by the intx_capable field of
>> struct pci_epc_features. Modify nvmet_pci_epf_raise_irq() to not
>> automatically fallback to trying raising an INTX interrupt after an MSI
>> or MSI-X error if the controller does not support INTX.
>>
>> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Hm..
> 
> 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver") was first
> included in v6.14-rc1.
> 
> 4b313c69a38e ("PCI: endpoint: Add intx_capable to epc_features struct") was first
> included in v6.15-rc1.
> 
> Perhaps add a:
> Cc: stable+noautosel@kernel.org # depends on patch introducing intx_capable flag
> 
> So that it is not automatically backported.

Good catch. Will drop the Fixes tag. No point in it.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] nvmet: pci-epf: Cleanup nvmet_pci_epf_raise_irq()

[Damien Le Moal]

On 5/8/25 10:07 PM, Niklas Cassel wrote:
> Nit: since we no longer perform the NVMET_PCI_EPF_Q_IRQ_ENABLED check in
> nvmet_pci_epf_should_raise_irq(), should we rename the function to something
> else? nvmet_pci_epf_irq_allowed() ? Any ideas ?

nvmet_pci_epf_should_raise_irq() is still in charge of handling IRQ coalescing,
returning true if we should raise an IRQ and false if the IRQs are still being
coalesced. So the name as is still seems fine to me.

> Reviewed-by: Niklas Cassel <cassel@kernel.org>

Thanks.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/4] nvmet: pci-epf: Clear completion queue IRQ flag on delete

[Niklas Cassel]

On Fri, May 09, 2025 at 08:13:22AM +0900, Damien Le Moal wrote:
> On 5/8/25 10:07 PM, Niklas Cassel wrote:
> > On Thu, May 08, 2025 at 03:57:42PM +0900, Damien Le Moal wrote:
> >> The function nvmet_pci_epf_delete_cq() unconditionnally calls
> > 
> > s/unconditionnally/unconditionally/
> > 
> > 
> >> nvmet_pci_epf_remove_irq_vector() even for completion queues that do not
> >> have interrupts enabled. Furthermore, for completion queues that do
> >> have IRQ enabled, deleting and re-creating the completion queue leaves
> >> the flag NVMET_PCI_EPF_Q_IRQ_ENABLED set, even if the completion queue
> >> is being re-created with IRQ disabled.
> >>
> >> Fix these issues by calling nvmet_pci_epf_remove_irq_vector() only if
> >> NVMET_PCI_EPF_Q_IRQ_ENABLED is set and make sure to always clear that
> >> flag.
> >>
> >> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> ---
> >>  drivers/nvme/target/pci-epf.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> >> index 7fab7f3d79b7..d5442991f2fb 100644
> >> --- a/drivers/nvme/target/pci-epf.c
> >> +++ b/drivers/nvme/target/pci-epf.c
> >> @@ -1344,7 +1344,8 @@ static u16 nvmet_pci_epf_delete_cq(struct nvmet_ctrl *tctrl, u16 cqid)
> >>  
> >>  	cancel_delayed_work_sync(&cq->work);
> >>  	nvmet_pci_epf_drain_queue(cq);
> >> -	nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
> >> +	if (test_and_clear_bit(NVMET_PCI_EPF_Q_IRQ_ENABLED, &cq->flags))
> >> +		nvmet_pci_epf_remove_irq_vector(ctrl, cq->vector);
> > 
> > I agree that we should only call nvmet_pci_epf_remove_irq_vector() if
> > NVMET_PCI_EPF_Q_IRQ_ENABLED is set.
> > 
> > However, it looks a bit weird to explicitly only clear only this flag bit.
> > What about the other flag bits?
> 
> NVMET_PCI_EPF_Q_LIVE is cleared in the same function in the same manner. The
> only other flag is NVMET_PCI_EPF_Q_IRQ_ENABLED, which this patch clears.
> 
> > I would have expected a memset() of flags, or flags = 0; in either
> > .create_cq/.create_sq, or in .delete_cq/.delete_sq.

You patch looks fine, I was just thinking that it would be slightly more robust
to do flags = 0; in e.g. .create_cq/.create_sq, or .delete_cq/.delete_sq.

That way, we remove the possibility of ever introducing a similar bug to what
this patch fixes. (I.e if we ever add a new flag, we will not need to remember
to also add code that explicitly clears the bit for that new flag, since
flags = 0; clears all flags in one go.)


Kind regards,
Niklas