Re: [PATCH] powerpc/4xx/ocm: fix compiler error

[Christian Lamparter <chunkeey@gmail.com>]

On Saturday, December 22, 2018 2:08:02 PM CET christophe leroy wrote:
> 
> Le 22/12/2018 à 12:27, Christian Lamparter a écrit :
> > On Saturday, December 22, 2018 11:59:04 AM CET christophe leroy wrote:
> >>
> >> Le 22/12/2018 à 11:09, Christian Lamparter a écrit :
> >>> This patch fixes a recent regression in ocm:
> >>>
> >>> ocm.c: In function ‘ocm_init_node’:
> >>> ocm.c:182:18: error: invalid operands to binary |
> >>> ocm.c:197:17: error: invalid operands to binary |
> >>>
> >>> Fixes: 56f3c1413f5c ("powerpc/mm: properly set PAGE_KERNEL flags in ioremap()")
> >>> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> >>
> >> What's the problem here ? Is PAGE_KERNEL_NCG undefined ? If that's the
> >> case, wouldn't it be better for fix that by including asm/pgtable.h ?
> > 
> > PAGE_KERNEL[_NCG] type is a struct of pgprot_t, whereas _PAGE_EXEC is
> > considered an int.
> 
> Oops, I missed that.
> 
> Could you put the entire error message in the commit log ?
Will do in v2... which I'm going to sent out shortly. I mainly wanted to
make the most "trivial fix" since it probably needs to be backported to
4.20-stable at this late in the rc phase.

> > 
> > arch/powerpc/platforms/4xx/ocm.c: In function ‘ocm_init_node’:
> > arch/powerpc/platforms/4xx/ocm.c:182:18: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
> >         _PAGE_EXEC | PAGE_KERNEL_NCG);
> 
> Usually, Guarded implies no exec (at least on 6xx and 8xx). Does the 4xx 
> accept guarded exec ?

Oh, I simply copied over the individual _PAGE_* flags that the PAGE_KERNEL_NCG
macro set. The OCM is usually a small (32 KiB) DMA descriptor storage, I don't
think anyone would want to execute code in there.

> 
> >                    ^
> > arch/powerpc/platforms/4xx/ocm.c:197:17: error: invalid operands to binary | (have ‘int’ and ‘pgprot_t’ {aka ‘struct <anonymous>’})
> >         _PAGE_EXEC | PAGE_KERNEL);
> 
> That's PAGE_KERNEL_X
> 
> >                    ^
> > 
> > I think pgprot_val(PAGE_KERNEL[_NCG]) could be an option too.
> 
> Yes I may have a preference for that.

K, I liked having all _PAGE_*. But I'm fine either way. 
see you for v2. :-D 
> > 
> > Christian
> >>> ---
> >>>    arch/powerpc/platforms/4xx/ocm.c | 6 ++++--
> >>>    1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/platforms/4xx/ocm.c b/arch/powerpc/platforms/4xx/ocm.c
> >>> index 561b09de69bf..04ff69ddac09 100644
> >>> --- a/arch/powerpc/platforms/4xx/ocm.c
> >>> +++ b/arch/powerpc/platforms/4xx/ocm.c
> >>> @@ -179,7 +179,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >>>    	/* ioremap the non-cached region */
> >>>    	if (ocm->nc.memtotal) {
> >>>    		ocm->nc.virt = __ioremap(ocm->nc.phys, ocm->nc.memtotal,
> >>> -					 _PAGE_EXEC | PAGE_KERNEL_NCG);
> >>> +			_PAGE_EXEC | _PAGE_BASE_NC |
> >>> +			_PAGE_KERNEL_RW | _PAGE_NO_CACHE | _PAGE_GUARDED);
> 
> Could be _PAGE_BASE_NC | _PAGE_KERNEL_RWX | _PAGE_NO_CACHE | _PAGE_GUARDED
> 
> Or pgprot_val(PAGE_KERNEL_NCG) | _PAGE_EXEC
> 
> Or pgprot_val(pgprot_noncached(PAGE_KERNEL_RWX)) (that's my preference)
> 
> >>>    
> >>>    		if (!ocm->nc.virt) {
> >>>    			printk(KERN_ERR
> >>> @@ -194,7 +195,8 @@ static void __init ocm_init_node(int count, struct device_node *node)
> >>>    
> >>>    	if (ocm->c.memtotal) {
> >>>    		ocm->c.virt = __ioremap(ocm->c.phys, ocm->c.memtotal,
> >>> -					_PAGE_EXEC | PAGE_KERNEL);
> >>> +					_PAGE_EXEC | _PAGE_BASE |
> >>> +					_PAGE_KERNEL_RW);
> 
> What about _PAGE_BASE | _PAGE_KERNEL_RWX instead ?
> 
> Or pgprot_val(PAGE_KERNEL_RWX) (that's my preference)

I'll test if any of these make a difference and sent a separate patch.

Christian