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

[Niklas Cassel <cassel@kernel.org>]

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>