x86: randconfig warning fixes

Hi,

I've gone through old patches of mine that I carried in my randconfig tree, to see which warnings are still present. Here is a set of fixes for arch/x86. Most of them were sent before at some point and missed out for one reason or another.

Please have another look and apply what you like.

Arnd

Arnd Bergmann (8):
perf/x86: shut up false-positive -Wmaybe-uninitialized warning
x86: math-emu: possible uninitialized variable use
x86: math-emu: avoid bogus -Wint-in-bool-context warning
x86: io: add "memory" clobber to insb/insw/insl/outsb/outsw/outsl
x86: silence build with "make -s"
x86: add MULTIUSER dependency for KVM
x86: add PCI dependency for PUNIT_ATOM_DEBUG
x86: intel-mid: fix a format string overflow warning

 arch/x86/Kconfig.debug                                   |  1 +
 arch/x86/boot/Makefile                                   |  5 +++--
 arch/x86/events/core.c                                   |  4 ++--
 arch/x86/include/asm/io.h                                |  4 ++--
 arch/x86/kvm/Kconfig                                     |  2 +-
 arch/x86/math-emu/Makefile                               |  4 ++--
 arch/x86/math-emu/fpu_emu.h                              |  2 +-
 arch/x86/math-emu/reg_compare.c                          | 16 ++++++++--------
 .../platform/intel-mid/device_libs/platform_max7315.c    |  6 ++++--
 9 files changed, 24 insertions(+), 20 deletions(-)

2.9.0

The intialization function checks for various failure scenarios, but unfortunately the compiler gets a little confused about the possible combinations, leading to a false-positive build warning when -Wmaybe-uninitialized is set:

arch/x86/events/core.c: In function ‘init_hw_perf_events’:
arch/x86/events/core.c:264:3: warning: ‘reg_fail’ may be used uninitialized in this function [-Wmaybe-uninitialized] arch/x86/events/core.c:264:3: warning: ‘val_fail’ may be used uninitialized in this function [-Wmaybe-uninitialized]
pr_err(FW_BUG "the BIOS has corrupted hw-PMU resources (MSR %x is %Lx)\n",

We can't actually run into this case, so this shuts up the warning by initializing the variables to a known-invalid state.

---
v2: replaced original patch that reordered the code instead of adding a fake initialization.

---
 arch/x86/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ff1ea2fb9705..8e3db8f642a7 100644
@@ -191,8 +191,8 @@ static void release_pmc_hardware(void) {}
 
 static bool check_hw_exists(void)
 {
-	u64 val, val_fail, val_new= ~0;
-	int i, reg, reg_fail, ret = 0;
+	u64 val, val_fail = -1, val_new= ~0;
+	int i, reg, reg_fail = -1, ret = 0;
 	int bios_fail = 0;
 	int reg_safe = -1;
 

2.9.0

When building the kernel with "make EXTRA_CFLAGS=...", this overrides the "PARANOID" preprocessor macro defined in arch/x86/math-emu/Makefile, and we run into a build warning:

arch/x86/math-emu/reg_compare.c: In function ‘compare_i_st_st’:
arch/x86/math-emu/reg_compare.c:254:6: error: ‘f’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

This fixes the implementation to work correctly even without the PARANOID flag, and also fixes the Makefile to not use the EXTRA_CFLAGS variable but instead use the ccflags-y variable in the Makefile that is meant for this purpose.

---
Originally sent on Oct. 17 2016, resending unmodified

---
 arch/x86/math-emu/Makefile      |  4 ++--
 arch/x86/math-emu/reg_compare.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

index 9b0c63b60302..1b2dac174321 100644
@@ -5,8 +5,8 @@
 #DEBUG	= -DDEBUGGING
 DEBUG	=
 PARANOID = -DPARANOID
-EXTRA_CFLAGS	:= $(PARANOID) $(DEBUG) -fno-builtin $(MATH_EMULATION)
-EXTRA_AFLAGS	:= $(PARANOID)
+ccflags-y += $(PARANOID) $(DEBUG) -fno-builtin $(MATH_EMULATION)
+asflags-y += $(PARANOID)
 
 # From 'C' language sources:
 C_OBJS =fpu_entry.o errors.o \

index b77360fdbf4a..19b33b50adfa 100644
@@ -168,7 +168,7 @@ static int compare(FPU_REG const *b, int tagb)
 /* This function requires that st(0) is not empty */
 int FPU_compare_st_data(FPU_REG const *loaded_data, u_char loaded_tag)
 {
-	int f = 0, c;
+	int f, c;
 
 	c = compare(loaded_data, loaded_tag);
 

@@ -189,12 +189,12 @@ int FPU_compare_st_data(FPU_REG const *loaded_data, u_char loaded_tag)
 		case COMP_No_Comp:
 			f = SW_C3 | SW_C2 | SW_C0;
 			break;
-#ifdef PARANOID
 		default:
+#ifdef PARANOID
 			EXCEPTION(EX_INTERNAL | 0x121);
+#endif /* PARANOID */
 			f = SW_C3 | SW_C2 | SW_C0;
 			break;
-#endif /* PARANOID */
 		}
 	setcc(f);
 	if (c & COMP_Denormal) {

@@ -205,7 +205,7 @@ int FPU_compare_st_data(FPU_REG const *loaded_data, u_char loaded_tag)
 
 static int compare_st_st(int nr)
 {
-	int f = 0, c;
+	int f, c;
 	FPU_REG *st_ptr;
 
 	if (!NOT_EMPTY(0) || !NOT_EMPTY(nr)) {

@@ -235,12 +235,12 @@ static int compare_st_st(int nr)
 		case COMP_No_Comp:
 			f = SW_C3 | SW_C2 | SW_C0;
 			break;
-#ifdef PARANOID
 		default:
+#ifdef PARANOID
 			EXCEPTION(EX_INTERNAL | 0x122);
+#endif /* PARANOID */
 			f = SW_C3 | SW_C2 | SW_C0;
 			break;
-#endif /* PARANOID */
 		}
 	setcc(f);
 	if (c & COMP_Denormal) {

@@ -283,12 +283,12 @@ static int compare_i_st_st(int nr)
 	case COMP_No_Comp:
 		f = X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF;
 		break;
-#ifdef PARANOID
 	default:
+#ifdef PARANOID
 		EXCEPTION(EX_INTERNAL | 0x122);
+#endif /* PARANOID */
 		f = 0;
 		break;
-#endif /* PARANOID */
 	}
 	FPU_EFLAGS = (FPU_EFLAGS & ~(X86_EFLAGS_ZF | X86_EFLAGS_PF | X86_EFLAGS_CF)) | f;
 	if (c & COMP_Denormal) {

2.9.0

gcc-7.1.1 produces this warning:

arch/x86/math-emu/reg_add_sub.c: In function 'FPU_add':
arch/x86/math-emu/reg_add_sub.c:80:48: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]

This appears to be a bug in gcc-7.1.1, and I have reported it as PR81484. The compiler suggests that code written as

	if (a & b ? c : d)

is usually incorrect and should have been

	if (a & (b ? c : d))

However, in this case, we correctly write

	if ((a & b) ? c : d)

and should not get a warning for it.

This adds a dirty workaround for the problem, adding a comparison with zero inside of the macro. The warning is currently disabled in the kernel, so we may decide not to apply the patch, and instead wait for future gcc releases to fix the problem. On the other hand, it seems to be the only instance of this particular problem.

---
Originally sent on July 14, this is the same patch again with an rewritten changelog.

---
 arch/x86/math-emu/fpu_emu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index afbc4d805d66..c9c320dccca1 100644
@@ -157,7 +157,7 @@ extern u_char const data_sizes_16[32];
 
 #define signbyte(a) (((u_char *)(a))[9])
 #define getsign(a) (signbyte(a) & 0x80)
-#define setsign(a,b) { if (b) signbyte(a) |= 0x80; else signbyte(a) &= 0x7f; }
+#define setsign(a,b) { if ((b) != 0) signbyte(a) |= 0x80; else signbyte(a) &= 0x7f; }
 #define copysign(a,b) { if (getsign(a)) signbyte(b) |= 0x80; \
                         else signbyte(b) &= 0x7f; }
 #define changesign(a) { signbyte(a) ^= 0x80; }

2.9.0

The x86 version of insb/insw/insl uses an inline assembly that does not have the target buffer listed as an output. This can confuse the compiler, leading it to think that a subsequent access of the buffer is uninitialized:

drivers/net/wireless/wl3501_cs.c: In function ‘wl3501_mgmt_scan_confirm’:
drivers/net/wireless/wl3501_cs.c:665:9: error: ‘sig.status’ is used uninitialized in this function [-Werror=uninitialized] drivers/net/wireless/wl3501_cs.c:668:12: error: ‘sig.cap_info’ may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/net/sb1000.c: In function 'sb1000_rx':
drivers/net/sb1000.c:775:9: error: 'st[0]' is used uninitialized in this function [-Werror=uninitialized] drivers/net/sb1000.c:776:10: error: 'st[1]' may be used uninitialized in this function [-Werror=maybe-uninitialized] drivers/net/sb1000.c:784:11: error: 'st[1]' may be used uninitialized in this function [-Werror=maybe-uninitialized]

I tried to mark the exact input buffer as an output here, but couldn't figure it out. As suggested by Linus, marking all memory as clobbered however is good enough too. For the outs operations, I also add the memory clobber, to force the input to be written to local variables. This is probably already guaranteed by the "asm volatile", but it can't hurt to do this for symmetry.

---
v2: replace broken original RFC patch with one that probably works.
The crazy stack usage I reported in teh wl3501_cs driver with my original patch is gone too now.

---
 arch/x86/include/asm/io.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index e080a39b2108..4bc6f459a8b6 100644
@@ -328,13 +328,13 @@ static inline unsigned type in##bwl##_p(int port)			\
 static inline void outs##bwl(int port, const void *addr, unsigned long count) \
 {									\
 	asm volatile("rep; outs" #bwl					\
-		     : "+S"(addr), "+c"(count) : "d"(port));		\
+		     : "+S"(addr), "+c"(count) : "d"(port) : "memory");	\
 }									\
 									\
 static inline void ins##bwl(int port, void *addr, unsigned long count)	\
 {									\
 	asm volatile("rep; ins" #bwl					\
-		     : "+D"(addr), "+c"(count) : "d"(port));		\
+		     : "+D"(addr), "+c"(count) : "d"(port) : "memory");	\
 }
 
 BUILDIO(b, b, char)

2.9.0

[adding Linus to Cc, apparently git send-email ignores the Suggested-by tag for the cc-list]

Ack, obviously looks fine to me.

Linus

Every kernel build on x86 will result in some output:

Setup is 13084 bytes (padded to 13312 bytes).
System is 4833 kB
CRC 6d35fa35
Kernel: arch/x86/boot/bzImage is ready (#2)

This shuts it up, so that 'make -s' is truely silent as long as everything works. Building without '-s' should produce unchanged output.

---
 arch/x86/boot/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 0d810fb15eac..d88a2fddba8c 100644
@@ -73,12 +73,13 @@ UBSAN_SANITIZE := n
 $(obj)/bzImage: asflags-y  := $(SVGA_MODE)
 
 quiet_cmd_image = BUILD   $@
+silent_redirect_image = >/dev/null
 cmd_image = $(obj)/tools/build $(obj)/setup.bin $(obj)/vmlinux.bin \
-			       $(obj)/zoffset.h $@
+			       $(obj)/zoffset.h $@ $($(quiet)redirect_image)
 
 $(obj)/bzImage: $(obj)/setup.bin $(obj)/vmlinux.bin $(obj)/tools/build FORCE
 	$(call if_changed,image)
-	@echo 'Kernel: $@ is ready' ' (#'`cat .version`')'
+	@$(kecho) 'Kernel: $@ is ready' ' (#'`cat .version`')'
 
 OBJCOPYFLAGS_vmlinux.bin := -O binary -R .note -R .comment -S
 $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE

2.9.0

KVM tries to select 'TASKSTATS', which had additional dependencies:

warning: (KVM) selects TASKSTATS which has unmet direct dependencies (NET && MULTIUSER)

---
 arch/x86/kvm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 760433b2574a..2688c7dc5323 100644
@@ -22,7 +22,7 @@ config KVM
 	depends on HAVE_KVM
 	depends on HIGH_RES_TIMERS
 	# for TASKSTATS/TASK_DELAY_ACCT:
-	depends on NET
+	depends on NET && MULTIUSER
 	select PREEMPT_NOTIFIERS
 	select MMU_NOTIFIER
 	select ANON_INODES

2.9.0

2017-07-19 14:53+0200, Arnd Bergmann:

Hm, do you know why Kconfig warns instead of propagating the dependencies?

Kconfig propagates 'depends on' dependencies, but cannot turn a 'select' into 'depends on', as those two mean different things.

Another solution to the problem would be to use 'depends on TASKSTATS'.

Generally speaking, using 'select' to turn on a user-visible option is a bad idea, but blindly turning those 'select' into 'depends on' is also dangerous, as it can break configurations of existing users that would here end up with neither TASKSTATS nor KVM after a 'make oldconfig'.

Arnd

2017-07-19 16:18+0200, Arnd Bergmann:

Good point, 'select' seems misused here.

There is no reason to depend on TASKSTATS (nor NET+MULTIUSER), we only suggest to enable it with KVM. KVM uses sched_info_on() to handle any any possible resulting configuration, c9aaa8957f20 ("KVM: Steal time implementation").

KVM would work as intended if 'select' would not enable the option if its dependencies failed (instead of unconditionally forcing the option).

Is the preferred way to encode it:

'default y if KVM' in config TASK_DELAY_ACCT
(that adds a non-local and enigmatic dependency and also needlessly
expands the possible configuration space)

or

'select TASKSTATS if NET && MULTIUSER' in config KVM (that is going to break when dependencies of TASKSTATS change again)

?

thanks.

I think the former is the closest to what the user actually wants, and it would let us clean up arch/x86/kvm/Kconfig. However it should be "default y if KVM && X86'.

Maybe there is room for a new operator "suggest Y" which, when added inside "config X", operates as if "config Y" had a "default y if X".

In this case, kvm could do

- depends on NET && MULTIUSER
- select TASKSTATS
+ suggest TASKSTATS

Thanks,

Paolo

The IOSF_MBI option requires PCI support, without it we get a harmless Kconfig warning when it gets selected by PUNIT_ATOM_DEBUG:

warning: (X86_INTEL_LPSS && SND_SST_IPC_ACPI && MMC_SDHCI_ACPI && PUNIT_ATOM_DEBUG) selects IOSF_MBI which has unmet direct dependencies (PCI)

This adds another dependency to avoid the warning.

---
 arch/x86/Kconfig.debug | 1 +
 1 file changed, 1 insertion(+)

index 353ed09f5fba..1fc519f3c49e 100644
@@ -346,6 +346,7 @@ config X86_DEBUG_FPU
 
 config PUNIT_ATOM_DEBUG
 	tristate "ATOM Punit debug driver"
+	depends on PCI
 	select DEBUG_FS
 	select IOSF_MBI
 	---help---

2.9.0

We have space for exactly three characters for the index in "max7315_%d_base", but as gcc points out having more would cause an string overflow:

arch/x86/platform/intel-mid/device_libs/platform_max7315.c: In function 'max7315_platform_data':
arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:26: error: '%d' directive writing between 1 and 11 bytes into a region of size 9 [-Werror=format-overflow=]

   sprintf(base_pin_name, "max7315_%d_base", nr);
                          ^~~~~~~~~~~~~~~~~

arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:26: note: directive argument in the range [-2147483647, 2147483647] arch/x86/platform/intel-mid/device_libs/platform_max7315.c:41:3: note: 'sprintf' output between 15 and 25 bytes into a destination of size 17
sprintf(base_pin_name, "max7315_%d_base", nr);

This makes it use an snprintf() to truncate the string if that happened rather than overflowing the stack. In practice, this is safe, because there won't be a large number of max7315 devices in the systems, and both the format and the length are defined by the firmware interface.

---
Originally submitted on July 14, this is the same patch with slightly improved changelog.

---
 arch/x86/platform/intel-mid/device_libs/platform_max7315.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 6e075afa7877..58337b2bc682 100644
@@ -38,8 +38,10 @@ static void __init *max7315_platform_data(void *info)
 	 */
 	strcpy(i2c_info->type, "max7315");
 	if (nr++) {
-		sprintf(base_pin_name, "max7315_%d_base", nr);
-		sprintf(intr_pin_name, "max7315_%d_int", nr);
+		snprintf(base_pin_name, sizeof(base_pin_name),
+			 "max7315_%d_base", nr);
+		snprintf(intr_pin_name, sizeof(intr_pin_name),
+			 "max7315_%d_int", nr);
 	} else {
 		strcpy(base_pin_name, "max7315_base");
 		strcpy(intr_pin_name, "max7315_int");

2.9.0