| Message ID | YnCXTPrbLhvfRVDm@e3a974050dc4 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: dove: fix returnvar.cocci warnings | expand |
On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > From: kernel test robot <lkp@intel.com> > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > Remove unneeded variable used to store return value. > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: kernel test robot <lkp@intel.com> I checked the patch, and unfortunately it is wrong, the current code needs to stay. The problem is the SET_DMA_ERRATA() macro that accesses the local 'errata' variable. Arnd
* Arnd Bergmann <arnd@arndb.de> [220503 07:18]: > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > > > From: kernel test robot <lkp@intel.com> > > > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > > > > Remove unneeded variable used to store return value. > > > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: kernel test robot <lkp@intel.com> > > I checked the patch, and unfortunately it is wrong, the current code > needs to stay. > The problem is the SET_DMA_ERRATA() macro that accesses the > local 'errata' variable. Yeah this one keeps popping up. Maybe we can make SET_DMA_ERRATA into a function or have it at least change it to set the errata value. Regards, Tony
On Tue, May 3, 2022 at 9:30 AM Tony Lindgren <tony@atomide.com> wrote: > * Arnd Bergmann <arnd@arndb.de> [220503 07:18]: > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > > > > > From: kernel test robot <lkp@intel.com> > > > > > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > > > > > > > Remove unneeded variable used to store return value. > > > > > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: kernel test robot <lkp@intel.com> > > > > I checked the patch, and unfortunately it is wrong, the current code > > needs to stay. > > The problem is the SET_DMA_ERRATA() macro that accesses the > > local 'errata' variable. > > Yeah this one keeps popping up. Maybe we can make SET_DMA_ERRATA > into a function or have it at least change it to set the errata > value. I would just remove the macro and open-code the assignment, which I think makes it more readable to both people and tools. Arnd
* Arnd Bergmann <arnd@arndb.de> [220503 08:12]: > On Tue, May 3, 2022 at 9:30 AM Tony Lindgren <tony@atomide.com> wrote: > > * Arnd Bergmann <arnd@arndb.de> [220503 07:18]: > > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > > > > > > > From: kernel test robot <lkp@intel.com> > > > > > > > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > > > > > > > > > > Remove unneeded variable used to store return value. > > > > > > > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Signed-off-by: kernel test robot <lkp@intel.com> > > > > > > I checked the patch, and unfortunately it is wrong, the current code > > > needs to stay. > > > The problem is the SET_DMA_ERRATA() macro that accesses the > > > local 'errata' variable. > > > > Yeah this one keeps popping up. Maybe we can make SET_DMA_ERRATA > > into a function or have it at least change it to set the errata > > value. > > I would just remove the macro and open-code the assignment, which > I think makes it more readable to both people and tools. Yeah agree after looking at the macro :) Regards, Tony
On Tue, May 03, 2022 at 10:45:32AM +0800, kernel test robot wrote: > From: kernel test robot <lkp@intel.com> > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > Remove unneeded variable used to store return value. > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: kernel test robot <lkp@intel.com> NAK. The analysis is wrong.
On 5/3/22 00:21, Arnd Bergmann wrote: > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: >> From: kernel test robot <lkp@intel.com> >> >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 >> >> Remove unneeded variable used to store return value. >> >> Generated by: scripts/coccinelle/misc/returnvar.cocci >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: kernel test robot <lkp@intel.com> > I checked the patch, and unfortunately it is wrong, the current code > needs to stay. > The problem is the SET_DMA_ERRATA() macro that accesses the > local 'errata' variable. 0day folks, do we have humans looking over these before they're going out to the list? If not, can we add some? If so, can the humans get a little more discerning? ;)
On Thu, May 05, 2022 at 09:31:37AM -0700, Dave Hansen wrote: > On 5/3/22 00:21, Arnd Bergmann wrote: > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > >> From: kernel test robot <lkp@intel.com> > >> > >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > >> > >> Remove unneeded variable used to store return value. > >> > >> Generated by: scripts/coccinelle/misc/returnvar.cocci > >> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Signed-off-by: kernel test robot <lkp@intel.com> > > I checked the patch, and unfortunately it is wrong, the current code > > needs to stay. > > The problem is the SET_DMA_ERRATA() macro that accesses the > > local 'errata' variable. > > 0day folks, do we have humans looking over these before they're going > out to the list? If not, can we add some? If so, can the humans get a > little more discerning? ;) Sorry all for the bad patch. So far, we pick up several cocci warnings that we have confidence based on early result analysis and feedback, for these warnings, 0day sends out patch automatically. Thanks for the suggestion Dave, We will change current process to be more conservative and to avoid false patch by adding human analysis. Thanks
On Thu, May 05, 2022 at 03:08:40PM +0100, Russell King (Oracle) wrote: > On Tue, May 03, 2022 at 10:45:32AM +0800, kernel test robot wrote: > > From: kernel test robot <lkp@intel.com> > > > > arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > > > > Remove unneeded variable used to store return value. > > > > Generated by: scripts/coccinelle/misc/returnvar.cocci > > > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: kernel test robot <lkp@intel.com> > > NAK. The analysis is wrong. sorry about the false patch, we will improve this. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > _______________________________________________ > kbuild-all mailing list -- kbuild-all@lists.01.org > To unsubscribe send an email to kbuild-all-leave@lists.01.org
On Fri, May 6, 2022 at 3:09 AM Philip Li <philip.li@intel.com> wrote: > On Thu, May 05, 2022 at 09:31:37AM -0700, Dave Hansen wrote: > > On 5/3/22 00:21, Arnd Bergmann wrote: > > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > >> From: kernel test robot <lkp@intel.com> > > >> > > >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > >> > > >> Remove unneeded variable used to store return value. > > >> > > >> Generated by: scripts/coccinelle/misc/returnvar.cocci > > >> > > >> Reported-by: kernel test robot <lkp@intel.com> > > >> Signed-off-by: kernel test robot <lkp@intel.com> > > > I checked the patch, and unfortunately it is wrong, the current code > > > needs to stay. > > > The problem is the SET_DMA_ERRATA() macro that accesses the > > > local 'errata' variable. > > > > 0day folks, do we have humans looking over these before they're going > > out to the list? If not, can we add some? If so, can the humans get a > > little more discerning? ;) > > Sorry all for the bad patch. So far, we pick up several cocci warnings that > we have confidence based on early result analysis and feedback, for these > warnings, 0day sends out patch automatically. > > Thanks for the suggestion Dave, We will change current process to be more > conservative and to avoid false patch by adding human analysis. For the returnvar.cocci false-positives, I wonder if it's possible to find them using another coccinelle helper that detects badly formed macros which access variables out of scope. I can't think of how this would be expressed, but maybe someone has an idea. Something else went wrong in this particular patch, and I can't explain how this happened: the subject line contains the name of the wrong platform, "dove" rather than "omap2". My guess is that this was human error copying the subject line from another patch, but if this came from a script, you may want to check how this gets generated. Arnd
On Fri, 6 May 2022 at 03:12, Philip Li <philip.li@intel.com> wrote: > > On Thu, May 05, 2022 at 09:31:37AM -0700, Dave Hansen wrote: > > On 5/3/22 00:21, Arnd Bergmann wrote: > > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > >> From: kernel test robot <lkp@intel.com> > > >> > > >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > >> > > >> Remove unneeded variable used to store return value. > > >> > > >> Generated by: scripts/coccinelle/misc/returnvar.cocci > > >> > > >> Reported-by: kernel test robot <lkp@intel.com> > > >> Signed-off-by: kernel test robot <lkp@intel.com> > > > I checked the patch, and unfortunately it is wrong, the current code > > > needs to stay. > > > The problem is the SET_DMA_ERRATA() macro that accesses the > > > local 'errata' variable. > > > > 0day folks, do we have humans looking over these before they're going > > out to the list? If not, can we add some? If so, can the humans get a > > little more discerning? ;) > > Sorry all for the bad patch. So far, we pick up several cocci warnings that > we have confidence based on early result analysis and feedback, for these > warnings, 0day sends out patch automatically. > Could you please add a special header or something to such emails so I can filter them out? I am strongly opposed to such automatic spambot patch generation, as it wastes valuable reviewer bandwidth to save the bot operator some time, but it think it should be the other way around. We expect contributors to carefully prepare their patch submissions before sending them to the list, and automatically generated patches simply don't mesh with that. The fact that you use a bot does not mean you can ignore these rules.
On Fri, May 06, 2022 at 09:24:26AM +0200, Ard Biesheuvel wrote: > On Fri, 6 May 2022 at 03:12, Philip Li <philip.li@intel.com> wrote: > > > > On Thu, May 05, 2022 at 09:31:37AM -0700, Dave Hansen wrote: > > > On 5/3/22 00:21, Arnd Bergmann wrote: > > > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > > >> From: kernel test robot <lkp@intel.com> > > > >> > > > >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > >> > > > >> Remove unneeded variable used to store return value. > > > >> > > > >> Generated by: scripts/coccinelle/misc/returnvar.cocci > > > >> > > > >> Reported-by: kernel test robot <lkp@intel.com> > > > >> Signed-off-by: kernel test robot <lkp@intel.com> > > > > I checked the patch, and unfortunately it is wrong, the current code > > > > needs to stay. > > > > The problem is the SET_DMA_ERRATA() macro that accesses the > > > > local 'errata' variable. > > > > > > 0day folks, do we have humans looking over these before they're going > > > out to the list? If not, can we add some? If so, can the humans get a > > > little more discerning? ;) > > > > Sorry all for the bad patch. So far, we pick up several cocci warnings that > > we have confidence based on early result analysis and feedback, for these > > warnings, 0day sends out patch automatically. > > > > Could you please add a special header or something to such emails so I > can filter them out? I am strongly opposed to such automatic spambot > patch generation, as it wastes valuable reviewer bandwidth to save the > bot operator some time, but it think it should be the other way > around. Sorry for the trouble, we will stop sending the patch automatically and only send out patch after human confirmed/reviewed. > > We expect contributors to carefully prepare their patch submissions > before sending them to the list, and automatically generated patches > simply don't mesh with that. The fact that you use a bot does not mean > you can ignore these rules. Got it, we will improve this to follow the right way to send out patches. Thanks
On Fri, May 06, 2022 at 09:17:44AM +0200, Arnd Bergmann wrote: > On Fri, May 6, 2022 at 3:09 AM Philip Li <philip.li@intel.com> wrote: > > On Thu, May 05, 2022 at 09:31:37AM -0700, Dave Hansen wrote: > > > On 5/3/22 00:21, Arnd Bergmann wrote: > > > > On Tue, May 3, 2022 at 4:45 AM kernel test robot <lkp@intel.com> wrote: > > > >> From: kernel test robot <lkp@intel.com> > > > >> > > > >> arch/arm/mach-omap2/dma.c:82:10-16: Unneeded variable: "errata". Return "0" on line 161 > > > >> > > > >> Remove unneeded variable used to store return value. > > > >> > > > >> Generated by: scripts/coccinelle/misc/returnvar.cocci > > > >> > > > >> Reported-by: kernel test robot <lkp@intel.com> > > > >> Signed-off-by: kernel test robot <lkp@intel.com> > > > > I checked the patch, and unfortunately it is wrong, the current code > > > > needs to stay. > > > > The problem is the SET_DMA_ERRATA() macro that accesses the > > > > local 'errata' variable. > > > > > > 0day folks, do we have humans looking over these before they're going > > > out to the list? If not, can we add some? If so, can the humans get a > > > little more discerning? ;) > > > > Sorry all for the bad patch. So far, we pick up several cocci warnings that > > we have confidence based on early result analysis and feedback, for these > > warnings, 0day sends out patch automatically. > > > > Thanks for the suggestion Dave, We will change current process to be more > > conservative and to avoid false patch by adding human analysis. > > For the returnvar.cocci false-positives, I wonder if it's possible to find them > using another coccinelle helper that detects badly formed macros which > access variables out of scope. I can't think of how this would be expressed, > but maybe someone has an idea. > > Something else went wrong in this particular patch, and I can't explain > how this happened: the subject line contains the name of the wrong platform, > "dove" rather than "omap2". My guess is that this was human error copying > the subject line from another patch, but if this came from a script, you > may want to check how this gets generated. Thanks Arnd, we will investigate this to fix our side issue. And thanks for taking time to check the detail, as mentioned in other reply, we will not send out patch unless it is carefully reviewed/acked by members of 0day. > > Arnd
--- a/arch/arm/mach-omap2/dma.c +++ b/arch/arm/mach-omap2/dma.c @@ -79,8 +79,6 @@ static const struct omap_dma_reg reg_map static unsigned configure_dma_errata(void) { - unsigned errata = 0; - /* * Errata applicable for OMAP2430ES1.0 and all omap2420 * @@ -158,7 +156,7 @@ static unsigned configure_dma_errata(voi if (cpu_is_omap34xx() && (omap_type() != OMAP2_DEVICE_TYPE_GP)) SET_DMA_ERRATA(DMA_ROMCODE_BUG); - return errata; + return 0; } static const struct dma_slave_map omap24xx_sdma_dt_map[] = {