linux-kernel - Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 5 Apr 2017 13:38:12 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Bjorn Helgaas <helgaas@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Arnd Bergmann <arnd@...db.de>,
        Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Richard Henderson <rth@...ddle.net>,
        Tony Luck <tony.luck@...el.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Haavard Skinnemoen <hskinnemoen@...il.com>,
        Pratyush Anand <pratyush.anand@...il.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Mingkai Hu <mingkai.hu@...escale.com>,
        John Garry <john.garry@...wei.com>,
        Tanmay Inamdar <tinamdar@....com>,
        Murali Karicheri <m-karicheri2@...com>,
        Bharat Kumar Gogada <bharat.kumar.gogada@...inx.com>,
        Ray Jui <rjui@...adcom.com>,
        Wenrui Li <wenrui.li@...k-chips.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Minghuan Lian <minghuan.Lian@...escale.com>,
        Jon Mason <jonmason@...adcom.com>,
        Gabriele Paoloni <gabriele.paoloni@...wei.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        Stanimir Varbanov <svarbanov@...sol.com>,
        Zhou Wang <wangzhou1@...ilicon.com>,
        Roy Zang <tie-fei.zang@...escale.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>
Subject: Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap
 interface

On Wed, Apr 05, 2017 at 11:58:41AM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 28, 2017 at 03:45:43PM +0100, Lorenzo Pieralisi wrote:
> > On Mon, Mar 27, 2017 at 08:41:10PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Mar 27, 2017 at 10:49:30AM +0100, Lorenzo Pieralisi wrote:
> > > > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > > > index 7afb0e2..50b292f 100644
> > > > --- a/arch/x86/include/asm/io.h
> > > > +++ b/arch/x86/include/asm/io.h
> > > > @@ -171,6 +171,7 @@ static inline unsigned int isa_virt_to_bus(volatile void *address)
> > > >  extern void __iomem *ioremap_nocache(resource_size_t offset, unsigned long size);
> > > >  extern void __iomem *ioremap_uc(resource_size_t offset, unsigned long size);
> > > >  #define ioremap_uc ioremap_uc
> > > > +#define ioremap_nopost ioremap_nocache
> > > 
> > > These are all the same as the default from asm-generic.h.  Do we really
> > > need these definitions in alpha, avr32, frv, ia64, x86?
> > 
> > Those arches do not include asm-generic.h (I suppose for a good reason)
> > but a definition is needed anyway if we want code using ioremap_nopost()
> > to be unconditional. This is one way of doing it, there are others not
> > sure they are any better, I am open to suggestions.
> 
> We do have linux/io.h, which should be included in preference to asm/io.h.
> linux/io.h has existed for years, but I still see (from time to time)
> patches adding drivers that (imho incorrectly) use asm/io.h.
> 
> Also, this:
> 
> > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> > > > index 7ef015e..0e81938 100644
> > > > --- a/include/asm-generic/io.h
> > > > +++ b/include/asm-generic/io.h
> > > > @@ -915,6 +915,10 @@ extern void ioport_unmap(void __iomem *p);
> > > >  #endif /* CONFIG_GENERIC_IOMAP */
> > > >  #endif /* CONFIG_HAS_IOPORT_MAP */
> > > >  
> > > > +#ifndef ioremap_nopost
> > > > +#define ioremap_nopost ioremap_nocache
> > > > +#endif
> > > > +
> > > >  #ifndef xlate_dev_kmem_ptr
> > > >  #define xlate_dev_kmem_ptr xlate_dev_kmem_ptr
> > > >  static inline void *xlate_dev_kmem_ptr(void *addr)
> 
> could well be located in linux/io.h, which would make it available
> everywhere.
> 
> I'd suggest one change to this though:
> 
> #ifndef ioremap_nopost
> static inline void __iomem *ioremap_nopost(resource_size_t offset,
> 					   unsigned long size)
> {
> 	return ioremap_nocache(offset, size);
> }
> #endif
> 
> This way, if someone forgets to define ioremap_nopost() in their
> asm/io.h but provides a definition or extern prototype for
> ioremap_nopost(), we end up with a compile error to highlight the
> error, rather than the error being hidden by the preprocessor.

Yes, I could add ioremap_nopost() to linux/io.h I did not do it because
linux/io.h, for whatever reason, does not seem to contain ioremap_*
prototypes; this does not mean we should not add it there but that
would look odd (with all others ioremap_* in asm/io.h), all I am
saying. This stuff requires some clean-up regardless, for the records.

As for the static inline, I did that and that did not make the
kbuild robot happy at all on some arches:

eg: openrisc

>> include/asm-generic/io.h:922:9: error: implicit declaration of
>> function 'ioremap_nocache' [-Werror=implicit-function-declaration]
     return ioremap_nocache(offset, size);

I will have another stab at it since what you put forward makes sense,
I just have to find a way that works across arches.

Thanks !
Lorenzo