linux-kernel - Re: [PATCH v1 1/2] platform/x86: intel-vbtn: Remove redundant inclusions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 05 Feb 2018 18:23:43 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Platform Driver <platform-driver-x86@...r.kernel.org>,
        AceLan Kao <acelan.kao@...onical.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/2] platform/x86: intel-vbtn: Remove redundant
 inclusions

On Wed, 2018-01-31 at 11:04 -0800, Darren Hart wrote:
> On Wed, Jan 31, 2018 at 08:59:20PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 31, 2018 at 8:50 PM, Darren Hart <dvhart@...radead.org>
> > wrote:
> > > On Wed, Jan 31, 2018 at 07:54:06PM +0200, Andy Shevchenko wrote:
> > > > Some headers are not needed since the driver can be built as
> > > > module.
> > > > Remove them.
> > > 
> > > Removing init because it's included by module.h, and removing
> > > acpi_bus.h
> > > because it's included by acpi.h - but not because it can be built
> > > as a
> > > module - right?  Just checking, the wording in the commit msg
> > > seemed odd
> > > to me.
> > 
> > Correct. I'll rephrase this in place.


My gosh, I forgot to do this and can't rebase anymore. Sorry.

> > > These removals seem appropriate to me, but so we have it recorded
> > > here -
> > > in general, including headers that you explicitly make use of is
> > > good
> > > practice, and not depending on others to include them. But in this
> > > case,
> > > the implicit includes are reasonable expectations as they are
> > > tightly
> > > coupled with the parent include.
> > 
> > There are two classes of headers (at least?):
> > - let say "user-visible" ones, which drivers usually include like
> > you
> > pointed above
> > - low level ones, which in most cases are not supposed to be
> > included directly
> > 
> > So, for first class I agree with you, and acpi_bus.h in this case
> > can
> > be considered as an example of second class.
> 
> Agreed, acpi_bus.h is tightly coupled with acpi.h. The practice I've
> seen from others and want to discourage / avoid is including acpi.h
> and
> then deleting ... say...  spinlock.h because somewhere somehow acpi.h
> also pulls it in. They are not tightly coupled conceptually, so
> spinlock.h should remain in the include list if the file uses
> spinlocks
> directly. I think we're in violent agreement here :-)

It's a problem of header organization I think. AFAIK Plan 9 has an idea
that each header is independent, and each C module has to include
headers in appropriate order. (Always trade off between flexibility and
strict hierarchy).

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy