Message ID | 20190921231022.kawfomtmka737arq@pburton-laptop (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | [GIT,PULL] MIPS changes | expand |
The pull request you sent on Sat, 21 Sep 2019 23:10:24 +0000:
> git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git tags/mips_5.4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5c6bd5de3c2e5bc8a17451e281ed2613375a7fd5
Thank you!
On Sat, Sep 21, 2019 at 4:10 PM Paul Burton <paul.burton@mips.com> wrote: > > Here are the main MIPS changes for v5.4; please pull. Hmm. I pulled and because initial tests didn't show any issues, I already pushed out. But some unrelated further testing then shows that this: > Florian Fainelli (2): > firmware: bcm47xx_nvram: Correct size_t printf format > firmware: bcm47xx_nvram: Allow COMPILE_TEST causes problems, and commit feb4eb060c3a ("firmware: bcm47xx_nvram: Correct size_t printf format") is buggy: drivers/firmware/broadcom/bcm47xx_nvram.c: In function ‘nvram_init’: drivers/firmware/broadcom/bcm47xx_nvram.c:151: warning: format ‘%zu’ expects argument of type ‘size_t’, but argument 2 has type ‘u32’ {aka ‘unsigned int’} [-Wformat=] and the change to use %zu was completely wrong. It prints out 'header.len', which is an u32, not nvram_len which is a size_t. Tssk tssk. I've fixed it in my tree, but this should have shown up in linux-next, or in MIPS testing. The process clearly failed. Linus
On 9/22/2019 11:35 AM, Linus Torvalds wrote: > On Sat, Sep 21, 2019 at 4:10 PM Paul Burton <paul.burton@mips.com> wrote: >> >> Here are the main MIPS changes for v5.4; please pull. > > Hmm. I pulled and because initial tests didn't show any issues, I > already pushed out. > > But some unrelated further testing then shows that this: > >> Florian Fainelli (2): >> firmware: bcm47xx_nvram: Correct size_t printf format >> firmware: bcm47xx_nvram: Allow COMPILE_TEST > > causes problems, and commit feb4eb060c3a ("firmware: bcm47xx_nvram: > Correct size_t printf format") is buggy: > > drivers/firmware/broadcom/bcm47xx_nvram.c: In function ‘nvram_init’: > drivers/firmware/broadcom/bcm47xx_nvram.c:151: warning: format ‘%zu’ > expects argument of type ‘size_t’, but argument 2 has type ‘u32’ {aka > ‘unsigned int’} [-Wformat=] > > and the change to use %zu was completely wrong. > > It prints out 'header.len', which is an u32, not nvram_len which is a size_t. > > Tssk tssk. > > I've fixed it in my tree, but this should have shown up in linux-next, > or in MIPS testing. The process clearly failed. Thanks for fixing that. The process worked, there was an email sent by the kbuild robot but I saw it only now somehow and failed to address it in time before Paul sent out the pull request.
Hi Linus, On Sun, Sep 22, 2019 at 11:35:36AM -0700, Linus Torvalds wrote: > On Sat, Sep 21, 2019 at 4:10 PM Paul Burton <paul.burton@mips.com> wrote: > > > > Here are the main MIPS changes for v5.4; please pull. > > Hmm. I pulled and because initial tests didn't show any issues, I > already pushed out. > > But some unrelated further testing then shows that this: > > > Florian Fainelli (2): > > firmware: bcm47xx_nvram: Correct size_t printf format > > firmware: bcm47xx_nvram: Allow COMPILE_TEST > > causes problems, and commit feb4eb060c3a ("firmware: bcm47xx_nvram: > Correct size_t printf format") is buggy: > > drivers/firmware/broadcom/bcm47xx_nvram.c: In function ‘nvram_init’: > drivers/firmware/broadcom/bcm47xx_nvram.c:151: warning: format ‘%zu’ > expects argument of type ‘size_t’, but argument 2 has type ‘u32’ {aka > ‘unsigned int’} [-Wformat=] > > and the change to use %zu was completely wrong. > > It prints out 'header.len', which is an u32, not nvram_len which is a size_t. > > Tssk tssk. Oopsie. > I've fixed it in my tree, but this should have shown up in linux-next, > or in MIPS testing. The process clearly failed. Looking back I do see that the kbuild test robot reported an issue here, so one aspect of the failure is squarely on my not having caught up on email properly yet. Another issue is that there are currently 'expected' warnings dotted through the tree for various defconfigs, so whilst I do perform test builds before submitting pull requests I generally don't pay too much attention to warnings... So an improvement I'll look at is fixing up those warnings & building with -Werror, or at least not ignoring warnings. My apologies and thanks for fixing this up, Paul
On Mon, Sep 23, 2019 at 11:07 AM Paul Burton <paul.burton@mips.com> wrote: > > Another issue is that there are currently 'expected' warnings dotted > through the tree for various defconfigs This is why I refuse to have _any_ warnings at all in my tree during the merge window. If you have expected warnings, you will ignore the new and valid ones. So the only acceptable situation is "no warnings". In honesty, I actually do have one warning in my tree: samples/vfs/test-statx.c:24:15: warning: ‘struct foo’ declared inside parameter list will not be visible outside of this definition or declaration 24 | #define statx foo | ^~~ but because it's in the sample code, it pretty much never gets rebuilt for me unless I basically do a "git clean" to get rid of everything, so I don't normally see it for any normal pull. So I've ignored that one warning, although I've actually been tempted to just remove the sample because of it. Adding David and Al to the cc just in case they have some simple fixup for it that is likely to work across different user headers. I considered just adding a struct foo; declaration, but the whole thing is incredibly ugly. Linus
Linus Torvalds <torvalds@linux-foundation.org> wrote: > In honesty, I actually do have one warning in my tree: > > samples/vfs/test-statx.c:24:15: warning: ‘struct foo’ declared > inside parameter list will not be visible outside of this definition > or declaration > 24 | #define statx foo > | ^~~ Were there any note lines from the compiler associated with this? The warning message can't actually be taking place on this line. Another thing I'm wondering is why your compiler shows this warning - and mine does not. I've seen this before with uninitialised variables too where you get a warning and I don't. > but because it's in the sample code, it pretty much never gets rebuilt > for me unless I basically do a "git clean" to get rid of everything, > so I don't normally see it for any normal pull. > > So I've ignored that one warning, although I've actually been tempted > to just remove the sample because of it. > > Adding David and Al to the cc just in case they have some simple fixup > for it that is likely to work across different user headers. > > I considered just adding a > > struct foo; > > declaration, but the whole thing is incredibly ugly. Yeah - I'm not sure the best way to deal with this. The problem being that userspace may have a conflicting definition - or no definition at all - and I need to use the one from the kernel in preference as that may have changes in it that aren't yet reflected in userspace. I'd rather not remove the sample if I can avoid it as I use it occasionally, but maybe I should switch to relying on glibc. David
On Tue, Sep 24, 2019 at 5:40 AM David Howells <dhowells@redhat.com> wrote: > > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > In honesty, I actually do have one warning in my tree: > > > > samples/vfs/test-statx.c:24:15: warning: ‘struct foo’ declared > > inside parameter list > > Were there any note lines from the compiler associated with this? The warning > message can't actually be taking place on this line. That's the only thing that gcc says. I agree that it's not where the problem occurs, but the gcc warning system tries to avoid warning inside system header files, so it seems to have logic tracing it back to the user. But I have system header files that look like this: /* Fill *BUF with information about PATH in DIRFD. */ int statx (int __dirfd, const char *__restrict __path, int __flags, unsigned int __mask, struct statx *__restrict __buf) __THROW __nonnull ((2, 5)); and I think that's the one that triggers. You must have hit *something* similar too, since the only reason for that #define statx foo #define statx_timestamp foo_timestamp #include <sys/stat.h> #undef statx #undef statx_timestamp is that you're playing games with the kernel 'statx' clashing with user 'statx' use. And what I think happens is that you had the <sys/types.h> include *without* that #define, so the 'struct statx' got declared there, and then in <sys/stat.h> it gets used, but it gets used as 'struct foo', so now the compiler complains (properly) that you're using this undeclared 'struct foo' in the function declaration, and because of namespace rules it's not the same thing as then a later 'struct foo' would be. Linus Linus
diff --cc Documentation/mips/index.rst index fd9023c8a89f,321b4794f3b8..3616fb872af3 --- a/Documentation/mips/index.rst diff --cc arch/mips/Kconfig index 904c096fa4da,2f7c050e8cde..cc8e2b1032a5 --- a/arch/mips/Kconfig diff --cc drivers/video/fbdev/Makefile index aab7155884ea,49502d6256cb..aa6352798cf4 --- a/drivers/video/fbdev/Makefile