Re: [PATCH 00/12] iio:accel: Header Cleanups.

[Rong Chen <rong.a.chen@intel.com>]

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