| Message ID | 20230818102322.142582-2-lucas.segarra.fernandez@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add debugfs pm_status for qat driver | expand |
On Fri, Aug 18, 2023 at 12:23:08PM +0200, Lucas Segarra Fernandez wrote: > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > index 34c6cd8e27c0..3bde8759c2a2 100644 > --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > @@ -2,6 +2,9 @@ > /* Copyright(c) 2022 Intel Corporation */ > #include <linux/bitfield.h> > #include <linux/iopoll.h> > +#include <linux/kstrtox.h> > +#include <linux/types.h> Please include linux/kernel.h instead of these two. Thanks,
On Fri, Aug 25, 2023 at 06:36:12PM +0800, Herbert Xu wrote: > On Fri, Aug 18, 2023 at 12:23:08PM +0200, Lucas Segarra Fernandez wrote: > > > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > > index 34c6cd8e27c0..3bde8759c2a2 100644 > > --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > > +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c > > @@ -2,6 +2,9 @@ > > /* Copyright(c) 2022 Intel Corporation */ > > #include <linux/bitfield.h> > > #include <linux/iopoll.h> > > +#include <linux/kstrtox.h> > > +#include <linux/types.h> > > Please include linux/kernel.h instead of these two. No need to resubmit. I will fix this up when I apply these two patches. Cheers,
On Fri, Aug 25, 2023 at 06:41:18PM +0800, Herbert Xu wrote: > > No need to resubmit. I will fix this up when I apply these two > patches. I take that back. This doesn't even build with CONFIG_DEBUG_FS=n. ../drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c: In function ‘adf_gen4_init_dev_pm_data’: ../drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c:403:55: error: ‘adf_gen4_print_pm_status’ undeclared (first use in this function) 403 | accel_dev->power_management.print_pm_status = adf_gen4_print_pm_status; | ^~~~~~~~~~~~~~~~~~~~~~~~ ../drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c:403:55: note: each undeclared identifier is reported only once for each function it appears in make[8]: *** [../scripts/Makefile.build:243: drivers/crypto/intel/qat/qat_common/adf_gen4_pm.o] Error 1 Please resubmit after you fix that and include kernel.h while you're at it. Thanks,
On Fri, Aug 25, 2023 at 06:36:12PM +0800, Herbert Xu wrote: > On Fri, Aug 18, 2023 at 12:23:08PM +0200, Lucas Segarra Fernandez wrote: > > +#include <linux/kstrtox.h> > > +#include <linux/types.h> > > Please include linux/kernel.h instead of these two. Why?
On Fri, Aug 25, 2023 at 03:37:53PM +0300, Andy Shevchenko wrote: > > Why? Because we shouldn't be including every single header file that kernel.h includes individually. Cheers,
On Mon, Aug 28, 2023 at 06:22:00PM +0800, Herbert Xu wrote: > On Fri, Aug 25, 2023 at 03:37:53PM +0300, Andy Shevchenko wrote: > > > > Why? > > Because we shouldn't be including every single header file that > kernel.h includes individually. kernel.h is misleading here. It includes 98% of something which this file is not using or going to use. Can you tell _why_ we need that 98% bulk to be included here?
On Mon, Aug 28, 2023 at 01:46:18PM +0300, Andy Shevchenko wrote: > > kernel.h is misleading here. It includes 98% of something which this file is > not using or going to use. Can you tell _why_ we need that 98% bulk to be > included here? For most drivers in drivers/crypto they will need multiple header files included by kernel.h. I'd hate for people to start posting patches replacing one inclusion of kernel.h with multiple inclusions. They're bound to get it wrong and we'll be forever dealing with random build failures because someone changes a random header elsewhere which then exposes a missed inclusion. Cheers,
On Tue, Aug 29, 2023 at 06:18:24PM +0800, Herbert Xu wrote: > On Mon, Aug 28, 2023 at 01:46:18PM +0300, Andy Shevchenko wrote: > > > > kernel.h is misleading here. It includes 98% of something which this file is > > not using or going to use. Can you tell _why_ we need that 98% bulk to be > > included here? > > For most drivers in drivers/crypto they will need multiple header > files included by kernel.h. I'd hate for people to start posting > patches replacing one inclusion of kernel.h with multiple inclusions. > > They're bound to get it wrong and we'll be forever dealing with > random build failures because someone changes a random header > elsewhere which then exposes a missed inclusion. Do I understand correctly that you want *ideally* to have THE kernel.h as a _single_ header and that's it? While I understand your motivation as a maintainer, I hate the idea of current kernel.h to be included as a silver bullet to every file because people are not capable to understand this C language part of design. The usage of the proper headers show that developer _thought_ very well about what they are doing in the driver. Neglecting this affects the quality of the code in my opinion. That's why I strongly recommend to avoid kernel.h inclusion unless it's indeed the one that provides something that is used in the driver. Even though, the rest headers also need to be included (as it wasn't done by kernel.h at any circumstances).
On Tue, Aug 29, 2023 at 05:08:37PM +0300, Andy Shevchenko wrote: > > Do I understand correctly that you want *ideally* to have THE kernel.h > as a _single_ header and that's it? My rule of thumb for a .c file is that if you need more than two headers directly included by kernel.h then you should just use kernel.h. > While I understand your motivation as a maintainer, I hate the idea of current > kernel.h to be included as a silver bullet to every file because people are not > capable to understand this C language part of design. The usage of the proper > headers show that developer _thought_ very well about what they are doing in > the driver. Neglecting this affects the quality of the code in my opinion. > That's why I strongly recommend to avoid kernel.h inclusion unless it's indeed > the one that provides something that is used in the driver. Even though, the > rest headers also need to be included (as it wasn't done by kernel.h at any > circumstances). I have no qualms with fixing header files that include kernel.h to include whatever it is that they need directly. That is a worthy goal and should be enforced for all new header files. I just don't share your enthusiasm about doing the same for .c files. Cheers,
Hi Herbert, On 2023-08-31 05:55, Herbert Xu wrote: > On Tue, Aug 29, 2023 at 05:08:37PM +0300, Andy Shevchenko wrote: >> >> Do I understand correctly that you want *ideally* to have THE kernel.h >> as a _single_ header and that's it? > > My rule of thumb for a .c file is that if you need more than two > headers directly included by kernel.h then you should just use > kernel.h. > >> While I understand your motivation as a maintainer, I hate the idea of current >> kernel.h to be included as a silver bullet to every file because people are not >> capable to understand this C language part of design. The usage of the proper >> headers show that developer _thought_ very well about what they are doing in >> the driver. Neglecting this affects the quality of the code in my opinion. >> That's why I strongly recommend to avoid kernel.h inclusion unless it's indeed >> the one that provides something that is used in the driver. Even though, the >> rest headers also need to be included (as it wasn't done by kernel.h at any >> circumstances). > > I have no qualms with fixing header files that include kernel.h > to include whatever it is that they need directly. That is a > worthy goal and should be enforced for all new header files. > > I just don't share your enthusiasm about doing the same for .c > files. <https://include-what-you-use.org/ Maybe this is helpful, if you didn't know about it. :) (I disagree with the forward declarations that are recommended there, though.) Cheers, Alex > > Cheers,
On Thu, Aug 31, 2023 at 11:55:52AM +0800, Herbert Xu wrote: > On Tue, Aug 29, 2023 at 05:08:37PM +0300, Andy Shevchenko wrote: > > > > Do I understand correctly that you want *ideally* to have THE kernel.h > > as a _single_ header and that's it? > > My rule of thumb for a .c file is that if you need more than two > headers directly included by kernel.h then you should just use > kernel.h. > > > While I understand your motivation as a maintainer, I hate the idea of current > > kernel.h to be included as a silver bullet to every file because people are not > > capable to understand this C language part of design. The usage of the proper > > headers show that developer _thought_ very well about what they are doing in > > the driver. Neglecting this affects the quality of the code in my opinion. > > That's why I strongly recommend to avoid kernel.h inclusion unless it's indeed > > the one that provides something that is used in the driver. Even though, the > > rest headers also need to be included (as it wasn't done by kernel.h at any > > circumstances). > > I have no qualms with fixing header files that include kernel.h > to include whatever it is that they need directly. That is a > worthy goal and should be enforced for all new header files. > > I just don't share your enthusiasm about doing the same for .c > files. I see, thanks for clarifying this. While you are right about *.c files that it's not so critical for them, the kernel.h use is still a burden everywhere in the kernel (at least in the current form). That's why I prefer to exclude it from *.c-files as well. This will reduce amount of work in the future in case we will be capable to clean up the crap from kernel.h and make it sane.
On Thu, Aug 31, 2023 at 01:18:11PM +0200, Alejandro Colomar wrote: > On 2023-08-31 05:55, Herbert Xu wrote: > > On Tue, Aug 29, 2023 at 05:08:37PM +0300, Andy Shevchenko wrote: > >> > >> Do I understand correctly that you want *ideally* to have THE kernel.h > >> as a _single_ header and that's it? > > > > My rule of thumb for a .c file is that if you need more than two > > headers directly included by kernel.h then you should just use > > kernel.h. > > > >> While I understand your motivation as a maintainer, I hate the idea of current > >> kernel.h to be included as a silver bullet to every file because people are not > >> capable to understand this C language part of design. The usage of the proper > >> headers show that developer _thought_ very well about what they are doing in > >> the driver. Neglecting this affects the quality of the code in my opinion. > >> That's why I strongly recommend to avoid kernel.h inclusion unless it's indeed > >> the one that provides something that is used in the driver. Even though, the > >> rest headers also need to be included (as it wasn't done by kernel.h at any > >> circumstances). > > > > I have no qualms with fixing header files that include kernel.h > > to include whatever it is that they need directly. That is a > > worthy goal and should be enforced for all new header files. > > > > I just don't share your enthusiasm about doing the same for .c > > files. > > <https://include-what-you-use.org/ > > Maybe this is helpful, if you didn't know about it. :) > (I disagree with the forward declarations that are recommended there, > though.) Yeah, but IWYU is too radical and requires a lot of manual job done in the kernel. Jonathan tried it at some point. I prefer to have a balance here (not to include literally _everything_ what we are using, just generic enough).
diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c index 34c6cd8e27c0..3bde8759c2a2 100644 --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.c @@ -2,6 +2,9 @@ /* Copyright(c) 2022 Intel Corporation */ #include <linux/bitfield.h> #include <linux/iopoll.h> +#include <linux/kstrtox.h> +#include <linux/types.h> + #include "adf_accel_devices.h" #include "adf_common_drv.h" #include "adf_gen4_pm.h" diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.h b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.h index c2768762cca3..39d37b352b45 100644 --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.h +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm.h @@ -3,7 +3,9 @@ #ifndef ADF_GEN4_PM_H #define ADF_GEN4_PM_H -#include "adf_accel_devices.h" +#include <linux/bits.h> + +struct adf_accel_dev; /* Power management registers */ #define ADF_GEN4_PM_HOST_MSG (0x50A01C)