* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-06 18:27 ` Valdis.Kletnieks
@ 2014-08-06 18:35 ` Ilia Mirkin
0 siblings, 0 replies; 16+ messages in thread
From: Ilia Mirkin @ 2014-08-06 18:35 UTC (permalink / raw)
To: Valdis.Kletnieks-PjAqaU27lzQ
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
On Wed, Aug 6, 2014 at 2:27 PM, <Valdis.Kletnieks-PjAqaU27lzQ@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
@ 2014-08-06 18:35 ` Ilia Mirkin
0 siblings, 0 replies; 16+ messages in thread
From: Ilia Mirkin @ 2014-08-06 18:35 UTC (permalink / raw)
To: Valdis.Kletnieks
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-06 18:27 ` Valdis.Kletnieks
@ 2014-08-06 18:50 ` Geert Uytterhoeven
0 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2014-08-06 18:50 UTC (permalink / raw)
To: Valdis Kletnieks
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
On Wed, Aug 6, 2014 at 8:27 PM, <Valdis.Kletnieks-PjAqaU27lzQ@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] spi: Remove unused definitions
@ 2014-08-06 18:50 ` Geert Uytterhoeven
0 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2014-08-06 18:50 UTC (permalink / raw)
To: Valdis Kletnieks
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-06 18:50 ` Geert Uytterhoeven
(?)
@ 2014-08-06 19:34 ` Mark Brown
-1 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2014-08-06 19:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Valdis Kletnieks, Nick Krause, Richard Weinberger,
open list:SPI SUBSYSTEM, open list, Greg Kroah-Hartman
[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-06 18:27 ` Valdis.Kletnieks
@ 2014-08-20 20:26 ` Pavel Machek
0 siblings, 0 replies; 16+ messages in thread
From: Pavel Machek @ 2014-08-20 20:26 UTC (permalink / raw)
To: Valdis.Kletnieks-PjAqaU27lzQ
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
On Wed 2014-08-06 14:27:20, Valdis.Kletnieks-PjAqaU27lzQ@public.gmane.org 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?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
@ 2014-08-20 20:26 ` Pavel Machek
0 siblings, 0 replies; 16+ messages in thread
From: Pavel Machek @ 2014-08-20 20:26 UTC (permalink / raw)
To: Valdis.Kletnieks
Cc: Nick Krause, Richard Weinberger, Mark Brown,
open list:SPI SUBSYSTEM, open list
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?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-20 20:26 ` Pavel Machek
(?)
@ 2014-08-20 21:12 ` Valdis.Kletnieks
2014-08-20 21:56 ` Pavel Machek
-1 siblings, 1 reply; 16+ messages in thread
From: Valdis.Kletnieks @ 2014-08-20 21:12 UTC (permalink / raw)
To: Pavel Machek, linux-fsdevel, Alexander Viro
Cc: Nick Krause, Richard Weinberger, open list
[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]
(Adding Al Viro and linux-fsdevel, dropping Mark Brown and the SPI list, because this is
heading off in a different direction now)
On Wed, 20 Aug 2014 22:26:02 +0200, Pavel Machek said:
> 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?
I had the same thought, but other than a test rename to XYZZY_READ and PLUGH_WRITE
and doing a 'make allmodconfig' and seeing what throws an error, I'm not sure how
to track down all the users. On my fairly stripped-down .config, I have:
[/usr/src/linux-next] find * -name '.*.cmd' | wc -l
4671
[/usr/src/linux-next] find * -name '.*.cmd' | xargs grep include/linux/fs.h | wc -l
2339
Which is telling me that pretty much half the world ends up including fs.h indirectly.
Now for the mandatory bikeshedding:
What do we want to rename them to? :)
[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] spi: Remove unused definitions
2014-08-20 21:12 ` Valdis.Kletnieks
@ 2014-08-20 21:56 ` Pavel Machek
0 siblings, 0 replies; 16+ messages in thread
From: Pavel Machek @ 2014-08-20 21:56 UTC (permalink / raw)
To: Valdis.Kletnieks
Cc: linux-fsdevel, Alexander Viro, Nick Krause, Richard Weinberger,
open list
On Wed 2014-08-20 17:12:42, Valdis.Kletnieks@vt.edu wrote:
> (Adding Al Viro and linux-fsdevel, dropping Mark Brown and the SPI list, because this is
> heading off in a different direction now)
>
> On Wed, 20 Aug 2014 22:26:02 +0200, Pavel Machek said:
> > 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?
>
> I had the same thought, but other than a test rename to XYZZY_READ and PLUGH_WRITE
> and doing a 'make allmodconfig' and seeing what throws an error, I'm not sure how
> to track down all the users. On my fairly stripped-down .config, I have:
>
> [/usr/src/linux-next] find * -name '.*.cmd' | wc -l
> 4671
> [/usr/src/linux-next] find * -name '.*.cmd' | xargs grep include/linux/fs.h | wc -l
> 2339
>
> Which is telling me that pretty much half the world ends up including fs.h indirectly.
Yep.
I hope sh math emulator does not include fs.h.
arch/sh/math-emu/math.c:#define READ(d,a) ({if(get_user(d, (typeof (d)*)a)
> Now for the mandatory bikeshedding:
>
> What do we want to rename them to? :)
REQ_ prefix would fit there well, I'd say.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 16+ messages in thread