Re: [PATCH v2] drm/amd: Fix the flag setting code for interrupt request

["Christian König" <christian.koenig@amd.com>]

Am 06.09.23 um 08:55 schrieb Ma Jun:
> [1] Remove the irq flags setting code since pci_alloc_irq_vectors()
> handles these flags.
> [2] Free the msi vectors in case of error.
>
> v2:
> - Remove local variable initializing code (Christian)
> - Use PCI_IRQ_ALL_TYPES (Alex)
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 45 ++++++++++++++-----------
>   1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index fa6d0adcec20..64c245015e17 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -270,29 +270,29 @@ static void amdgpu_restore_msix(struct amdgpu_device *adev)
>    */
>   int amdgpu_irq_init(struct amdgpu_device *adev)
>   {
> -	int r = 0;
> -	unsigned int irq;
> +	int r;
> +	unsigned int irq, flags;

It's also good style to define variables like "r" and "i" last. Some 
upstream maintainers even require reverse xmas tree style defines (e.g. 
longest first, shortest last).

With that changed the patch is Acked-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

>   
>   	spin_lock_init(&adev->irq.lock);
>   
>   	/* Enable MSI if not disabled by module parameter */
>   	adev->irq.msi_enabled = false;
>   
> +	if (!amdgpu_msi_ok(adev))
> +		flags = PCI_IRQ_LEGACY;
> +	else
> +		flags = PCI_IRQ_ALL_TYPES;
> +
> +	/* we only need one vector */
> +	r = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
> +	if (r < 0) {
> +		dev_err(adev->dev, "Failed to alloc msi vectors\n");
> +		return r;
> +	}
> +
>   	if (amdgpu_msi_ok(adev)) {
> -		int nvec = pci_msix_vec_count(adev->pdev);
> -		unsigned int flags;
> -
> -		if (nvec <= 0)
> -			flags = PCI_IRQ_MSI;
> -		else
> -			flags = PCI_IRQ_MSI | PCI_IRQ_MSIX;
> -
> -		/* we only need one vector */
> -		nvec = pci_alloc_irq_vectors(adev->pdev, 1, 1, flags);
> -		if (nvec > 0) {
> -			adev->irq.msi_enabled = true;
> -			dev_dbg(adev->dev, "using MSI/MSI-X.\n");
> -		}
> +		adev->irq.msi_enabled = true;
> +		dev_dbg(adev->dev, "using MSI/MSI-X.\n");
>   	}
>   
>   	INIT_WORK(&adev->irq.ih1_work, amdgpu_irq_handle_ih1);
> @@ -302,22 +302,29 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>   	/* Use vector 0 for MSI-X. */
>   	r = pci_irq_vector(adev->pdev, 0);
>   	if (r < 0)
> -		return r;
> +		goto free_vectors;
>   	irq = r;
>   
>   	/* PCI devices require shared interrupts. */
>   	r = request_irq(irq, amdgpu_irq_handler, IRQF_SHARED, adev_to_drm(adev)->driver->name,
>   			adev_to_drm(adev));
>   	if (r)
> -		return r;
> +		goto free_vectors;
> +
>   	adev->irq.installed = true;
>   	adev->irq.irq = irq;
>   	adev_to_drm(adev)->max_vblank_count = 0x00ffffff;
>   
>   	DRM_DEBUG("amdgpu: irq initialized.\n");
>   	return 0;
> -}
>   
> +free_vectors:
> +	if (adev->irq.msi_enabled)
> +		pci_free_irq_vectors(adev->pdev);
> +
> +	adev->irq.msi_enabled = false;
> +	return r;
> +}
>   
>   void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>   {