Thanks for Dan's advice.
I will revise the patch code.
Next upload will be PATCH v1.1 16 patches.
On Fri, 2014-05-02 at 11:31 +0300, Dan Carpenter wrote:
> On Wed, Apr 30, 2014 at 05:50:12PM +0800, ching wrote:
> > From: Ching<ching2048@xxxxxxxxxxxx>
> >
> > Adding code for supporting MSI-X interrupt
> >
> > Signed-off-by: Ching<ching2048@xxxxxxxxxxxx>
> > ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
