Re: [PATCH v1.0 2/16] arcmsr: Support MSI-X interrupt

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Apr 30, 2014 at 05:50:12PM +0800, ching wrote:
> From: Ching<ching2048@areca.com.tw>
> 
> Adding code for supporting MSI-X interrupt
> 
> Signed-off-by: Ching<ching2048@areca.com.tw>
> ---

These patches seem to be broken up nicely into patches which do one
thing per patch and they are much easier to review now.  Thanks!

> +static int arcmsr_request_irq(struct pci_dev *pdev, struct AdapterControlBlock *acb)
> +{
> +	int	i, j, r;
> +	struct msix_entry entries[ARCMST_NUM_MSIX_VECTORS];
> +

This function is more confusing than necessary and it goes over the 80
character limit.  One hint for improving it is that you should always
test for failure and handle the failure condition first before moving
on.  In other words reverse these tests.

Also let's rename LEG_INT to legacy_int: and MSI_INT to msi_int:.

> +	if (pci_find_capability(pdev, PCI_CAP_ID_MSIX)) {

	if (!pci_find_capability(pdev, PCI_CAP_ID_MSIX))
		goto msi_int;

We can pull the following code in one indent level.

> +		for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
> +			entries[i].entry = i;
> +		}

Remove unneeded curly braces.

> +		r = pci_enable_msix(pdev, entries, ARCMST_NUM_MSIX_VECTORS);

		if (r < 0)
			goto msi_int;

> +		if (r == 0) {
> +			for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
> +				if (request_irq(entries[i].vector,
> +					arcmsr_do_interrupt, 0, "arcmsr", acb)) {
> +					pr_warn("arcmsr%d: request_irq =%d failed!\n",
> +						acb->host->host_no, pdev->irq);
> +					for (j = 0 ; j < i ; j++)
> +						free_irq(entries[i].vector, acb);
> +					pci_disable_msix(pdev);
> +					goto MSI_INT;
> +				}
> +				acb->entries[i] = entries[i];
> +			}
> +			acb->acb_flags |= ACB_F_MSIX_ENABLED;
> +			pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);

This should be pr_info().  pr_warn() will probably trigger a popup in
gnome.
			return ARC_SUCCESS;

Returning right away is easier to understand than a return at the end
of the function.

> +		} else if (r > 0) {

This else statement is not needed because we already handled the other
cases.  Pull the code in one additional indent level.

> +			if (!pci_enable_msix(pdev, entries, r)) {

			if (pci_enable_msix(pdev, entries, r))
				goto msi_int;

Pull the code in a third indent level.

> +				for (i = 0; i < r; i++) {
> +					if (request_irq(entries[i].vector,
> +						arcmsr_do_interrupt, 0, "arcmsr", acb)) {
> +						pr_warn("arcmsr%d: request_irq =%d failed!\n",
> +							acb->host->host_no, pdev->irq);
> +						for (j = 0 ; j < i ; j++)
> +							free_irq(entries[i].vector, acb);
> +						pci_disable_msix(pdev);
> +						goto MSI_INT;
> +					}
> +					acb->entries[i] = entries[i];
> +				}
> +				acb->acb_flags |= ACB_F_MSIX_ENABLED;
> +				pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);


Same thing.  Change this to pr_info() and add a return ARC_SUCCESS;

> +			} else {
> +				goto MSI_INT;
> +			}
> +		} else {


Ok.  At this point we have removed a lot of indenting so we are back to
level 1 indenting.

msi_int:
	if (!pci_find_capability(pdev, PCI_CAP_ID_MSI))
		goto legacy_int;
	if (pci_enable_msi(pdev))
		goto legacy_int;
	if (request_irq(...)) {
		....
		goto legacy_int;
	}

> +MSI_INT:
> +			if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
> +				if (pci_enable_msi(pdev))
> +					goto LEG_INT;
> +				if (request_irq(pdev->irq, arcmsr_do_interrupt,
> +					IRQF_SHARED, "arcmsr", acb)) {
> +					pr_warn("arcmsr%d: request_irq =%d failed!\n",
> +						acb->host->host_no, pdev->irq);
> +					pci_disable_msi(pdev);
> +					goto LEG_INT;
> +				}
> +				acb->acb_flags |= ACB_F_MSI_ENABLED;
> +				pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);

return sucess.

> +			} else {
> +				goto LEG_INT;
> +			}
> +		}


The following block is all duplicative code and we have already handled
msi_int.  Just delete it.

> +	} else if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
> +		if (pci_enable_msi(pdev))
> +			goto LEG_INT;
> +		if (request_irq(pdev->irq, arcmsr_do_interrupt,
> +			IRQF_SHARED, "arcmsr", acb)) {
> +			pr_warn("arcmsr%d: request_irq =%d failed!\n",
> +				acb->host->host_no, pdev->irq);
> +			pci_disable_msi(pdev);
> +			goto LEG_INT;
> +		}
> +		acb->acb_flags |= ACB_F_MSI_ENABLED;
> +		pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);
> +	} else {

Ok.  We're back to level one indenting now.

legacy_int:
	if (request_irq(...) {
		...
		return ARC_FAILURE;
	}
	return ARC_SUCCESS;

Flipping the conditions around and adding immediate returns makes the
code a lot easier to read.

> +LEG_INT:
> +		if (request_irq(pdev->irq, arcmsr_do_interrupt,
> +			IRQF_SHARED, "arcmsr", acb)) {
> +			pr_warn("arcmsr%d: request_irq = %d failed!\n",
> +				acb->host->host_no, pdev->irq);
> +			return ARC_FAILURE;
> +		}
> +	}
> +	return ARC_SUCCESS;
> +}
> +

regards,
dan carpenter