Re: PAGE_KERNEL_RO

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2011-01-13 at 08:27 +0100, Clemens Ladisch wrote:
> Marc Gauthier wrote:
> > Stefan Richter wrote:
> >> On Jan 13 Benjamin Herrenschmidt wrote at linux-arch:
> >>> On Wed, 2011-01-12 at 16:07 -0800, Andrew Morton wrote:
> >>>> drivers/firewire/ohci.c now needs PAGE_KERNEL_RO, but many
> >>>> architectures don't implement it.  Broke my sparc64 build.
> >>>
> >>> Some architectures actually cannot implement it even... at least some
> >>> variants of powerpc MMUs don't have a combination of protection bits
> >>> that allow a kernel-only RO mapping (yeah odd).
> >>
> >> The simplest perceivable fix, to disable firewire-ohci on
> >> architectures which don't have PAGE_KERNEL_RO, would be bad since
> >> there are actually sparc64 machines with these controllers.
> >>
> >> As far as I can tell, the new RO mapping in firewire-ohci can as well be
> >> an r/w mapping.  We just never need to write at these virtual
> >> addresses. So, should we just change the driver to map it r/w when we
> >> can't have PAGE_KERNEL_RO, or for simplicity on all architectures?
> > 
> > What do I know...  Is a simple:
> > 
> >   #ifndef PAGE_KERNEL_RO
> >   #define PAGE_KERNEL_RO PAGE_KERNEL
> >   #endif
> > 
> > in drivers/firewire/ohci.c too hacky?
> 
> --8<---------------------------------------------------------------->8--
> firewire: ohci: fix compilation on arches without PAGE_KERNEL_RO, e.g. sparc
> 
> PAGE_KERNEL_RO is not available on all architectures, so its use in the
> new AR code broke compilation on sparc64.
> 
> Because the R/O mapping is only used to catch drivers that try to write
> to the reception buffer and not actually required for correct operation,
> we can just use a normal PAGE_KERNEL mapping where _RO is not available.
> 
> Thanks to Stefan Richter and Marc Gauthier <marc@tensilica.com> for
> suggesting this fix.
> 
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> 
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -91,6 +91,14 @@ struct descriptor {
>  #define MAX_AR_PACKET_SIZE	(16 + MAX_ASYNC_PAYLOAD + 4)
>  #define AR_WRAPAROUND_PAGES	DIV_ROUND_UP(MAX_AR_PACKET_SIZE, PAGE_SIZE)
>  
> +/*
> + * For archs where PAGE_KERNEL_RO is not supported;
> + * mapping the AR buffers readonly for the CPU is just a debugging aid.
> + */
> +#ifndef PAGE_KERNEL_RO
> +#define PAGE_KERNEL_RO PAGE_KERNEL
> +#endif

This might cause interesting issues on sparc64 if it ever acquired a
PAGE_KERNEL_RO.  Sparc64 has extern pgprot_t for it's PAGE_KERNEL types
rather than #defines, so the #ifdef check wouldn't see this.

I think either PAGE_PROT_RO becomes part of our arch API (so all
architectures are forced to add it), or, if it's not part of the API,
ohci isn't entitled to use it.  The latter seems simplest since you have
no real use for write protection anyway.

James