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: <adabpxmx6f3.fsf@cisco.com>
Date:	Tue, 14 Oct 2008 13:34:08 -0700
From:	Roland Dreier <rdreier@...co.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.

 > 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html