| Message ID | 20210611171355.202903-1-jic23@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio:accel: Header Cleanups. | expand |
On Fri, Jun 11, 2021 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > 0-day recently started running the include-what-you-use checker with LLVM builds. > After it identified a header we should have dropped in a particular patch, > I decided to experiment with it a little and see if it was useful for tidying > up includes that have gotten rather out of sync with the code over the years. > > Note the tool is something I'd only advocate using to give you hints on what > might want adjusting so each of these was done by hand inspection. > > I've grouped them by manufacturer as otherwise we would have a lot of patches. > Note that the big 'many device / device type' drivers have been done separately > so you won't see them in this series. > > I'm rather hoping this approach may ease getting reviews of these, but we > shall see. If people have time to look at ones I haven't directly cc'd them > on that would be great. There are some drivers touched in here where I don't > know of a current maintainer. Same comment as per staging series. I know that kernel.h provides some crucial everywhere used macros / helpers which are in the TODO list to be split. I would recommend dropping kernel.h from the drivers and see what's missed. These series probably need to be based on splitting out container_of() and ARRAY_SIZE() first. So, no tag from me for now (but I like the idea in general, and thanks for looking into this).
On Fri, 11 Jun 2021 20:48:08 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 11, 2021 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > 0-day recently started running the include-what-you-use checker with LLVM builds. > > After it identified a header we should have dropped in a particular patch, > > I decided to experiment with it a little and see if it was useful for tidying > > up includes that have gotten rather out of sync with the code over the years. > > > > Note the tool is something I'd only advocate using to give you hints on what > > might want adjusting so each of these was done by hand inspection. > > > > I've grouped them by manufacturer as otherwise we would have a lot of patches. > > Note that the big 'many device / device type' drivers have been done separately > > so you won't see them in this series. > > > > I'm rather hoping this approach may ease getting reviews of these, but we > > shall see. If people have time to look at ones I haven't directly cc'd them > > on that would be great. There are some drivers touched in here where I don't > > know of a current maintainer. > > Same comment as per staging series. > > I know that kernel.h provides some crucial everywhere used macros / > helpers which are in the TODO list to be split. > > I would recommend dropping kernel.h from the drivers and see what's missed. > > These series probably need to be based on splitting out container_of() > and ARRAY_SIZE() first. Tool obligingly tells me at least one item from kernel.h and you have the correct pair (occasionally it's the kernel specific string routines). e.g. #include "linux/kernel.h" // for kstrtouint, ARRAY_SIZE > > So, no tag from me for now (but I like the idea in general, and thanks > for looking into this). > The tool is rather a blunderbuss for this job which makes it rather time consuming to use. Splitting kernel.h up is a step beyond where I'm interested in going at this stage because it's sure to involve a lot of bike shedding. I 'could' just drop the kernel.h where added on the basis we were clearly getting it indirectly. I've not included a whole bunch of other suggestions on that basis. Note I didn't include a whole bunch of other headers on the basis they were a bit more esoteric. To give an idea of how noisy this is here's the output another file... drivers/iio/trigger/stm32-timer-trigger.c should add these lines: #include <stddef.h> // for NULL, size_t #include <stdint.h> // for uintptr_t #include <sys/types.h> // for ssize_t #include "asm-generic/div64.h" // for do_div #include "asm-generic/errno-base.h" // for EINVAL, ENOMEM, EBUSY #include "asm-generic/int-ll64.h" // for u32 #include "asm/page_types.h" // for PAGE_SIZE #include "linux/clk.h" // for clk_enable, clk_dis... #include "linux/compiler_attributes.h" // for __maybe_unused #include "linux/dev_printk.h" // for dev_err #include "linux/device.h" // for dev_get_drvdata #include "linux/device/driver.h" // for device_driver #include "linux/export.h" // for EXPORT_SYMBOL #include "linux/gfp.h" // for GFP_KERNEL #include "linux/iio/types.h" // for IIO_CHAN_INFO_ENABLE #include "linux/kernel.h" // for kstrtouint, ARRAY_SIZE #include "linux/list.h" // for INIT_LIST_HEAD, lis... #include "linux/mod_devicetable.h" // for of_device_id #include "linux/mutex.h" // for mutex_lock, mutex_u... #include "linux/of.h" // for of_property_read_u32 #include "linux/pm.h" // for SIMPLE_DEV_PM_OPS #include "linux/regmap.h" // for regmap_update_bits #include "linux/stddef.h" // for true, false #include "linux/string.h" // for strstr, strlen, str... #include "linux/sysfs.h" // for sysfs_emit, attribu... #include "linux/types.h" // for bool, list_head #include "vdso/bits.h" // for BIT drivers/iio/trigger/stm32-timer-trigger.c should remove these lines: The full include-list for drivers/iio/trigger/stm32-timer-trigger.c: #include <linux/iio/iio.h> // for iio_priv, IIO_SEPARATE #include <linux/iio/sysfs.h> // for iio_dev_attr, IIO_D... #include <linux/iio/timer/stm32-timer-trigger.h> // for TIM4_TRGO, TIM1_TRGO #include <linux/iio/trigger.h> // for iio_trigger, to_iio... #include <linux/mfd/stm32-timers.h> // for TIM_CR1, TIM_CR2 #include <linux/module.h> // for MODULE_ALIAS, MODUL... #include <linux/of_device.h> // for of_match_device #include <linux/platform_device.h> // for platform_get_drvdata #include <stddef.h> // for NULL, size_t #include <stdint.h> // for uintptr_t #include <sys/types.h> // for ssize_t #include "asm-generic/div64.h" // for do_div #include "asm-generic/errno-base.h" // for EINVAL, ENOMEM, EBUSY #include "asm-generic/int-ll64.h" // for u32 #include "asm/page_types.h" // for PAGE_SIZE #include "linux/clk.h" // for clk_enable, clk_dis... #include "linux/compiler_attributes.h" // for __maybe_unused #include "linux/dev_printk.h" // for dev_err #include "linux/device.h" // for dev_get_drvdata #include "linux/device/driver.h" // for device_driver #include "linux/export.h" // for EXPORT_SYMBOL #include "linux/gfp.h" // for GFP_KERNEL #include "linux/iio/types.h" // for IIO_CHAN_INFO_ENABLE #include "linux/kernel.h" // for kstrtouint, ARRAY_SIZE #include "linux/list.h" // for INIT_LIST_HEAD, lis... #include "linux/mod_devicetable.h" // for of_device_id #include "linux/mutex.h" // for mutex_lock, mutex_u... #include "linux/of.h" // for of_property_read_u32 #include "linux/pm.h" // for SIMPLE_DEV_PM_OPS #include "linux/regmap.h" // for regmap_update_bits #include "linux/stddef.h" // for true, false #include "linux/string.h" // for strstr, strlen, str... #include "linux/sysfs.h" // for sysfs_emit, attribu... #include "linux/types.h" // for bool, list_head #include "vdso/bits.h" // for BIT
On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 11 Jun 2021 20:48:08 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Jun 11, 2021 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > 0-day recently started running the include-what-you-use checker with LLVM builds. > > > After it identified a header we should have dropped in a particular patch, > > > I decided to experiment with it a little and see if it was useful for tidying > > > up includes that have gotten rather out of sync with the code over the years. > > > > > > Note the tool is something I'd only advocate using to give you hints on what > > > might want adjusting so each of these was done by hand inspection. > > > > > > I've grouped them by manufacturer as otherwise we would have a lot of patches. > > > Note that the big 'many device / device type' drivers have been done separately > > > so you won't see them in this series. > > > > > > I'm rather hoping this approach may ease getting reviews of these, but we > > > shall see. If people have time to look at ones I haven't directly cc'd them > > > on that would be great. There are some drivers touched in here where I don't > > > know of a current maintainer. > > > > Same comment as per staging series. > > > > I know that kernel.h provides some crucial everywhere used macros / > > helpers which are in the TODO list to be split. > > > > I would recommend dropping kernel.h from the drivers and see what's missed. > > > > These series probably need to be based on splitting out container_of() > > and ARRAY_SIZE() first. > > Tool obligingly tells me at least one item from kernel.h and you have > the correct pair (occasionally it's the kernel specific string routines). > e.g. > #include "linux/kernel.h" // for kstrtouint, ARRAY_SIZE Oh yes, kstrtox.h should be born (I have even somewhere in my tree the preliminary split). > > So, no tag from me for now (but I like the idea in general, and thanks > > for looking into this). > > > > The tool is rather a blunderbuss for this job which makes it rather time > consuming to use. > > Splitting kernel.h up is a step beyond where I'm interested in going > at this stage because it's sure to involve a lot of bike shedding. Not really. There is a kind of consensus about the mentioned container_of() and ARRAY_SIZE() and similar to be split out from kernel.h. But I understand you and not insisting that you have to go with it. My point is that... > I 'could' just drop the kernel.h where added on the basis we were clearly > getting it indirectly. I've not included a whole bunch of other suggestions > on that basis. (Which is probably not a good idea, because explicit in this case is better than implicit, i.e. kernel.h is not guaranteed to be included by other headers and I have long standing work to actually make sure that most of the headers won't require kernel.h!) > Note I didn't include a whole bunch of other headers on the basis > they were a bit more esoteric. > > To give an idea of how noisy this is here's the output another file... ...the tool simply doesn't know anything about kernel and header guarantees. That's why it tries play dumb. If you would like to continue with this, please drop the removal of the headers that are not guaranteed to be included by others (excluding kernel.h from the equation). Otherwise it will become someone else's problem to _reinstantiate_ all those headers, and since I already had a headache with panic.h, I won't repeat it. Still no tag from me, although no explicit NAK (actually opposite, no explicit ACK because of the dependencies), you just really need to spend more time on this.
On Fri, 11 Jun 2021 21:35:06 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 11 Jun 2021 20:48:08 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Fri, Jun 11, 2021 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > 0-day recently started running the include-what-you-use checker with LLVM builds. > > > > After it identified a header we should have dropped in a particular patch, > > > > I decided to experiment with it a little and see if it was useful for tidying > > > > up includes that have gotten rather out of sync with the code over the years. > > > > > > > > Note the tool is something I'd only advocate using to give you hints on what > > > > might want adjusting so each of these was done by hand inspection. > > > > > > > > I've grouped them by manufacturer as otherwise we would have a lot of patches. > > > > Note that the big 'many device / device type' drivers have been done separately > > > > so you won't see them in this series. > > > > > > > > I'm rather hoping this approach may ease getting reviews of these, but we > > > > shall see. If people have time to look at ones I haven't directly cc'd them > > > > on that would be great. There are some drivers touched in here where I don't > > > > know of a current maintainer. > > > > > > Same comment as per staging series. > > > > > > I know that kernel.h provides some crucial everywhere used macros / > > > helpers which are in the TODO list to be split. > > > > > > I would recommend dropping kernel.h from the drivers and see what's missed. > > > > > > These series probably need to be based on splitting out container_of() > > > and ARRAY_SIZE() first. > > > > Tool obligingly tells me at least one item from kernel.h and you have > > the correct pair (occasionally it's the kernel specific string routines). > > e.g. > > #include "linux/kernel.h" // for kstrtouint, ARRAY_SIZE > > Oh yes, kstrtox.h should be born (I have even somewhere in my tree the > preliminary split). That set definitely make sense to break out. > > > > So, no tag from me for now (but I like the idea in general, and thanks > > > for looking into this). > > > > > > > The tool is rather a blunderbuss for this job which makes it rather time > > consuming to use. > > > > Splitting kernel.h up is a step beyond where I'm interested in going > > at this stage because it's sure to involve a lot of bike shedding. > > Not really. There is a kind of consensus about the mentioned > container_of() and ARRAY_SIZE() and similar to be split out from > kernel.h. Ah fair enough. I'd not caught the earlier discussion of this. > > But I understand you and not insisting that you have to go with it. My > point is that... > > > I 'could' just drop the kernel.h where added on the basis we were clearly > > getting it indirectly. I've not included a whole bunch of other suggestions > > on that basis. > > (Which is probably not a good idea, because explicit in this case is > better than implicit, i.e. kernel.h is not guaranteed to be included > by other headers and I have long standing work to actually make sure > that most of the headers won't require kernel.h!) I think I was unclear, what I was proposing was not to touch includes of kernel.h at all. So not make anything worse, but also not make it any better. Where added in this set, kernel.h was for things directly in kernel.h, not the files it includes. > > > Note I didn't include a whole bunch of other headers on the basis > > they were a bit more esoteric. > > > > To give an idea of how noisy this is here's the output another file... > > ...the tool simply doesn't know anything about kernel and header > guarantees. That's why it tries play dumb. > > If you would like to continue with this, please drop the removal of > the headers that are not guaranteed to be included by others > (excluding kernel.h from the equation). This is where the confusion lies... I haven't done that (subject to bugs of course) > Otherwise it will become > someone else's problem to _reinstantiate_ all those headers, and since > I already had a headache with panic.h, I won't repeat it. Still no tag > from me, although no explicit NAK (actually opposite, no explicit ACK > because of the dependencies), you just really need to spend more time > on this. I've not removed any headers on the basis they were guaranteed to be included by others. The tool assumes the opposite model - everything should be explicitly included directly in the file where it's used. For some files it lists 50+ headers. These patches are very conservative on that front. What I haven't done is included everything under the sun that wasn't already included. e.g. I've not included linux/device/driver.h on the basis it is definitely included by linux/device.h and that seems very unlikely to change. There are some corner cases that are more interesting - such as whether we can rely on interrupt.h always including irqreturn.h. Plenty of IIO drivers don't call anything in interrupt.h because of various wrappers but do use the return values. So in this case we could switch many of them to the more specific header, but I haven't done that yet. So, in short, the headers for which includes are removed in this series are not used at all in the files in question (unless I messed up of course!). Jonathan >
On Fri, Jun 11, 2021 at 10:17 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 11 Jun 2021 21:35:06 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > On Fri, 11 Jun 2021 20:48:08 +0300 > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > But I understand you and not insisting that you have to go with it. My > > point is that... > > > > > I 'could' just drop the kernel.h where added on the basis we were clearly > > > getting it indirectly. I've not included a whole bunch of other suggestions > > > on that basis. > > > > (Which is probably not a good idea, because explicit in this case is > > better than implicit, i.e. kernel.h is not guaranteed to be included > > by other headers and I have long standing work to actually make sure > > that most of the headers won't require kernel.h!) > > I think I was unclear, what I was proposing was not to touch includes of > kernel.h at all. So not make anything worse, but also not make it any better. > Where added in this set, kernel.h was for things directly in kernel.h, > not the files it includes. Ah, that's good! That was my main point of worry. > > > Note I didn't include a whole bunch of other headers on the basis > > > they were a bit more esoteric. > > > > > > To give an idea of how noisy this is here's the output another file... > > > > ...the tool simply doesn't know anything about kernel and header > > guarantees. That's why it tries play dumb. > > > > If you would like to continue with this, please drop the removal of > > the headers that are not guaranteed to be included by others > > (excluding kernel.h from the equation). > > This is where the confusion lies... I haven't done that (subject to bugs > of course) > > > Otherwise it will become > > someone else's problem to _reinstantiate_ all those headers, and since > > I already had a headache with panic.h, I won't repeat it. Still no tag > > from me, although no explicit NAK (actually opposite, no explicit ACK > > because of the dependencies), you just really need to spend more time > > on this. > > I've not removed any headers on the basis they were guaranteed to be > included by others. The tool assumes the opposite model - everything > should be explicitly included directly in the file where it's used. > For some files it lists 50+ headers. > These patches are very conservative on that front. > > What I haven't done is included everything under the sun that wasn't > already included. e.g. I've not included > linux/device/driver.h on the basis it is definitely included by > linux/device.h and that seems very unlikely to change. > > There are some corner cases that are more interesting - such as whether > we can rely on interrupt.h always including irqreturn.h. Plenty of IIO > drivers don't call anything in interrupt.h because of various wrappers but > do use the return values. So in this case we could switch many of them > to the more specific header, but I haven't done that yet. I guess you may consider a guarantee there. Actually what kernel header mess misses right now is the list of those guarantees. But you see there cases like using dev_err() and struct device * in the same C file, what do you include? Temptation is to go with device.h, but I would go with #include <linux/dev_printk.h> struct device; Not sure if that tool can handle this kind of use. > So, in short, the headers for which includes are removed in this series are > not used at all in the files in question (unless I messed up of course!). Cool! So, let bots and other people have more time on eyeballing this. Will see how it goes.
On Fri, 11 Jun 2021 22:36:52 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 11, 2021 at 10:17 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 11 Jun 2021 21:35:06 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 11 Jun 2021 20:48:08 +0300 > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > ... > > > > But I understand you and not insisting that you have to go with it. My > > > point is that... > > > > > > > I 'could' just drop the kernel.h where added on the basis we were clearly > > > > getting it indirectly. I've not included a whole bunch of other suggestions > > > > on that basis. > > > > > > (Which is probably not a good idea, because explicit in this case is > > > better than implicit, i.e. kernel.h is not guaranteed to be included > > > by other headers and I have long standing work to actually make sure > > > that most of the headers won't require kernel.h!) > > > > I think I was unclear, what I was proposing was not to touch includes of > > kernel.h at all. So not make anything worse, but also not make it any better. > > Where added in this set, kernel.h was for things directly in kernel.h, > > not the files it includes. > > Ah, that's good! That was my main point of worry. > > > > > Note I didn't include a whole bunch of other headers on the basis > > > > they were a bit more esoteric. > > > > > > > > To give an idea of how noisy this is here's the output another file... > > > > > > ...the tool simply doesn't know anything about kernel and header > > > guarantees. That's why it tries play dumb. > > > > > > If you would like to continue with this, please drop the removal of > > > the headers that are not guaranteed to be included by others > > > (excluding kernel.h from the equation). > > > > This is where the confusion lies... I haven't done that (subject to bugs > > of course) > > > > > Otherwise it will become > > > someone else's problem to _reinstantiate_ all those headers, and since > > > I already had a headache with panic.h, I won't repeat it. Still no tag > > > from me, although no explicit NAK (actually opposite, no explicit ACK > > > because of the dependencies), you just really need to spend more time > > > on this. > > > > I've not removed any headers on the basis they were guaranteed to be > > included by others. The tool assumes the opposite model - everything > > should be explicitly included directly in the file where it's used. > > For some files it lists 50+ headers. > > These patches are very conservative on that front. > > > > What I haven't done is included everything under the sun that wasn't > > already included. e.g. I've not included > > linux/device/driver.h on the basis it is definitely included by > > linux/device.h and that seems very unlikely to change. > > > > There are some corner cases that are more interesting - such as whether > > we can rely on interrupt.h always including irqreturn.h. Plenty of IIO > > drivers don't call anything in interrupt.h because of various wrappers but > > do use the return values. So in this case we could switch many of them > > to the more specific header, but I haven't done that yet. > > I guess you may consider a guarantee there. > > Actually what kernel header mess misses right now is the list of those > guarantees. > > But you see there cases like > > using dev_err() and struct device * in the same C file, what do you include? > > Temptation is to go with device.h, but I would go with > > #include <linux/dev_printk.h> > > struct device; > > Not sure if that tool can handle this kind of use. It does. I decided it was a step too far and didn't act on this one for da311.c, but here is an example of that case. drivers/iio/accel/da311.c should add these lines: #include "asm-generic/errno-base.h" // for EINVAL, ENODEV, ENOMEM #include "asm-generic/int-ll64.h" // for u8, u16 #include "generated/autoconf.h" // for CONFIG_PM_SLEEP #include "linux/dev_printk.h" // for dev_err #include "linux/iio/types.h" // for IIO_CHAN_INFO_RAW, IIO_CHAN_INFO... #include "linux/kernel.h" // for ARRAY_SIZE #include "linux/pm.h" // for SIMPLE_DEV_PM_OPS #include "linux/stddef.h" // for false, true #include "linux/types.h" // for bool #include "vdso/bits.h" // for BIT struct device; If we can teach the tool (or post process it a bit) about places we want to do something beyond the direct (e.g. asm-generic/ernrno-base.h) that would make it much more useful. Even then I think we'll want a human in the loop. I'm running an older version than 0-day has been using I think. +Cc 0-day person who kind of kicked this off :) Note the warning from 0-day was about a header no longer needing to be included (the mutex in the ADIS drivers got recently moved to a wrapper in the library). Those warnings are almost certain to be correct - and many of the removals in here are things that would have been picked up in similar circumstances. Note things can get a bit silly, even if technically correct... + it gets confused on where headers are coming from sometimes. The full include-list for drivers/iio/industrialio-core.c: #include <asm/bug.h> // for WARN #include <linux/cdev.h> // for cdev_init, cdev_... #include <linux/debugfs.h> // for debugfs_create_dir #include <linux/device.h> // for device, device_a... #include <linux/err.h> // for IS_ERR, PTR_ERR #include <linux/fs.h> // for noop_llseek, inode #include <linux/idr.h> // for DEFINE_IDA, ida_... #include <linux/iio/iio-opaque.h> // for iio_dev_opaque #include <linux/iio/iio.h> // for iio_dev, iio_cha... #include <linux/iio/sysfs.h> // for iio_dev_attr #include <linux/kdev_t.h> // for MAJOR, MKDEV #include <linux/kernel.h> // for kasprintf, sscanf #include <linux/ktime.h> // for ktime_get_bootti... #include <linux/module.h> // for MODULE_AUTHOR #include <linux/mutex.h> // for mutex_unlock #include <linux/property.h> // for device_property_... #include <linux/slab.h> // for kfree, kzalloc #include <stddef.h> // for NULL, size_t #include <stdint.h> // for uintptr_t #include <sys/types.h> // for ssize_t, dev_t #include "asm-generic/bitops/instrumented-atomic.h" // for clear_bit, test_... #include "asm-generic/errno-base.h" // for EINVAL, EBUSY #include "asm-generic/int-ll64.h" // for s64, u64 #include "asm/bug.h" // for BUG #include "asm/page_types.h" // for PAGE_SIZE #include "asm/string_64.h" // for memcpy #include "generated/autoconf.h" // for CONFIG_DEBUG_FS #include "iio_core.h" // for iio_ioctl_handler #include "iio_core_trigger.h" // for iio_device_regis... #include "linux/align.h" // for ALIGN #include "linux/bitops.h" // for for_each_set_bit #include "linux/compiler_attributes.h" // for fallthrough #include "linux/compiler_types.h" // for __user #include "linux/dev_printk.h" // for dev_err, dev_info #include "linux/device/bus.h" // for bus_unregister #include "linux/export.h" // for EXPORT_SYMBOL_GPL #include "linux/gfp.h" // for GFP_KERNEL #include "linux/hrtimer.h" // for hrtimer_resolution #include "linux/iio/types.h" // for IIO_VAL_INT, IIO... #include "linux/init.h" // for __exit, __init #include "linux/ktime.h" // for ktime_to_ns #include "linux/list.h" // for INIT_LIST_HEAD #include "linux/math.h" // for abs #include "linux/math64.h" // for div_s64_rem, div... #include "linux/minmax.h" // for min_t #include "linux/of.h" // for of_get_property #include "linux/printk.h" // for pr_err #include "linux/stat.h" // for S_IRUGO, S_IWUSR #include "linux/stddef.h" // for false, true #include "linux/string.h" // for sysfs_streq, str... #include "linux/sysfs.h" // for sysfs_emit_at #include "linux/time.h" // for CLOCK_BOOTTIME #include "linux/time64.h" // for timespec64_to_ns #include "linux/timex.h" // for shift_right #include "linux/types.h" // for list_head, bool #include "linux/uaccess.h" // for copy_from_user #include "vdso/ktime.h" // for LOW_RES_NSEC struct iio_buffer; > > > So, in short, the headers for which includes are removed in this series are > > not used at all in the files in question (unless I messed up of course!). > > Cool! So, let bots and other people have more time on eyeballing this. > Will see how it goes. Sure. Jonathan >
On 6/12/21 4:10 AM, Jonathan Cameron wrote: > On Fri, 11 Jun 2021 22:36:52 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Fri, Jun 11, 2021 at 10:17 PM Jonathan Cameron <jic23@kernel.org> wrote: >>> On Fri, 11 Jun 2021 21:35:06 +0300 >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>> On Fri, Jun 11, 2021 at 9:21 PM Jonathan Cameron <jic23@kernel.org> wrote: >>>>> On Fri, 11 Jun 2021 20:48:08 +0300 >>>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> ... >> >>>> But I understand you and not insisting that you have to go with it. My >>>> point is that... >>>> >>>>> I 'could' just drop the kernel.h where added on the basis we were clearly >>>>> getting it indirectly. I've not included a whole bunch of other suggestions >>>>> on that basis. >>>> (Which is probably not a good idea, because explicit in this case is >>>> better than implicit, i.e. kernel.h is not guaranteed to be included >>>> by other headers and I have long standing work to actually make sure >>>> that most of the headers won't require kernel.h!) >>> I think I was unclear, what I was proposing was not to touch includes of >>> kernel.h at all. So not make anything worse, but also not make it any better. >>> Where added in this set, kernel.h was for things directly in kernel.h, >>> not the files it includes. >> Ah, that's good! That was my main point of worry. >> >>>>> Note I didn't include a whole bunch of other headers on the basis >>>>> they were a bit more esoteric. >>>>> >>>>> To give an idea of how noisy this is here's the output another file... >>>> ...the tool simply doesn't know anything about kernel and header >>>> guarantees. That's why it tries play dumb. >>>> >>>> If you would like to continue with this, please drop the removal of >>>> the headers that are not guaranteed to be included by others >>>> (excluding kernel.h from the equation). >>> This is where the confusion lies... I haven't done that (subject to bugs >>> of course) >>> >>>> Otherwise it will become >>>> someone else's problem to _reinstantiate_ all those headers, and since >>>> I already had a headache with panic.h, I won't repeat it. Still no tag >>>> from me, although no explicit NAK (actually opposite, no explicit ACK >>>> because of the dependencies), you just really need to spend more time >>>> on this. >>> I've not removed any headers on the basis they were guaranteed to be >>> included by others. The tool assumes the opposite model - everything >>> should be explicitly included directly in the file where it's used. >>> For some files it lists 50+ headers. >>> These patches are very conservative on that front. >>> >>> What I haven't done is included everything under the sun that wasn't >>> already included. e.g. I've not included >>> linux/device/driver.h on the basis it is definitely included by >>> linux/device.h and that seems very unlikely to change. >>> >>> There are some corner cases that are more interesting - such as whether >>> we can rely on interrupt.h always including irqreturn.h. Plenty of IIO >>> drivers don't call anything in interrupt.h because of various wrappers but >>> do use the return values. So in this case we could switch many of them >>> to the more specific header, but I haven't done that yet. >> I guess you may consider a guarantee there. >> >> Actually what kernel header mess misses right now is the list of those >> guarantees. >> >> But you see there cases like >> >> using dev_err() and struct device * in the same C file, what do you include? >> >> Temptation is to go with device.h, but I would go with >> >> #include <linux/dev_printk.h> >> >> struct device; >> >> Not sure if that tool can handle this kind of use. > It does. I decided it was a step too far and didn't act on this one > for da311.c, but here is an example of that case. > > drivers/iio/accel/da311.c should add these lines: > #include "asm-generic/errno-base.h" // for EINVAL, ENODEV, ENOMEM > #include "asm-generic/int-ll64.h" // for u8, u16 > #include "generated/autoconf.h" // for CONFIG_PM_SLEEP > #include "linux/dev_printk.h" // for dev_err > #include "linux/iio/types.h" // for IIO_CHAN_INFO_RAW, IIO_CHAN_INFO... > #include "linux/kernel.h" // for ARRAY_SIZE > #include "linux/pm.h" // for SIMPLE_DEV_PM_OPS > #include "linux/stddef.h" // for false, true > #include "linux/types.h" // for bool > #include "vdso/bits.h" // for BIT > struct device; > > If we can teach the tool (or post process it a bit) about places we > want to do something beyond the direct (e.g. asm-generic/ernrno-base.h) > that would make it much more useful. Even then I think we'll want > a human in the loop. Hi, The tool allows to correct the recommended headers, please see https://github.com/include-what-you-use/include-what-you-use#how-to-correct-iwyu-mistakes > > I'm running an older version than 0-day has been using I think. > > +Cc 0-day person who kind of kicked this off :) > > Note the warning from 0-day was about a header no longer needing to be included > (the mutex in the ADIS drivers got recently moved to a wrapper in the library). > Those warnings are almost certain to be correct - and many of the removals > in here are things that would have been picked up in similar circumstances. Yes, 0day only detects useless headers experimentally, we're willing to extend the range if include-what-you-use tool (or other tool) can output a more friendly header list. Best Regards, Rong Chen > > Note things can get a bit silly, even if technically correct... > + it gets confused on where headers are coming from sometimes. > > The full include-list for drivers/iio/industrialio-core.c: > #include <asm/bug.h> // for WARN > #include <linux/cdev.h> // for cdev_init, cdev_... > #include <linux/debugfs.h> // for debugfs_create_dir > #include <linux/device.h> // for device, device_a... > #include <linux/err.h> // for IS_ERR, PTR_ERR > #include <linux/fs.h> // for noop_llseek, inode > #include <linux/idr.h> // for DEFINE_IDA, ida_... > #include <linux/iio/iio-opaque.h> // for iio_dev_opaque > #include <linux/iio/iio.h> // for iio_dev, iio_cha... > #include <linux/iio/sysfs.h> // for iio_dev_attr > #include <linux/kdev_t.h> // for MAJOR, MKDEV > #include <linux/kernel.h> // for kasprintf, sscanf > #include <linux/ktime.h> // for ktime_get_bootti... > #include <linux/module.h> // for MODULE_AUTHOR > #include <linux/mutex.h> // for mutex_unlock > #include <linux/property.h> // for device_property_... > #include <linux/slab.h> // for kfree, kzalloc > #include <stddef.h> // for NULL, size_t > #include <stdint.h> // for uintptr_t > #include <sys/types.h> // for ssize_t, dev_t > #include "asm-generic/bitops/instrumented-atomic.h" // for clear_bit, test_... > #include "asm-generic/errno-base.h" // for EINVAL, EBUSY > #include "asm-generic/int-ll64.h" // for s64, u64 > #include "asm/bug.h" // for BUG > #include "asm/page_types.h" // for PAGE_SIZE > #include "asm/string_64.h" // for memcpy > #include "generated/autoconf.h" // for CONFIG_DEBUG_FS > #include "iio_core.h" // for iio_ioctl_handler > #include "iio_core_trigger.h" // for iio_device_regis... > #include "linux/align.h" // for ALIGN > #include "linux/bitops.h" // for for_each_set_bit > #include "linux/compiler_attributes.h" // for fallthrough > #include "linux/compiler_types.h" // for __user > #include "linux/dev_printk.h" // for dev_err, dev_info > #include "linux/device/bus.h" // for bus_unregister > #include "linux/export.h" // for EXPORT_SYMBOL_GPL > #include "linux/gfp.h" // for GFP_KERNEL > #include "linux/hrtimer.h" // for hrtimer_resolution > #include "linux/iio/types.h" // for IIO_VAL_INT, IIO... > #include "linux/init.h" // for __exit, __init > #include "linux/ktime.h" // for ktime_to_ns > #include "linux/list.h" // for INIT_LIST_HEAD > #include "linux/math.h" // for abs > #include "linux/math64.h" // for div_s64_rem, div... > #include "linux/minmax.h" // for min_t > #include "linux/of.h" // for of_get_property > #include "linux/printk.h" // for pr_err > #include "linux/stat.h" // for S_IRUGO, S_IWUSR > #include "linux/stddef.h" // for false, true > #include "linux/string.h" // for sysfs_streq, str... > #include "linux/sysfs.h" // for sysfs_emit_at > #include "linux/time.h" // for CLOCK_BOOTTIME > #include "linux/time64.h" // for timespec64_to_ns > #include "linux/timex.h" // for shift_right > #include "linux/types.h" // for list_head, bool > #include "linux/uaccess.h" // for copy_from_user > #include "vdso/ktime.h" // for LOW_RES_NSEC > struct iio_buffer; >>> So, in short, the headers for which includes are removed in this series are >>> not used at all in the files in question (unless I messed up of course!). >> Cool! So, let bots and other people have more time on eyeballing this. >> Will see how it goes. > Sure. > > Jonathan
On 6/11/21 7:13 PM, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > 0-day recently started running the include-what-you-use checker with LLVM builds. > After it identified a header we should have dropped in a particular patch, > I decided to experiment with it a little and see if it was useful for tidying > up includes that have gotten rather out of sync with the code over the years. Oh, that's nice! I've always been looking for something like that.
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> 0-day recently started running the include-what-you-use checker with LLVM builds. After it identified a header we should have dropped in a particular patch, I decided to experiment with it a little and see if it was useful for tidying up includes that have gotten rather out of sync with the code over the years. Note the tool is something I'd only advocate using to give you hints on what might want adjusting so each of these was done by hand inspection. I've grouped them by manufacturer as otherwise we would have a lot of patches. Note that the big 'many device / device type' drivers have been done separately so you won't see them in this series. I'm rather hoping this approach may ease getting reviews of these, but we shall see. If people have time to look at ones I haven't directly cc'd them on that would be great. There are some drivers touched in here where I don't know of a current maintainer. Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Aleksei Mamlin <mamlinav@gmail.com> Cc: Dan Robertson <dan@dlrobertson.com> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Harinath Nampally <harinath922@gmail.com> Cc: Jelle van der Waa <jelle@vdwaa.nl> Cc: Jonathan Bakker <xc-racer2@live.ca> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Michael Hennerich <Michael.Hennerich@analog.com> Cc: Mike Looijmans <mike.looijmans@topic.nl> Cc: Sean Nyekjaer <sean@geanix.com> Cc: Tomas Melin <tomas.melin@vaisala.com> Jonathan Cameron (12): iio:accel:adxl372: Cleanup includes iio:accel:bma180: Use generic device properties. iio:accel:bosch drivers: Cleanup includes iio:accel:miramems drivers: Cleanup headers iio:accel:domintech: Cleanup includes. iio:accel:fxls8962af: Add a few missing includes. iio:accel:kionix drivers: Cleanup includes iio:accel:mc3220: Cleanup includes. iio:accel:freescale drivers: Cleanup includes iio:accel:memsic drivers: Cleanup includes iio:accel:murata/vti drivers: Include cleanups for the sca**** parts. iio:accel:sensortek drivers: Add some includes drivers/iio/accel/adxl372.c | 4 ++-- drivers/iio/accel/adxl372.h | 2 ++ drivers/iio/accel/adxl372_spi.c | 3 +-- drivers/iio/accel/bma180.c | 11 +++++------ drivers/iio/accel/bma220_spi.c | 2 +- drivers/iio/accel/bma400_core.c | 1 - drivers/iio/accel/bma400_spi.c | 1 - drivers/iio/accel/bmc150-accel-core.c | 1 + drivers/iio/accel/bmc150-accel-spi.c | 1 - drivers/iio/accel/bmi088-accel-core.c | 3 --- drivers/iio/accel/bmi088-accel-spi.c | 3 ++- drivers/iio/accel/da280.c | 3 +-- drivers/iio/accel/da311.c | 4 ++-- drivers/iio/accel/dmard06.c | 4 +++- drivers/iio/accel/dmard09.c | 2 ++ drivers/iio/accel/dmard10.c | 2 +- drivers/iio/accel/fxls8962af-core.c | 3 ++- drivers/iio/accel/kxcjk-1013.c | 4 +++- drivers/iio/accel/kxsd9-i2c.c | 3 --- drivers/iio/accel/kxsd9-spi.c | 5 +---- drivers/iio/accel/kxsd9.c | 1 - drivers/iio/accel/kxsd9.h | 2 ++ drivers/iio/accel/mc3230.c | 2 +- drivers/iio/accel/mma7455_core.c | 3 ++- drivers/iio/accel/mma7455_i2c.c | 1 + drivers/iio/accel/mma7455_spi.c | 1 + drivers/iio/accel/mma7660.c | 3 +++ drivers/iio/accel/mma8452.c | 5 +++++ drivers/iio/accel/mma9551.c | 7 ++++--- drivers/iio/accel/mma9551_core.h | 2 ++ drivers/iio/accel/mma9553.c | 6 ++++-- drivers/iio/accel/mxc4005.c | 4 ++++ drivers/iio/accel/mxc6255.c | 5 ++--- drivers/iio/accel/sca3000.c | 5 ++--- drivers/iio/accel/sca3300.c | 3 ++- drivers/iio/accel/stk8312.c | 3 +++ drivers/iio/accel/stk8ba50.c | 2 ++ 37 files changed, 69 insertions(+), 48 deletions(-)