Skip to contentSkip to conversation

arch: x86: kernel: fixed unused label issue

PATCH In review Piotr Gregor 2 participants 2 messages Dec 14, 2016

Subsystems (3)
x86 virtio core PARAVIRT_OPS interface
lore
Share & tools
lore

Piotr Gregor

author
#0 lore raw

PATCH 1/1 arch: x86: kernel: fixed unused label issue

The patch_default label is only used from within
case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
and
case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true. Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS is not defined and label should be removed in that case. Moving #endif directive just after that label fixes the issue.

In addition,there are three errors reported by checkpatch script on this file. This commit fixes two of them. The last one is
ERROR: Macros with multiple statements should be enclosed in a do - while loop
which probably is a false alarm here as PATCH_SITE macro defines a case in switch local to native_patch function not meant to be used in other places.

Signed-off-by Piotr Gregor <[email protected]>
---
 arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 33 deletions(-)
arch/x86/kernel/paravirt_patch_64.c +34 -33
index f4fcf26..7ce2848 100644
@@ -44,43 +44,44 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
 	const unsigned char *start, *end;
 	unsigned ret;
 
-#define PATCH_SITE(ops, x)					\
-		case PARAVIRT_PATCH(ops.x):			\
-			start = start_##ops##_##x;		\
-			end = end_##ops##_##x;			\
-			goto patch_site
-	switch(type) {
-		PATCH_SITE(pv_irq_ops, restore_fl);
-		PATCH_SITE(pv_irq_ops, save_fl);
-		PATCH_SITE(pv_irq_ops, irq_enable);
-		PATCH_SITE(pv_irq_ops, irq_disable);
-		PATCH_SITE(pv_cpu_ops, usergs_sysret64);
-		PATCH_SITE(pv_cpu_ops, swapgs);
-		PATCH_SITE(pv_mmu_ops, read_cr2);
-		PATCH_SITE(pv_mmu_ops, read_cr3);
-		PATCH_SITE(pv_mmu_ops, write_cr3);
-		PATCH_SITE(pv_mmu_ops, flush_tlb_single);
-		PATCH_SITE(pv_cpu_ops, wbinvd);
+#define PATCH_SITE(ops, x)				\
+	case PARAVIRT_PATCH(ops.x):			\
+		start = start_##ops##_##x;		\
+		end = end_##ops##_##x;			\
+		goto patch_site
+
+	switch (type) {
+	PATCH_SITE(pv_irq_ops, restore_fl);
+	PATCH_SITE(pv_irq_ops, save_fl);
+	PATCH_SITE(pv_irq_ops, irq_enable);
+	PATCH_SITE(pv_irq_ops, irq_disable);
+	PATCH_SITE(pv_cpu_ops, usergs_sysret64);
+	PATCH_SITE(pv_cpu_ops, swapgs);
+	PATCH_SITE(pv_mmu_ops, read_cr2);
+	PATCH_SITE(pv_mmu_ops, read_cr3);
+	PATCH_SITE(pv_mmu_ops, write_cr3);
+	PATCH_SITE(pv_mmu_ops, flush_tlb_single);
+	PATCH_SITE(pv_cpu_ops, wbinvd);
 #if defined(CONFIG_PARAVIRT_SPINLOCKS)
-		case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
-			if (pv_is_native_spin_unlock()) {
-				start = start_pv_lock_ops_queued_spin_unlock;
-				end   = end_pv_lock_ops_queued_spin_unlock;
-				goto patch_site;
-			}
-			goto patch_default;
+	case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
+		if (pv_is_native_spin_unlock()) {
+			start = start_pv_lock_ops_queued_spin_unlock;
+			end   = end_pv_lock_ops_queued_spin_unlock;
+			goto patch_site;
+		}
+		goto patch_default;
 
-		case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
-			if (pv_is_native_vcpu_is_preempted()) {
-				start = start_pv_lock_ops_vcpu_is_preempted;
-				end   = end_pv_lock_ops_vcpu_is_preempted;
-				goto patch_site;
-			}
-			goto patch_default;
-#endif
+	case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
+		if (pv_is_native_vcpu_is_preempted()) {
+			start = start_pv_lock_ops_vcpu_is_preempted;
+			end   = end_pv_lock_ops_vcpu_is_preempted;
+			goto patch_site;
+		}
+		goto patch_default;
 
-	default:
 patch_default:
+#endif
+	default:
 		ret = paravirt_patch_default(type, clobbers, ibuf, addr, len);
 		break;
Signature · 1 line
2.1.4
#1 lore raw
Quoted 17 lines · Piotr Gregor

The patch_default label is only used from within
case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
and
case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true. Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS is not defined and label should be removed in that case. Moving #endif directive just after that label fixes the issue.

In addition,there are three errors reported by checkpatch script on this file. This commit fixes two of them. The last one is
ERROR: Macros with multiple statements should be enclosed in a do - while loop
which probably is a false alarm here as PATCH_SITE macro defines a case in switch local to native_patch function not meant to be used in other places.

Quoted 4 lines
Signed-off-by Piotr Gregor <[email protected]>
---
 arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 33 deletions(-)

*urgh*

So you forgot to look up the patch that caused it, which made you fail to Cc the author of the patch that caused it (me).

That also made you miss the fact that you're only fixing half the problem, namely you failed to touch arch/x86/kernel/paravirt_patch_32.c which has identical code in.

Thirdly you used checkpatch and moved muck about creating a trainwreck of a patch.

Anyway, you can use __maybe_unused on a label, I'm not sure which I prefer, this way you jump into the previous case block and use fall-through, while with the variant I did you jump into the right case.

Its a shame you cannot jump to the default label itself.