| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1399031048.5711.3.camel@localhost>
Date: Fri, 02 May 2014 19:44:08 +0800
From: ching <ching2048@...ca.com.tw>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: jbottomley@...allels.com, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org, thenzl@...hat.com
Subject: Re: [PATCH v1.0 2/16] arcmsr: Support MSI-X interrupt
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@...ca.com.tw>
> >
> > Adding code for supporting MSI-X interrupt
> >
> > Signed-off-by: Ching<ching2048@...ca.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists