| Message ID | 1223745433-26440-1-git-send-email-ron.mercer@qlogic.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
Ron Mercer <ron.mercer@qlogic.com> : [...] > static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32 intr) > { > u32 var = 0; > > - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) > + if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) > goto exit; > - else if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { > - ql_write32(qdev, INTR_EN, > - qdev->intr_context[intr].intr_dis_mask); > - var = ql_read32(qdev, STS); > - } > - atomic_inc(&qdev->intr_context[intr].irq_cnt); > + else { > + unsigned long hw_flags = 0; > + spin_lock_irqsave(&qdev->hw_lock, hw_flags); > + if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { > + ql_write32(qdev, INTR_EN, > + qdev->intr_context[intr].intr_dis_mask); > + var = ql_read32(qdev, STS); > + } > + atomic_inc(&qdev->intr_context[intr].irq_cnt); > + spin_unlock_irqrestore(&qdev->hw_lock, hw_flags); > + } > exit: > return var; > } Ron, the style is a bit convoluted and the indent mixes whitespaces and tabs. Could you make it look something like : { u32 var = 0; if (unlikely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) { unsigned long flags; spin_lock_irqsave(&qdev->hw_lock, flags); if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { ql_write32(qdev, INTR_EN, qdev->intr_context[intr].intr_dis_mask); var = ql_read32(qdev, STS); } atomic_inc(&qdev->intr_context[intr].irq_cnt); spin_unlock_irqrestore(&qdev->hw_lock, flags); } return var; } or (assuming there is no locking issue with the local variable ctx) : { u32 var = 0; if (unlikely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) { struct intr_context *ctx = qdev->intr_context + intr; unsigned long flags; spin_lock_irqsave(&qdev->hw_lock, flags); if (!atomic_read(&ctx->irq_cnt)) { ql_write32(qdev, INTR_EN, ctx->intr_dis_mask); var = ql_read32(qdev, STS); } atomic_inc(&ctx->irq_cnt); spin_unlock_irqrestore(&qdev->hw_lock, flags); } return var; } [...] > @@ -1752,12 +1743,15 @@ static irqreturn_t qlge_isr(int irq, void *dev_id) > int i; > int work_done = 0; > > - if (qdev->legacy_check && qdev->legacy_check(qdev)) { > - QPRINTK(qdev, INTR, INFO, "Already busy, not our interrupt.\n"); > - return IRQ_NONE; /* Not our interrupt */ > + spin_lock(&qdev->hw_lock); > + if(atomic_read(&qdev->intr_context[0].irq_cnt)) { ^^ if ( [...] > @@ -2791,8 +2786,13 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev) > /* > * Single interrupt means one handler for all rings. > */ > - intr_context->handler = qlge_isr; > - sprintf(intr_context->name, "%s-single_irq", qdev->ndev->name); > + if (likely(test_bit(QL_MSI_ENABLED, &qdev->flags))) { > + intr_context->handler = qlge_isr; > + sprintf(intr_context->name, "%s-msi-single_irq", qdev->ndev->name); > + } else { > + intr_context->handler = qlge_isr; > + sprintf(intr_context->name, "%s-legacy-single_irq", qdev->ndev->name); > + } It is the same "intr_context->handler = qlge_isr;" in both branches, is not it ?
On Sat, Oct 11, 2008 at 10:59:03PM +0200, Francois Romieu wrote: > Ron Mercer <ron.mercer@qlogic.com> : > [...] > > static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32 intr) > > { > > u32 var = 0; > > > > - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) > > + if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) > > goto exit; > > - else if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { > > - ql_write32(qdev, INTR_EN, > > - qdev->intr_context[intr].intr_dis_mask); > > - var = ql_read32(qdev, STS); > > - } > > - atomic_inc(&qdev->intr_context[intr].irq_cnt); > > + else { > > + unsigned long hw_flags = 0; > > + spin_lock_irqsave(&qdev->hw_lock, hw_flags); > > + if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { > > + ql_write32(qdev, INTR_EN, > > + qdev->intr_context[intr].intr_dis_mask); > > + var = ql_read32(qdev, STS); > > + } > > + atomic_inc(&qdev->intr_context[intr].irq_cnt); > > + spin_unlock_irqrestore(&qdev->hw_lock, hw_flags); > > + } > > exit: > > return var; > > } > > Ron, the style is a bit convoluted and the indent mixes whitespaces and > tabs. Could you make it look something like : Sorry about the tabs/whitespaces. I'll be more vigilant. > > { > u32 var = 0; > > if (unlikely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) { > unsigned long flags; > > spin_lock_irqsave(&qdev->hw_lock, flags); > if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { > ql_write32(qdev, INTR_EN, > qdev->intr_context[intr].intr_dis_mask); > var = ql_read32(qdev, STS); > } > atomic_inc(&qdev->intr_context[intr].irq_cnt); > spin_unlock_irqrestore(&qdev->hw_lock, flags); > } > return var; > } > > or (assuming there is no locking issue with the local variable ctx) : > > { > u32 var = 0; > > if (unlikely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) { > struct intr_context *ctx = qdev->intr_context + intr; > unsigned long flags; > > spin_lock_irqsave(&qdev->hw_lock, flags); > if (!atomic_read(&ctx->irq_cnt)) { > ql_write32(qdev, INTR_EN, ctx->intr_dis_mask); > var = ql_read32(qdev, STS); > } > atomic_inc(&ctx->irq_cnt); > spin_unlock_irqrestore(&qdev->hw_lock, flags); > } > return var; > } > Your example looks much better than what I had, but it reverses the logic and won't work. What I'm trying to do is skip the operation if we're running with MSIX-multiple interrupts AND it's not context zero. For all other cases we do the operation. So fixing your suggestion it would be: { u32 var = 0; if (unlikely(!(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))) { struct intr_context *ctx = qdev->intr_context + intr; unsigned long flags; spin_lock_irqsave(&qdev->hw_lock, flags); if (!atomic_read(&ctx->irq_cnt)) { ql_write32(qdev, INTR_EN, ctx->intr_dis_mask); var = ql_read32(qdev, STS); } atomic_inc(&ctx->irq_cnt); spin_unlock_irqrestore(&qdev->hw_lock, flags); } return var; } This is more compact, it's still optimized, but I think it becomes a little hard to read. I'll gladly try it and make the change if you want. > [...] > > @@ -1752,12 +1743,15 @@ static irqreturn_t qlge_isr(int irq, void *dev_id) > > int i; > > int work_done = 0; > > > > - if (qdev->legacy_check && qdev->legacy_check(qdev)) { > > - QPRINTK(qdev, INTR, INFO, "Already busy, not our interrupt.\n"); > > - return IRQ_NONE; /* Not our interrupt */ > > + spin_lock(&qdev->hw_lock); > > + if(atomic_read(&qdev->intr_context[0].irq_cnt)) { > ^^ if ( > > [...] > > @@ -2791,8 +2786,13 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev) > > /* > > * Single interrupt means one handler for all rings. > > */ > > - intr_context->handler = qlge_isr; > > - sprintf(intr_context->name, "%s-single_irq", qdev->ndev->name); > > + if (likely(test_bit(QL_MSI_ENABLED, &qdev->flags))) { > > + intr_context->handler = qlge_isr; > > + sprintf(intr_context->name, "%s-msi-single_irq", qdev->ndev->name); > > + } else { > > + intr_context->handler = qlge_isr; > > + sprintf(intr_context->name, "%s-legacy-single_irq", qdev->ndev->name); > > + } > > It is the same "intr_context->handler = qlge_isr;" in both branches, is not it ? Yes, thanks. I'll fix this. Ron -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Your example looks much better than what I had, but it reverses the > logic and won't work. What I'm trying to do is skip the operation if > we're running with MSIX-multiple interrupts AND it's not context zero. > For all other cases we do the operation. So fixing your suggestion it would > be: > > { > u32 var = 0; > > if (unlikely(!(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr))) { > struct intr_context *ctx = qdev->intr_context + intr; > unsigned long flags; > > spin_lock_irqsave(&qdev->hw_lock, flags); > if (!atomic_read(&ctx->irq_cnt)) { > ql_write32(qdev, INTR_EN, ctx->intr_dis_mask); > var = ql_read32(qdev, STS); > } > atomic_inc(&ctx->irq_cnt); > spin_unlock_irqrestore(&qdev->hw_lock, flags); > } > return var; > } I think the simplest thing is to avoid the goto and the else clause; ie instead of putting the bulk of the function inside an block, just have the exception leave early, and have the other code in the main flow of the function... so you could do the following, and even include a comment if you want: { u32 var; struct intr_context *ctx; unsigned long flags; /* Nothing to do if we're using MSI-X and this is not context zero */ if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) return 0; ctx = qdev->intr_context + intr; spin_lock_irqsave(&qdev->hw_lock, flags); if (!atomic_read(&ctx->irq_cnt)) { ql_write32(qdev, INTR_EN, ctx->intr_dis_mask); var = ql_read32(qdev, STS); } atomic_inc(&ctx->irq_cnt); spin_unlock_irqrestore(&qdev->hw_lock, flags); return var; } That should generate good code and IMO is the most readable variant. (BTW the "if (likely(something) && somethingelse)" seems a little fishy -- I don't know what gcc really does with that. Either the whole condition should be marked likely or it's probably not worth annotating it at all) - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ron Mercer <ron.mercer@qlogic.com> : [...] > Your example looks much better than what I had, but it reverses the > logic and won't work. What I'm trying to do is skip the operation if > we're running with MSIX-multiple interrupts AND it's not context zero. Ok, ok. > For all other cases we do the operation. So fixing your suggestion it would > be: [snip] > This is more compact, it's still optimized, but I think it becomes a > little hard to read. Remove 'unlikely' ? > I'll gladly try it and make the change if you Your choice. I'm fine with Roland's code as well.
On Tue, Oct 14, 2008 at 10:40:25PM +0200, Francois Romieu wrote: > Ron Mercer <ron.mercer@qlogic.com> : > [...] > > Your example looks much better than what I had, but it reverses the > > logic and won't work. What I'm trying to do is skip the operation if > > we're running with MSIX-multiple interrupts AND it's not context zero. > > Ok, ok. > > > For all other cases we do the operation. So fixing your suggestion it would > > be: > [snip] > > This is more compact, it's still optimized, but I think it becomes a > > little hard to read. > > Remove 'unlikely' ? > > > I'll gladly try it and make the change if you > > Your choice. I'm fine with Roland's code as well. > > -- > Ueimor I'll use Roland's as it does the most with the fewest lines and is easy to understand. Thanks to both of you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h index c37ea43..cc246f8 100644 --- a/drivers/net/qlge/qlge.h +++ b/drivers/net/qlge/qlge.h @@ -1375,7 +1375,6 @@ struct ql_adapter { spinlock_t adapter_lock; spinlock_t hw_lock; spinlock_t stats_lock; - spinlock_t legacy_lock; /* used for maintaining legacy intr sync */ /* PCI Bus Relative Register Addresses */ void __iomem *reg_base; @@ -1502,7 +1501,7 @@ void ql_mpi_work(struct work_struct *work); void ql_mpi_reset_work(struct work_struct *work); int ql_wait_reg_rdy(struct ql_adapter *qdev, u32 reg, u32 bit, u32 ebit); void ql_queue_asic_error(struct ql_adapter *qdev); -void ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr); +u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr); void ql_set_ethtool_ops(struct net_device *ndev); int ql_read_xgmac_reg64(struct ql_adapter *qdev, u32 reg, u64 *data); diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c index 3af822b..77a66e5 100644 --- a/drivers/net/qlge/qlge_main.c +++ b/drivers/net/qlge/qlge_main.c @@ -577,40 +577,43 @@ static void ql_disable_interrupts(struct ql_adapter *qdev) * incremented everytime we queue a worker and decremented everytime * a worker finishes. Once it hits zero we enable the interrupt. */ -void ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr) +u32 ql_enable_completion_interrupt(struct ql_adapter *qdev, u32 intr) { - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) + u32 var = 0; + if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) { ql_write32(qdev, INTR_EN, qdev->intr_context[intr].intr_en_mask); - else { - if (qdev->legacy_check) - spin_lock(&qdev->legacy_lock); + var = ql_read32(qdev, STS); + } else { + unsigned long hw_flags=0; + spin_lock_irqsave(&qdev->hw_lock, hw_flags); if (atomic_dec_and_test(&qdev->intr_context[intr].irq_cnt)) { - QPRINTK(qdev, INTR, ERR, "Enabling interrupt %d.\n", - intr); ql_write32(qdev, INTR_EN, qdev->intr_context[intr].intr_en_mask); - } else { - QPRINTK(qdev, INTR, ERR, - "Skip enable, other queue(s) are active.\n"); + var = ql_read32(qdev, STS); } - if (qdev->legacy_check) - spin_unlock(&qdev->legacy_lock); + spin_unlock_irqrestore(&qdev->hw_lock, hw_flags); } + return var; } static u32 ql_disable_completion_interrupt(struct ql_adapter *qdev, u32 intr) { u32 var = 0; - if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) + if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags) && intr)) goto exit; - else if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { - ql_write32(qdev, INTR_EN, - qdev->intr_context[intr].intr_dis_mask); - var = ql_read32(qdev, STS); - } - atomic_inc(&qdev->intr_context[intr].irq_cnt); + else { + unsigned long hw_flags = 0; + spin_lock_irqsave(&qdev->hw_lock, hw_flags); + if (!atomic_read(&qdev->intr_context[intr].irq_cnt)) { + ql_write32(qdev, INTR_EN, + qdev->intr_context[intr].intr_dis_mask); + var = ql_read32(qdev, STS); + } + atomic_inc(&qdev->intr_context[intr].irq_cnt); + spin_unlock_irqrestore(&qdev->hw_lock, hw_flags); + } exit: return var; } @@ -623,7 +626,8 @@ static void ql_enable_all_completion_interrupts(struct ql_adapter *qdev) * and enables only if the result is zero. * So we precharge it here. */ - atomic_set(&qdev->intr_context[i].irq_cnt, 1); + if (unlikely(!test_bit(QL_MSIX_ENABLED, &qdev->flags) || i==0)) + atomic_set(&qdev->intr_context[i].irq_cnt, 1); ql_enable_completion_interrupt(qdev, i); } @@ -1725,19 +1729,6 @@ static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id) return IRQ_HANDLED; } -/* We check here to see if we're already handling a legacy - * interrupt. If we are, then it must belong to another - * chip with which we're sharing the interrupt line. - */ -int ql_legacy_check(struct ql_adapter *qdev) -{ - int err; - spin_lock(&qdev->legacy_lock); - err = atomic_read(&qdev->intr_context[0].irq_cnt); - spin_unlock(&qdev->legacy_lock); - return err; -} - /* This handles a fatal error, MPI activity, and the default * rx_ring in an MSI-X multiple vector environment. * In MSI/Legacy environment it also process the rest of @@ -1752,12 +1743,15 @@ static irqreturn_t qlge_isr(int irq, void *dev_id) int i; int work_done = 0; - if (qdev->legacy_check && qdev->legacy_check(qdev)) { - QPRINTK(qdev, INTR, INFO, "Already busy, not our interrupt.\n"); - return IRQ_NONE; /* Not our interrupt */ + spin_lock(&qdev->hw_lock); + if(atomic_read(&qdev->intr_context[0].irq_cnt)) { + QPRINTK(qdev, INTR, DEBUG, "Shared Interrupt, Not ours!\n"); + spin_unlock(&qdev->hw_lock); + return IRQ_NONE; } + spin_unlock(&qdev->hw_lock); - var = ql_read32(qdev, STS); + var = ql_disable_completion_interrupt(qdev, intr_context->intr); /* * Check for fatal error. @@ -1791,22 +1785,23 @@ static irqreturn_t qlge_isr(int irq, void *dev_id) */ rx_ring = &qdev->rx_ring[0]; if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) != rx_ring->cnsmr_idx) { - QPRINTK(qdev, INTR, INFO, "Waking handler for rx_ring[0].\n"); + QPRINTK(qdev, INTR, DEBUG, "Waking handler for rx_ring[0].\n"); ql_disable_completion_interrupt(qdev, intr_context->intr); queue_delayed_work_on(smp_processor_id(), qdev->q_workqueue, &rx_ring->rx_work, 0); work_done++; } + + /* + * Start the DPC for each active queue. + */ if (!test_bit(QL_MSIX_ENABLED, &qdev->flags)) { - /* - * Start the DPC for each active queue. - */ for (i = 1; i < qdev->rx_ring_count; i++) { rx_ring = &qdev->rx_ring[i]; if (ql_read_sh_reg(rx_ring->prod_idx_sh_reg) != rx_ring->cnsmr_idx) { - QPRINTK(qdev, INTR, INFO, + QPRINTK(qdev, INTR, DEBUG, "Waking handler for rx_ring[%d].\n", i); ql_disable_completion_interrupt(qdev, intr_context-> @@ -1823,6 +1818,8 @@ static irqreturn_t qlge_isr(int irq, void *dev_id) } } } + ql_enable_completion_interrupt(qdev, intr_context->intr); + return work_done ? IRQ_HANDLED : IRQ_NONE; } @@ -2701,8 +2698,6 @@ msi: } } irq_type = LEG_IRQ; - spin_lock_init(&qdev->legacy_lock); - qdev->legacy_check = ql_legacy_check; QPRINTK(qdev, IFUP, DEBUG, "Running with legacy interrupts.\n"); } @@ -2791,8 +2786,13 @@ static void ql_resolve_queues_to_irqs(struct ql_adapter *qdev) /* * Single interrupt means one handler for all rings. */ - intr_context->handler = qlge_isr; - sprintf(intr_context->name, "%s-single_irq", qdev->ndev->name); + if (likely(test_bit(QL_MSI_ENABLED, &qdev->flags))) { + intr_context->handler = qlge_isr; + sprintf(intr_context->name, "%s-msi-single_irq", qdev->ndev->name); + } else { + intr_context->handler = qlge_isr; + sprintf(intr_context->name, "%s-legacy-single_irq", qdev->ndev->name); + } for (i = 0; i < qdev->rx_ring_count; i++) qdev->rx_ring[i].irq = 0; } @@ -2865,7 +2865,8 @@ static int ql_request_irq(struct ql_adapter *qdev) "%s: dev_id = 0x%p.\n", __func__, &qdev->rx_ring[0]); status = - request_irq(pdev->irq, qlge_isr, + request_irq(pdev->irq, + intr_context->handler, test_bit(QL_MSI_ENABLED, &qdev-> flags) ? 0 : IRQF_SHARED, @@ -3310,7 +3311,6 @@ static int ql_configure_rings(struct ql_adapter *qdev) * completion handler rx_rings. */ qdev->rx_ring_count = qdev->tx_ring_count + qdev->rss_ring_count + 1; - if (ql_alloc_ring_cb(qdev)) return -ENOMEM; @@ -3411,6 +3411,7 @@ static int qlge_open(struct net_device *ndev) error_up: ql_release_adapter_resources(qdev); ql_free_ring_cb(qdev); + QL_DUMP_ALL(qdev); return err; } diff --git a/drivers/net/qlge/qlge_mpi.c b/drivers/net/qlge/qlge_mpi.c index 24fe344..6bd3fad 100644 --- a/drivers/net/qlge/qlge_mpi.c +++ b/drivers/net/qlge/qlge_mpi.c @@ -132,7 +132,6 @@ void ql_mpi_work(struct work_struct *work) default: /* Clear the MPI firmware status. */ ql_write32(qdev, CSR, CSR_CMD_CLR_R2PCI_INT); - break; } } ql_enable_completion_interrupt(qdev, 0);
Signed-off-by: Ron Mercer <ron.mercer@qlogic.com> --- drivers/net/qlge/qlge.h | 3 +- drivers/net/qlge/qlge_main.c | 97 +++++++++++++++++++++--------------------- drivers/net/qlge/qlge_mpi.c | 1 - 3 files changed, 50 insertions(+), 51 deletions(-)