On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck linux@roeck-us.net wrote:
This patch results in:
arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus': arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result'
when building sh:defconfig. Checking for calls to request_irq() suggests that there will be other similar errors in various builds. Reverting the patch fixes the problem.
Which ones? From a quick grep and some filtering I could only find one file with wrong usage apart from this one:
drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv); drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
Of course, this does not cover other functions, but it means there aren't many issues and/or people building the code if nobody complains within a few weeks. So I think we can fix them as they come.
Cheers, Miguel
On 12/12/20 9:04 PM, Miguel Ojeda wrote:
On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck linux@roeck-us.net wrote:
This patch results in:
arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus': arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result'
when building sh:defconfig. Checking for calls to request_irq() suggests that there will be other similar errors in various builds. Reverting the patch fixes the problem.
Which ones? From a quick grep and some filtering I could only find one file with wrong usage apart from this one:
drivers/net/ethernet/lantiq_etop.c:request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv); drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv);
Of course, this does not cover other functions, but it means there aren't many issues and/or people building the code if nobody complains within a few weeks. So I think we can fix them as they come.
Witz komm raus, Du bist umzingelt.
The key here is "if nobody complains". I would argue that it is _your_ responsibility to do those builds, and not the reponsibility of others to do it for you.
But, sure, your call. Please feel free to ignore my report.
Guenter
On Sun, Dec 13, 2020 at 1:55 PM Guenter Roeck linux@roeck-us.net wrote:
Witz komm raus, Du bist umzingelt.
Please, explain this reference. :-)
The key here is "if nobody complains". I would argue that it is _your_ responsibility to do those builds, and not the reponsibility of others to do it for you.
Testing allmodconfig for a popular architecture, agreed, it is due diligence to avoid messing -next that day.
Testing a matrix of configs * arches * gcc/clang * compiler versions? No, sorry, that is what CI/-next/-rcs are for and that is where the "if nobody complains" comes from.
If you think building a set of code for a given arch/config/etc. is particularly important, then it is _your_ responsibility to build it once in a while in -next (as you have done). If it is not that important, somebody will speak up in one -rc. If not, is anyone actually building that code at all?
Otherwise, changing core/shared code would be impossible. Please don't blame the author for making a sensible change that will improve code quality for everyone.
But, sure, your call. Please feel free to ignore my report.
I'm not ignoring the report, quite the opposite. I am trying to understand why you think reverting is needed for something that has been more than a week in -next without any major breakage and still has a long road to v5.11.
Cheers, Miguel
On Sun, Dec 13, 2020 at 03:58:20PM +0100, Miguel Ojeda wrote:
The key here is "if nobody complains". I would argue that it is _your_ responsibility to do those builds, and not the reponsibility of others to do it for you.
Testing allmodconfig for a popular architecture, agreed, it is due diligence to avoid messing -next that day.
Testing a matrix of configs * arches * gcc/clang * compiler versions? No, sorry, that is what CI/-next/-rcs are for and that is where the "if nobody complains" comes from.
If you think building a set of code for a given arch/config/etc. is particularly important, then it is _your_ responsibility to build it once in a while in -next (as you have done). If it is not that important, somebody will speak up in one -rc. If not, is anyone actually building that code at all?
Otherwise, changing core/shared code would be impossible. Please don't blame the author for making a sensible change that will improve code quality for everyone.
But, sure, your call. Please feel free to ignore my report.
I'm not ignoring the report, quite the opposite. I am trying to understand why you think reverting is needed for something that has been more than a week in -next without any major breakage and still has a long road to v5.11.
Because if you get a report of something breaking for your change, you need to work to resolve it, not argue about it. Otherwise it needs to be dropped/reverted.
Please fix.
thanks,
greg k-h
On Sun, Dec 13, 2020 at 4:16 PM Greg KH gregkh@linuxfoundation.org wrote:
Because if you get a report of something breaking for your change, you need to work to resolve it, not argue about it. Otherwise it needs to be dropped/reverted.
Nobody has argued that. In fact, I explicitly said the opposite: "So I think we can fix them as they come.".
I am expecting Masahiro to follow up. It has been less than 24 hours since the report, on a weekend.
Cheers, Miguel
On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, Dec 13, 2020 at 4:16 PM Greg KH gregkh@linuxfoundation.org wrote:
Because if you get a report of something breaking for your change, you need to work to resolve it, not argue about it. Otherwise it needs to be dropped/reverted.
Nobody has argued that. In fact, I explicitly said the opposite: "So I think we can fix them as they come.".
I am expecting Masahiro to follow up. It has been less than 24 hours since the report, on a weekend.
Cheers, Miguel
Sorry for the delay.
Now I sent out the fix for lantiq_etop.c
https://lore.kernel.org/patchwork/patch/1355595/
The reason of the complication was I was trying to merge the following patch in the same development cycle: https://patchwork.kernel.org/project/linux-kbuild/patch/20201117104736.24997...
-Werror=return-type gives a bigger impact because any instance of __must_check violation results in build breakage. So, I just dropped it from my tree (and, I will aim for 5.12).
The removal of CONFIG_ENABLE_MUST_CHECK is less impactive, because we are still able to build with some warnings.
Tomorrow's linux-next should be OK and, you can send my patch in this merge window.
On 12/20/20 10:18 PM, Masahiro Yamada wrote:
On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sun, Dec 13, 2020 at 4:16 PM Greg KH gregkh@linuxfoundation.org wrote:
Because if you get a report of something breaking for your change, you need to work to resolve it, not argue about it. Otherwise it needs to be dropped/reverted.
Nobody has argued that. In fact, I explicitly said the opposite: "So I think we can fix them as they come.".
I am expecting Masahiro to follow up. It has been less than 24 hours since the report, on a weekend.
Cheers, Miguel
Sorry for the delay.
Now I sent out the fix for lantiq_etop.c
next-20201218, alpha:allmodconfig:
fs/binfmt_em86.c: In function 'load_em86': fs/binfmt_em86.c:66:2: error: ignoring return value of 'remove_arg_zero' declared with attribute 'warn_unused_result'
With a change like this, I'd have expected that there is a coccinelle script or similar to ensure that claims made in the commit message are true.
Guenter
On Mon, Dec 21, 2020 at 11:02 AM Guenter Roeck linux@roeck-us.net wrote:
On 12/20/20 10:18 PM, Masahiro Yamada wrote: With a change like this, I'd have expected that there is a coccinelle script or similar to ensure that claims made in the commit message are true.
It is only a warning -- the compiler already tells us what is wrong.
Cheers, Miguel
On Mon, Dec 21, 2020 at 7:20 AM Masahiro Yamada masahiroy@kernel.org wrote:
Sorry for the delay.
No problem!
Now I sent out the fix for lantiq_etop.c
I saw it, thanks for the Cc!
The reason of the complication was I was trying to merge the following patch in the same development cycle: https://patchwork.kernel.org/project/linux-kbuild/patch/20201117104736.24997...
Ah, then that explains why Guenter's had an error instead of a warning.
Tomorrow's linux-next should be OK and, you can send my patch in this merge window.
Thanks!
Cheers, Miguel
Miguel Ojeda wrote:
I think we can fix them as they come.
If your change to a function breaks its callers, it's your job to fix the callers proactively instead of waiting for "as they come" bug reports. (Assuming, of course, that you know about the breakage. Which you do when you tell us that the bad pattern can simply be grepped for.)
If nothing else, that's far more efficient than [number_of_callers] separate patches by other people who each need to find the offending change, figure out what to change and/or who to report the problem to, and so on until the fix lands in the kernel.
Moreover, this wouldn't leave the kernel sources in a non-bisect-able state during that time.
On Sun, Dec 13, 2020 at 4:38 PM 'Matthias Urlichs' via Clang Built Linux clang-built-linux@googlegroups.com wrote:
If your change to a function breaks its callers, it's your job to fix
No function has changed. This patch enables a warning (that for some reason is an error in the case of Guenter).
Even if this was a hard error, the same applies: the function hasn't changed. It just means callers never tested with `CONFIG_ENABLE_MUST_CHECK` for *years*.
the callers proactively instead of waiting for "as they come" bug reports. (Assuming, of course, that you know about the breakage. Which you do when you tell us that the bad pattern can simply be grepped for.)
No, *we don't know about the breakage*. The grep was for the particular function Guenter reported, and done to validate his concern.
If you want to manually inspect every caller of every `__must_check` function, or to write a cocci patch or a clang-tidy check or similar (that would be obsolete as soon as `__must_check` is enabled), you are welcome to do so. But a much better usage of our time would be letting machines do their job.
If nothing else, that's far more efficient than [number_of_callers] separate patches by other people who each need to find the offending change, figure out what to change and/or who to report the problem to, and so on until the fix lands in the kernel.
This change is not in Linus' tree, it is on -next.
Moreover, this wouldn't leave the kernel sources in a non-bisect-able state during that time.
Again, the change is in -next. That is the point: to do integration testing and let the bots run against it.
Cheers, Miguel
linux-kselftest-mirror@lists.linaro.org
-
Greg KH -
Guenter Roeck -
Masahiro Yamada -
Matthias Urlichs -
Miguel Ojeda