| Message ID | 1407347597-2168-1-git-send-email-xerofoiffy@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9f5b8b4f56dd194fd33021810636879036d2acdd |
| Headers | show |
On Wed, 06 Aug 2014 13:53:17 -0400, Nick Krause said: > Remove unused definition which cause the following warnings > > drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] > include/linux/fs.h:193:0: note: this is the location of the previous definition > drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] > include/linux/fs.h:192:0: note: this is the location of the previous definition > -#define WRITE 0 > -#define READ 1 NAK. Full stop. These are potentially used in an inner macro someplace, and by removing these, the conflicting values from fs.h will be used instead. #define READ 0 #define WRITE RW_MASK So if there *is* a use in an inner macro, you just screwed the pooch and introduced a bug in this "clean up" - somebody will be expecting to see a 0 for a READ, and will receive a 1 instead. This can't end well. Nick - how *exactly* did you identify that these are in fact not used? Given your history of submitting poorly researched patches, you're going to have to justify the "unused" better than the handwaving you've done here.
On Wed, Aug 06, 2014 at 01:53:17PM -0400, Nick Krause wrote: > Remove unused definition which cause the following warnings > > drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] > include/linux/fs.h:193:0: note: this is the location of the previous definition > drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] > include/linux/fs.h:192:0: note: this is the location of the previous definition > > Signed-off-by: Nick Krause <xerofoiffy@gmail.com> > --- > drivers/spi/spi-omap-100k.c | 4 ---- > 1 file changed, 4 deletions(-) Nick, you were warned about this numerous times in the past. By changing email addresses you have now ensured that we will never listen to you, as you obviously are not listening to us. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 6, 2014 at 2:27 PM, <Valdis.Kletnieks@vt.edu> wrote: > On Wed, 06 Aug 2014 13:53:17 -0400, Nick Krause said: >> Remove unused definition which cause the following warnings >> >> drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] >> include/linux/fs.h:193:0: note: this is the location of the previous definition >> drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] >> include/linux/fs.h:192:0: note: this is the location of the previous definition > >> -#define WRITE 0 >> -#define READ 1 > > NAK. Full stop. These are potentially used in an inner macro someplace, and by > removing these, the conflicting values from fs.h will be used instead. > > #define READ 0 > #define WRITE RW_MASK > > So if there *is* a use in an inner macro, you just screwed the pooch > and introduced a bug in this "clean up" - somebody will be expecting to see > a 0 for a READ, and will receive a 1 instead. This can't end well. I was actually about to send a similar response, but then I noticed I couldn't actually find any uses of READ/WRITE in the file... But I could easily have missed them. > > Nick - how *exactly* did you identify that these are in fact not used? > Given your history of submitting poorly researched patches, you're going to > have to justify the "unused" better than the handwaving you've done here. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 6, 2014 at 8:27 PM, <Valdis.Kletnieks@vt.edu> wrote: > On Wed, 06 Aug 2014 13:53:17 -0400, Nick Krause said: >> Remove unused definition which cause the following warnings >> >> drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] >> include/linux/fs.h:193:0: note: this is the location of the previous definition >> drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] >> include/linux/fs.h:192:0: note: this is the location of the previous definition > >> -#define WRITE 0 >> -#define READ 1 > > NAK. Full stop. These are potentially used in an inner macro someplace, and by > removing these, the conflicting values from fs.h will be used instead. > > #define READ 0 > #define WRITE RW_MASK > > So if there *is* a use in an inner macro, you just screwed the pooch > and introduced a bug in this "clean up" - somebody will be expecting to see > a 0 for a READ, and will receive a 1 instead. This can't end well. > > Nick - how *exactly* did you identify that these are in fact not used? > Given your history of submitting poorly researched patches, you're going to > have to justify the "unused" better than the handwaving you've done here. Just looking into this... It seems READ and WRITE are not used at all in this file. To be 100% sure, I tried to find with which kernel config this warning shows up. It doesn't happen for omap1_defconfig with CONFIG_SPI_OMAP_100K, which was the most likely culprit. It does happen with sparc/sparc64 allmodconfig. However, changing or removing the READ and WRITE definitions in drivers/spi/spi-omap-100k.c doesn't have any influence on the preprocessed source files ("make drivers/spi/spi-omap-100k.i", modulo line number changes), for both omap1 and sparc builds. (Nick: I believe the above is what people really want to see) So I conclude they are really not used, and they can be safely removed. Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 06, 2014 at 11:33:19AM -0700, Greg KH wrote: > On Wed, Aug 06, 2014 at 01:53:17PM -0400, Nick Krause wrote: > > Remove unused definition which cause the following warnings > > > > drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] > > include/linux/fs.h:193:0: note: this is the location of the previous definition > > drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] > > include/linux/fs.h:192:0: note: this is the location of the previous definition > > > > Signed-off-by: Nick Krause <xerofoiffy@gmail.com> > > --- > > drivers/spi/spi-omap-100k.c | 4 ---- > > 1 file changed, 4 deletions(-) > > Nick, you were warned about this numerous times in the past. > > By changing email addresses you have now ensured that we will never > listen to you, as you obviously are not listening to us. > He does manage to get our attention, though. Also, it is not that simple - for my part I keep looking out for the patches just to make sure that we don't get any more less than well researched "fixes" into the kernel. I figure it pays to spend some time now instead of having to clean up the resulting mess if something slips by. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 06, 2014 at 08:50:15PM +0200, Geert Uytterhoeven wrote: > To be 100% sure, I tried to find with which kernel config this warning shows up. > It doesn't happen for omap1_defconfig with CONFIG_SPI_OMAP_100K, > which was the most likely culprit. > It does happen with sparc/sparc64 allmodconfig. However, changing or > removing the READ and WRITE definitions in drivers/spi/spi-omap-100k.c > doesn't have any influence on the preprocessed source files > ("make drivers/spi/spi-omap-100k.i", modulo line number changes), > for both omap1 and sparc builds. > (Nick: I believe the above is what people really want to see) > So I conclude they are really not used, and they can be safely removed. > Acked-by: Geert Uytterhoeven <geert@linux-m68k.org> Right, I'd already done the same analysis myself with looking for uses myself (though I didn't go looking for how to trigger). Nick, please do look at the analysis Geert provided above - it is indeed exactly the sort of thing that people would want to see, just saying what's in the diff isn't enough especially given the well advertised quality problems with your submissions. You really need to explain why the change you're making is a good idea. I've applied the change.
On Wed 2014-08-06 14:27:20, Valdis.Kletnieks@vt.edu wrote: > On Wed, 06 Aug 2014 13:53:17 -0400, Nick Krause said: > > Remove unused definition which cause the following warnings > > > > drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] > > include/linux/fs.h:193:0: note: this is the location of the previous definition > > drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] > > include/linux/fs.h:192:0: note: this is the location of the previous definition > > > -#define WRITE 0 > > -#define READ 1 > > NAK. Full stop. These are potentially used in an inner macro someplace, and by > removing these, the conflicting values from fs.h will be used instead. > > #define READ 0 > #define WRITE RW_MASK > > So if there *is* a use in an inner macro, you just screwed the pooch > and introduced a bug in this "clean up" - somebody will be expecting to see > a 0 for a READ, and will receive a 1 instead. This can't end well. Actually.. having macros called READ and WRITE in fs.h is already something I'd say can't end well. Can we rename those?
diff --git a/drivers/spi/spi-omap-100k.c b/drivers/spi/spi-omap-100k.c index 5e91858..fb52276 100644 --- a/drivers/spi/spi-omap-100k.c +++ b/drivers/spi/spi-omap-100k.c @@ -70,10 +70,6 @@ #define SPI_STATUS_WE (1UL << 1) #define SPI_STATUS_RD (1UL << 0) -#define WRITE 0 -#define READ 1 - - /* use PIO for small transfers, avoiding DMA setup/teardown overhead and * cache operations; better heuristics consider wordsize and bitrate. */
Remove unused definition which cause the following warnings drivers/spi/spi-omap-100k.c:73:0: warning: "WRITE" redefined [enabled by default] include/linux/fs.h:193:0: note: this is the location of the previous definition drivers/spi/spi-omap-100k.c:74:0: warning: "READ" redefined [enabled by default] include/linux/fs.h:192:0: note: this is the location of the previous definition Signed-off-by: Nick Krause <xerofoiffy@gmail.com> --- drivers/spi/spi-omap-100k.c | 4 ---- 1 file changed, 4 deletions(-)