| Message ID | 20190619151248.75618-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi Andy, On Wed, 19 Jun 2019 18:12:48 +0300, Andy Shevchenko wrote: > There is no need to include linux/init.h when at the same time > we include linux/module.h. > > Remove redundant inclusion. > > For more details, refer to the commit > 0fd972a7d91d ("module: relocate module_init from init.h to module.h") > where the split had been introduced. I've read it. It's not a split, it's a move. A move which makes sense because, as explained in the commit message, module_init() is only needed by modular code, so including it in init.h was slowing down the pre-processing of non-modular code. That being said, this alone does not imply that explicit inclusion of both linux/init.h and linux/module.h in the same file is wrong. The only case where this commit would directly lead to the removal of #include <linux/init.h> from i2c-i801.c is if module_init() was the only thing from linux/init.h that this driver was using. Which is not the case, as I see various occurrences of __init and __exit left, and these are declared in linux/init.h. As a matter of fact I still count 3921 driver files which include both linux/init.h and linux/module.h. And I see no comment in either header file that including one exempts you from including the other. So I'm not taking this change, sorry. If this is really the direction you want us to take (and I'm not convinced, but my opinion does not necessarily matter), then it must be documented first, and I believe it should then be addressed tree-wide. 3921 individual commits do not seem to be the most efficient to get it done. > Cc: Paul Gortmaker <paul.gortmaker@windriver.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index b9c5d7933d12..69c3ccb69669 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -85,7 +85,6 @@ > #include <linux/stddef.h> > #include <linux/delay.h> > #include <linux/ioport.h> > -#include <linux/init.h> > #include <linux/i2c.h> > #include <linux/i2c-smbus.h> > #include <linux/acpi.h>
[Re: [PATCH v2 2/2] i2c: i801: Remove linux/init.h and sort headers] On 20/06/2019 (Thu 15:24) Jean Delvare wrote: > Hi Andy, > > On Wed, 19 Jun 2019 18:12:48 +0300, Andy Shevchenko wrote: > > There is no need to include linux/init.h when at the same time > > we include linux/module.h. > > > > Remove redundant inclusion. > > > > For more details, refer to the commit > > 0fd972a7d91d ("module: relocate module_init from init.h to module.h") > > where the split had been introduced. > > I've read it. It's not a split, it's a move. A move which makes sense > because, as explained in the commit message, module_init() is only > needed by modular code, so including it in init.h was slowing down the > pre-processing of non-modular code. > > That being said, this alone does not imply that explicit inclusion of > both linux/init.h and linux/module.h in the same file is wrong. The Agreed. If one looks at all the many follow-on changes that relocation enabled, they will see that related changes are a "downgrade" of module.h to init.h inclusion - to reduce CPP overhead as stated above. To be clear, that kind of change does NOT introduce any implicit header use, but the reverse: init.h ---> module.h DOES introduce implicit use. I've never intentionally removed init.h from a file just because module.h was already present. I don't think implicit header inclusion is a good thing. It leads to random compile failures that pop up depending on what is in your .config file and/or what architecture you are building for, etc etc. Those issues wont happen from the patch proposed here, since module.h does explictly draw in init.h -- but that still doesn't make it right. Note that solving all cases of implicit use of init.h is a totally different problem space than what I set out to tackle, as it doesn't impact CPP loading, and as Jean implies, it might not be worth tackling that problem - definitely not with 4000 individual commits! Thanks, Paul. -- > only case where this commit would directly lead to the removal of > #include <linux/init.h> from i2c-i801.c is if module_init() was the > only thing from linux/init.h that this driver was using. Which is not > the case, as I see various occurrences of __init and __exit left, and > these are declared in linux/init.h. > > As a matter of fact I still count 3921 driver files which include both > linux/init.h and linux/module.h. And I see no comment in either header > file that including one exempts you from including the other. > > So I'm not taking this change, sorry. If this is really the direction > you want us to take (and I'm not convinced, but my opinion does not > necessarily matter), then it must be documented first, and I believe it > should then be addressed tree-wide. 3921 individual commits do not seem > to be the most efficient to get it done. > > > Cc: Paul Gortmaker <paul.gortmaker@windriver.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Pali Rohár <pali.rohar@gmail.com> > > --- > > drivers/i2c/busses/i2c-i801.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > > index b9c5d7933d12..69c3ccb69669 100644 > > --- a/drivers/i2c/busses/i2c-i801.c > > +++ b/drivers/i2c/busses/i2c-i801.c > > @@ -85,7 +85,6 @@ > > #include <linux/stddef.h> > > #include <linux/delay.h> > > #include <linux/ioport.h> > > -#include <linux/init.h> > > #include <linux/i2c.h> > > #include <linux/i2c-smbus.h> > > #include <linux/acpi.h> > > -- > Jean Delvare > SUSE L3 Support
On Thu, Jun 20, 2019 at 01:32:56PM -0400, Paul Gortmaker wrote: > [Re: [PATCH v2 2/2] i2c: i801: Remove linux/init.h and sort headers] On 20/06/2019 (Thu 15:24) Jean Delvare wrote: > > > Hi Andy, > > > > On Wed, 19 Jun 2019 18:12:48 +0300, Andy Shevchenko wrote: > > > There is no need to include linux/init.h when at the same time > > > we include linux/module.h. > > > > > > Remove redundant inclusion. > > > > > > For more details, refer to the commit > > > 0fd972a7d91d ("module: relocate module_init from init.h to module.h") > > > where the split had been introduced. > > > > I've read it. It's not a split, it's a move. A move which makes sense > > because, as explained in the commit message, module_init() is only > > needed by modular code, so including it in init.h was slowing down the > > pre-processing of non-modular code. > > > > That being said, this alone does not imply that explicit inclusion of > > both linux/init.h and linux/module.h in the same file is wrong. The > > Agreed. If one looks at all the many follow-on changes that relocation > enabled, they will see that related changes are a "downgrade" of > module.h to init.h inclusion - to reduce CPP overhead as stated above. > > To be clear, that kind of change does NOT introduce any implicit header > use, but the reverse: init.h ---> module.h DOES introduce implicit use. > > I've never intentionally removed init.h from a file just because > module.h was already present. > > I don't think implicit header inclusion is a good thing. It leads to > random compile failures that pop up depending on what is in your .config > file and/or what architecture you are building for, etc etc. > > Those issues wont happen from the patch proposed here, since module.h > does explictly draw in init.h -- but that still doesn't make it right. > > Note that solving all cases of implicit use of init.h is a totally > different problem space than what I set out to tackle, as it doesn't > impact CPP loading, and as Jean implies, it might not be worth tackling > that problem - definitely not with 4000 individual commits! Thank you for explanation.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index b9c5d7933d12..69c3ccb69669 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -85,7 +85,6 @@ #include <linux/stddef.h> #include <linux/delay.h> #include <linux/ioport.h> -#include <linux/init.h> #include <linux/i2c.h> #include <linux/i2c-smbus.h> #include <linux/acpi.h>
There is no need to include linux/init.h when at the same time we include linux/module.h. Remove redundant inclusion. For more details, refer to the commit 0fd972a7d91d ("module: relocate module_init from init.h to module.h") where the split had been introduced. Cc: Paul Gortmaker <paul.gortmaker@windriver.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Pali Rohár <pali.rohar@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 1 - 1 file changed, 1 deletion(-)