netdev - Re: [PATCH 1/6] [NET-NEXT]qlge: Clean up and fix MSI and legacy irq handling.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081014201915.GA27393@susedev.qlogic.org>
Date:	Tue, 14 Oct 2008 13:19:15 -0700
From:	Ron Mercer <ron.mercer@...gic.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	jeff@...zik.org, netdev@...r.kernel.org, linux-driver@...gic.com
Subject: Re: [PATCH 1/6] [NET-NEXT]qlge: Clean up and fix MSI and legacy irq handling.

On Sat, Oct 11, 2008 at 10:59:03PM +0200, Francois Romieu wrote:
> Ron Mercer <ron.mercer@...gic.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html