Re: [PATCH v2 08/16] PCI: epf-test: Simplify IRQ test commands execution

[Manivannan Sadhasivam <mani@kernel.org>]

On Wed, Mar 08, 2023 at 06:03:05PM +0900, Damien Le Moal wrote:
> For the commands COMMAND_RAISE_LEGACY_IRQ, COMMAND_RAISE_MSI_IRQ and
> COMMAND_RAISE_MSIX_IRQ, the function pci_epf_test_cmd_handler()
> sets the STATUS_IRQ_RAISED status flag and calls the epc function
> pci_epc_raise_irq() directly. However, this is also exactly what the
> pci_epf_test_raise_irq() function does. Avoid duplicating these
> operations by directly using pci_epf_test_raise_irq() for the IRQ test
> commands. It is OK to do so as the host side endpoint test driver always
> set the correct irq type for the IRQ test commands.
> 
> At the same time, the irq number check done for the
> COMMAND_RAISE_MSI_IRQ and COMMAND_RAISE_MSIX_IRQ commands can also be
> moved to pci_epf_test_raise_irq() to also check the IRQ number requested
> by the host for other test commands.
> 
> Overall, this significantly simplifies the pci_epf_test_cmd_handler()
> function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 43 ++++++++-----------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 6f4ef5251452..43d623682850 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -615,6 +615,7 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	int count;
>  
>  	reg->status |= STATUS_IRQ_RAISED;
>  
> @@ -624,10 +625,22 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  				  PCI_EPC_IRQ_LEGACY, 0);
>  		break;
>  	case IRQ_TYPE_MSI:
> +		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
> +		if (reg->irq_number > count || count <= 0) {
> +			dev_err(dev, "Invalid MSI IRQ number %d / %d\n",
> +				reg->irq_number, count);
> +			return;
> +		}
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
>  				  PCI_EPC_IRQ_MSI, reg->irq_number);
>  		break;
>  	case IRQ_TYPE_MSIX:
> +		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
> +		if (reg->irq_number > count || count <= 0) {
> +			dev_err(dev, "Invalid MSIX IRQ number %d / %d\n",
> +				reg->irq_number, count);
> +			return;
> +		}
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
>  				  PCI_EPC_IRQ_MSIX, reg->irq_number);
>  		break;
> @@ -640,13 +653,11 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  static void pci_epf_test_cmd_handler(struct work_struct *work)
>  {
>  	int ret;
> -	int count;
>  	u32 command;
>  	struct pci_epf_test *epf_test = container_of(work, struct pci_epf_test,
>  						     cmd_handler.work);
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
> -	struct pci_epc *epc = epf->epc;
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
> @@ -662,10 +673,10 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  		goto reset_handler;
>  	}
>  
> -	if (command & COMMAND_RAISE_LEGACY_IRQ) {
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_LEGACY, 0);
> +	if ((command & COMMAND_RAISE_LEGACY_IRQ) ||
> +	    (command & COMMAND_RAISE_MSI_IRQ) ||
> +	    (command & COMMAND_RAISE_MSIX_IRQ)) {
> +		pci_epf_test_raise_irq(epf_test, reg);
>  		goto reset_handler;
>  	}
>  
> @@ -699,26 +710,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  		goto reset_handler;
>  	}
>  
> -	if (command & COMMAND_RAISE_MSI_IRQ) {
> -		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
> -		if (reg->irq_number > count || count <= 0)
> -			goto reset_handler;
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSI, reg->irq_number);
> -		goto reset_handler;
> -	}
> -
> -	if (command & COMMAND_RAISE_MSIX_IRQ) {
> -		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
> -		if (reg->irq_number > count || count <= 0)
> -			goto reset_handler;
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSIX, reg->irq_number);
> -		goto reset_handler;
> -	}
> -
>  reset_handler:
>  	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>  			   msecs_to_jiffies(1));
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்