Re: [PATCH 2/3] powerpc ioremap_prot

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 30 Apr 2008 08:07:53 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> On Tue, 2008-04-29 at 11:17 -0700, Andrew Morton wrote:
> > 
> > Given that x86 implements ioremap_prot() as a regular C function, it
> > would
> > be nicer to require that all architectures do that.  Especially as
> > macros
> > suck.
> > 
> > Your powerpc implementation of ioremap_prot() has a different
> > signature
> > from the x86 one: `phys_addr_t address' versus `resource_size_t
> > phys_addr'.
> > Can that be improved?
> 
> Well, we already had ioremap_flags() which is the same thing, that's why
> I made it just a #define :-)
> 
> But I'm pondering removing our ioremap_flags completely in favor of
> ioremap_prot. This was just a patch to "make it work" so Rik could move
> on with his core patch (btw. Rik, you got the SOBs in the wrong order on
> that one).
> 
> Regarding the difference, well, it has to do with us historically using
> that phys_addr_t type for ioremap. I can try to look into changing that
> but it will take a bit more effort.

It does seem pretty bad to create the same-named function in two
architectures, only with sometimes-different argument types.

A minimal fix would be to make powerpc's implementation be an out-of-line C
function which takes a resource_size_t and which calls ioremap_flags()?

> > >  static inline pte_t pte_mkspecial(pte_t pte) {
> > >       return pte; }
> > > +static inline unsigned long pte_pgprot(pte_t pte) {
> > > +     return __pgprot(pte_val(pte)) & PAGE_PROT_BITS; }
> > 
> > ick.  \n's are cheap.
> 
> Yeah well, just adapted to the style of the other ones around it :-)

I'm not a big believer in making new code match broken old code.

> Those things have been there forever, I think we can even blame
> pre-paulus maintainership here !

I blame Rusty.

> I'll change them all in one go in a different patch if you want.

whatever ;)