Hi, I just compiled my first Kernel from linus' tree and saw a warning in "sound/soc/codecs/wm8958-dsp2.c". I think a found a bug, but I am not sure. And I don`t want to waste the Kernel-dev's time on the lkml. In function wm8958_dsp2_fw(), at line 64, there is an uninitialized variable used. u32 data 32; ... /*not related code*/ ... if (memcmp(fw->data, "WMFW", 4) != 0) { dev_err(codec->dev, "%s: firmware has bad file magic %08x\n", name, data32); //shouldn't fw->data be used? goto err; } Am I right? And does this detail matter anyway? I made several small fixes for warnings and added some #ifdef CONFIG_BLA when code wasnt used. Are those changes welcome on the LKML? Thanks for your time.
On Tue, Jun 21, 2011 at 5:57 PM, Christian Deussen <chrisudeussen@googlemail.com> wrote:
Hi, I just compiled my first Kernel from linus' tree and saw a warning in "sound/soc/codecs/wm8958-dsp2.c". I think a found a bug, but I am not sure. And I don`t want to waste the Kernel-dev's time on the lkml. In function wm8958_dsp2_fw(), at line 64, there is an uninitialized variable used.
u32 data 32; ... /*not related code*/ ... if (memcmp(fw->data, "WMFW", 4) != 0) { dev_err(codec->dev, "%s: firmware has bad file magic %08x\n", name, data32); //shouldn't fw->data be used? goto err; } Am I right? And does this detail matter anyway? I made several small fixes for warnings and added some #ifdef CONFIG_BLA when code wasnt used. Are those changes welcome on the LKML? Thanks for your time.
First, every part of the kernel has a maintainer. You can look in the maintainers file to see who it is and which mailinglist to use for the code in question. Your find it in the root of your kernel source directory. Or online at http://lxr.linux.no/#linux+v2.6.39/MAINTAINERS So any patches need to go the right maintainer and the right mailing list. I for one avoid LKML itself due to the massive traffic. But I follow libata and ext4 and a couple others. That makes the process reasonable. Note your email should be to the right list and to the right maintainer. Both should be on the TO: line of your email. As to your question, I think it varies by the maintainer. If the maintainer is carrying a lot of out of tree patches for the part of the kernel he maintains, then every time he accepts a patch, it has the potential to cause those patches to no longer apply. As an example, the ext4 filesystem currently has dozens or even hundreds of patches that have been submitted fro review, or which are in the process of being developed, but which are not yet in the main kernel. A proposed patch to fix a compile warning or the code indention, etc. is likely not welcome on its own. On the other hand, if you are making a real code change which is an improvement, then submitting a patch series with the first patch or two addressing code style issues, etc. at the start of the series would be welcome. The reason being any other patches that are going to effect that same part of the code are going to need to re-based anyway due to the functional change, so putting in a cosmetic change at the same causes little extra work in the re-basing process. Hope that makes sense, even if it doesn't answer your question. At the end of the day, you might just want to send the maintainer (and the right list) a email asking if a patch like that is welcome, etc. Greg
you can use the get_maintainer.pl script: ~/linux-2.6 $ ./scripts/get_maintainer.pl -f sound/soc/codecs/wm8958-dsp2.c it will give you the maintainers names and emails On Wed, Jun 22, 2011 at 12:49 AM, Greg Freemyer <greg.freemyer@gmail.com> wrote:
On Tue, Jun 21, 2011 at 5:57 PM, Christian Deussen <chrisudeussen@googlemail.com> wrote:
Hi, I just compiled my first Kernel from linus' tree and saw a warning in "sound/soc/codecs/wm8958-dsp2.c". I think a found a bug, but I am not sure. And I don`t want to waste the Kernel-dev's time on the lkml. In function wm8958_dsp2_fw(), at line 64, there is an uninitialized variable used.
u32 data 32; ... /*not related code*/ ... if (memcmp(fw->data, "WMFW", 4) != 0) { dev_err(codec->dev, "%s: firmware has bad file magic %08x\n", name, data32); //shouldn't fw->data be used? goto err; } Am I right? And does this detail matter anyway? I made several small fixes for warnings and added some #ifdef CONFIG_BLA when code wasnt used. Are those changes welcome on the LKML? Thanks for your time.
First, every part of the kernel has a maintainer. You can look in the maintainers file to see who it is and which mailinglist to use for the code in question.
Your find it in the root of your kernel source directory. Or online at http://lxr.linux.no/#linux+v2.6.39/MAINTAINERS
So any patches need to go the right maintainer and the right mailing list. I for one avoid LKML itself due to the massive traffic. But I follow libata and ext4 and a couple others. That makes the process reasonable.
Note your email should be to the right list and to the right maintainer. Both should be on the TO: line of your email.
As to your question, I think it varies by the maintainer.
If the maintainer is carrying a lot of out of tree patches for the part of the kernel he maintains, then every time he accepts a patch, it has the potential to cause those patches to no longer apply.
As an example, the ext4 filesystem currently has dozens or even hundreds of patches that have been submitted fro review, or which are in the process of being developed, but which are not yet in the main kernel.
A proposed patch to fix a compile warning or the code indention, etc. is likely not welcome on its own.
On the other hand, if you are making a real code change which is an improvement, then submitting a patch series with the first patch or two addressing code style issues, etc. at the start of the series would be welcome.
The reason being any other patches that are going to effect that same part of the code are going to need to re-based anyway due to the functional change, so putting in a cosmetic change at the same causes little extra work in the re-basing process.
Hope that makes sense, even if it doesn't answer your question.
At the end of the day, you might just want to send the maintainer (and the right list) a email asking if a patch like that is welcome, etc.
Greg
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Hi Christian, Here's the most recent conversation I found (last month) on LKML regarding this file where someone sent a patch, you may find it helpful to read through the thread to see what the maintainers said. https://lkml.org/lkml/2011/5/6/353 HTH, Julie
Hmm, I just enabled the 'Build all ASoC CODEC drivers' option (I don't have the hardware) to build-test to see if I could get the same warning as Christian and I didn't, but I did get this in the same tree: sound/soc/codecs/ak4641.c: In function ‘ak4641_probe’: sound/soc/codecs/ak4641.c:524:6: error: ‘GPIOF_OUT_INIT_LOW’ undeclared (first use in this function) sound/soc/codecs/ak4641.c:524:6: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [sound/soc/codecs/ak4641.o] Error 1 make[2]: *** [sound/soc/codecs] Error 2 make[1]: *** [sound/soc] Error 2 make: *** [sound] Error 2 make: *** Waiting for unfinished jobs.... Odd. Is there some other CONFIG_ I'm not setting that I need to for your driver, Christian? (I'm using -rc3, -rc4 won't boot but that's another problem...) --- btw if you _are_ sending a patch and it's your first one: remember you can avoid the embarrassment of maintainers experiencing your tabs to space conversion, word-wrapping and other reformatting/corruption horrors by sending yourself the patch first to check it applies - or send it to us, I'm happy to 'apply check' it for you if you can let me know what kernel version to check out (can't promise I'll do it the same day though) :-) If you are thinking of using the Gmail web gui to send a patch be sure to read Documentation/email-clients.txt with special reference to the bit at the bottom of that file... Cheers Julie
Hi, thanks for your help. I a m using -rc3 on an Ubuntu system. I used "make loadmodconfig" to create my .config. I then enabled Kernel debugging. Should I append my .config file? The bug/warning seems to be added by this commit. http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h... . I would love to contribute a patch but it I think I better wait until I found a bigger/more problem. 2011/6/22 julie Sullivan <kernelmail.jms@gmail.com>
Hmm, I just enabled the 'Build all ASoC CODEC drivers' option (I don't have the hardware) to build-test to see if I could get the same warning as Christian and I didn't, but I did get this in the same tree:
sound/soc/codecs/ak4641.c: In function ‘ak4641_probe’: sound/soc/codecs/ak4641.c:524:6: error: ‘GPIOF_OUT_INIT_LOW’ undeclared (first use in this function) sound/soc/codecs/ak4641.c:524:6: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [sound/soc/codecs/ak4641.o] Error 1 make[2]: *** [sound/soc/codecs] Error 2 make[1]: *** [sound/soc] Error 2 make: *** [sound] Error 2 make: *** Waiting for unfinished jobs....
In my config 'Build all ASoC CODEC drivers' is selected to be build as module, but I can not reproduce your error. Isn't it bad that more than 20 warnings are produced during a kernel build?
On Thu, Jun 23, 2011 at 02:16:05PM +0200, Christian D. wrote:
Hi, thanks for your help. I a m using -rc3 on an Ubuntu system. I used "make loadmodconfig" to create my .config. I then enabled Kernel debugging. Should I append my .config file? The bug/warning seems to be added by this commit. http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h... . I would love to contribute a patch but it I think I better wait until I found a bigger/more problem.
Well, I think, you don't need to. I have seven patches in the kernel, four of which are just spelling fixes [1]. What you need is an explanation coming with every patch you send; again, if you're not sure just send your patch to this list (kernelnewbies) and someone will comment on it. [1] http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git&a=s... HTH, Jonathan Neuschäfer
On Tue, Jun 21, 2011 at 11:57:02PM +0200, Christian Deussen wrote:
Hi, I just compiled my first Kernel from linus' tree and saw a warning in "sound/soc/codecs/wm8958-dsp2.c". I think a found a bug, but I am not sure. And I don`t want to waste the Kernel-dev's time on the lkml. In function wm8958_dsp2_fw(), at line 64, there is an uninitialized variable used.
u32 data 32; ... /*not related code*/ ...
if (memcmp(fw->data, "WMFW", 4) != 0) { dev_err(codec->dev, "%s: firmware has bad file magic %08x\n", name, data32); //shouldn't fw->data be used? goto err; }
Unfortunately, printk("%08x", fw->data) will print 32 bit of the pointer fw->data, not the first 4 bytes of where it points. (CMIIW). One (somewhat unelegant) solution would be: dev_err(codec->dev, "%s: firmware has bad file magic %02x%02x%02x%02x\n", name, fw->data[0], fw->data[1], fw->data[2], fw->data[3]); Maybe a fix like this would be appropriate: diff --git a/sound/soc/codecs/wm8958-dsp2.c b/sound/soc/codecs/wm8958-dsp2.c index 0293763..9d92de5 100644 --- a/sound/soc/codecs/wm8958-dsp2.c +++ b/sound/soc/codecs/wm8958-dsp2.c @@ -59,6 +59,9 @@ static int wm8958_dsp2_fw(struct snd_soc_codec *codec, const char *name, goto err; } + memcpy(&data32, fw->data, sizeof(data32)); + data32 = be32_to_cpu(data32); + if (memcmp(fw->data, "WMFW", 4) != 0) { dev_err(codec->dev, "%s: firmware has bad file magic %08x\n", name, data32); HTH, Jonathan Neuschäfer
participants (6)
-
Christian D. -
Christian Deussen
-
Greg Freemyer -
Jonathan Neuschäfer -
julie Sullivan -
Wilson Felipe