Message-ID: <20170406162156.GA10134@red-moon>
Date:   Thu, 6 Apr 2017 17:21:56 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        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>
Subject: Re: [PATCH v2 02/22] asm-generic/io.h: add ioremap_nopost remap
 interface

On Thu, Apr 06, 2017 at 02:07:27PM +0100, Russell King - ARM Linux wrote:
> On Thu, Apr 06, 2017 at 01:59:07PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Apr 06, 2017 at 12:38:45PM +0100, Lorenzo Pieralisi wrote:
> > > Ok but where ? linux/io.h or asm-generic/io.h ? As I replied to Russell,
> > > having it in linux/io.h is a bit odd given that it would be the only
> > > ioremap_* implementation declared there, I think we need more
> > > consistency here instead of deviating again from what you did.
> > 
> > asm-generic/io.h is the right place for generics which let you override.
> 
> I disagree for two reasons, and I will refer you to Linus' comments back
> in 2005 at http://yarchive.net/comp/linux/generic.html
> 
> 1) asm-generic/io.h has become an ifdef mess, every single definition in
>    there is conditional.  The reason for this has happened is that
>    architectures can't pick and choose what they want from a single file
>    unless something like that is done.  It would be _far_ better to
>    split this file up by functional group - eg, ISA IO accessors,
>    io(read|write)*(), read|write*(), and so forth.
> 
> 2) We're at the point where we have various .c files around the kernel
>    _directly_ including asm-generic header files, which means the
>    use of asm-generic headers is no longer a choice of the architecture.
> 
> 3) asm-generic/ started out life as the place where example
>    implementations of asm/*.h header files were found, and but has
>    grown into a place where we dump default implementations.  We had
>    a place for default implementations for years, which were the
>    linux/*.h headers.  We have now ended up with a mixture of both
>    techniques, even for something like io.h, we have the messy
>    asm-generic/io.h, the architecture's asm/io.h, and then linux/io.h.
>    We have ended up with both linux/io.h and asm-generic/io.h containing
>    default definitions.
> 
> We've had the rule that where both linux/foo.h and asm/foo.h exist, the
> linux/ counterpart is the preferred include file.  That didn't really
> happen with asm/io.h due to the number of users that there were, but
> when new stuff was added to asm/io.h which we wanted to be generic,
> linux/io.h was created to contain that.  This actually pre-dates the
> "let's clutter up asm-generic with shared arch stuff" push.
> 
> Now, what you say _may_ make sense if we have an
> asm-generic/ioremap-nopost.h header which just defines a default
> ioremap_nopost.h implementation, and architectures would then be able to
> choose whether to include that or not depending on whether they provide
> their own implementation.  No ugly ifdefs are necessary with that
> approach.
> 
> Out of the three choices, I would much rather see the
> asm-generic/ioremap-nopost.h approach.  However, the down-side to that
> is it means all architectures asm/io.h would need to be touched to
> explicitly include that.
> 
> What I'm absolutely certain of, though, is that making asm-generic/io.h
> even worse by adding yet more conditional stuff to it is not a sane way
> forward.

Ok, so:

(1) I can do asm-generic/ioremap-nopost.h, which I assume you want to
    contain something like

static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
{
	return ioremap_nocache(offset, size);
}

Funny bit is that it has to be included by asm*/io.h files _after_
ioremap_nocache has been #defined (that's the reason my approach was
failing miserably even on arches like eg powerpc (see [1] below) that
does have ioremap_nocache), not sure it is going to be very nice to have
an include in the middle of asm*/io.h include files (and I have to patch
all arches which I can do).

Or

(2) I add:

#ifndef ioremap_nopost
static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
{
	return NULL; <-(making it return ioremap_nocache() does not
			work, see [1] for the reason)
}
#endif

in linux/io.h

(3) ioremap_nopost follows Luis' ioremap_uc approach

(1)-(2) bypass completely what Luis did for ioremap_uc(), which implies
that we have yet another way of implementing ioremap_*.

I would like to get this patchset done so if you have an opinion it
is time to state it.

Thanks,
Lorenzo

[1] powerpc build report:

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/linux.git pci/config-io-mappings-fix-v3
head:   283a324b549a662346c95c05b08983dd5b83354b
commit: e66b493fe93226c02b1a33355f79db7ed6efe718 [2/23] linux/io.h: add ioremap_nopost remap interface
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout e66b493fe93226c02b1a33355f79db7ed6efe718
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/io.h:28:0,
                    from arch/powerpc/oprofile/op_model_cell.c:29:
   include/linux/io.h: In function 'ioremap_nopost':
   include/linux/io.h:169:9: error: implicit declaration of function 'ioremap_nocache' [-Werror=implicit-function-declaration]
     return ioremap_nocache(offset, size);
            ^~~~~~~~~~~~~~~
>> include/linux/io.h:169:9: error: return makes pointer from integer without a cast [-Werror=int-conversion]
     return ioremap_nocache(offset, size);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +169 include/linux/io.h

   163  }
   164  #endif
   165 
   166  #ifndef ioremap_nopost
   167  static inline void __iomem *ioremap_nopost(phys_addr_t offset, size_t size)
   168  {
 > 169           return ioremap_nocache(offset, size);
   170  }
   171  #endif
   172 

Powered by blists - more mailing lists