[bug report] powerpc/mm/radix: Add tlbflush routines

[bug report] powerpc/mm/radix: Add tlbflush routines

[Dan Carpenter]

Hello Aneesh Kumar K.V,

The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
from Apr 29, 2016, leads to the following static checker warning:

	arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
	warn: always true condition '(pid != ~0) => (0-u32max != u64max)'

arch/powerpc/mm/tlb_nohash.c
   211  void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
   212                              int tsize, int ind)
   213  {
   214          unsigned int pid;
   215  
   216          preempt_disable();
   217          pid = mm ? mm->context.id : 0;
   218          if (pid != MMU_NO_CONTEXT)
                    ^^^^^^^^^^^^^^^^^^^^^
   219                  _tlbil_va(vmaddr, pid, tsize, ind);
   220          preempt_enable();
   221  }

I don't know very much about PowerPC.  The static checker is guessing
which headers to pull in instead of relying on the build system so there
are a lot of false positives.  It's apparently using the
arch/powerpc/include/asm/book3s/64/tlbflush.h header which does:

#define MMU_NO_CONTEXT ~0UL

so it's UINT_MAX vs U64_MAX which is making the checker complain.

regards,
dan carpenter

Re: [bug report] powerpc/mm/radix: Add tlbflush routines

[Christophe LEROY]

Le 31/01/2018 à 16:45, Dan Carpenter a écrit :
> Hello Aneesh Kumar K.V,
> 
> The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
> from Apr 29, 2016, leads to the following static checker warning:
> 
> 	arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
> 	warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
> 
> arch/powerpc/mm/tlb_nohash.c
>     211  void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
>     212                              int tsize, int ind)
>     213  {
>     214          unsigned int pid;
>     215
>     216          preempt_disable();
>     217          pid = mm ? mm->context.id : 0;
>     218          if (pid != MMU_NO_CONTEXT)
>                      ^^^^^^^^^^^^^^^^^^^^^
>     219                  _tlbil_va(vmaddr, pid, tsize, ind);
>     220          preempt_enable();
>     221  }
> 
> I don't know very much about PowerPC.  The static checker is guessing
> which headers to pull in instead of relying on the build system so there
> are a lot of false positives.  It's apparently using the
> arch/powerpc/include/asm/book3s/64/tlbflush.h header which does:
> 
> #define MMU_NO_CONTEXT ~0UL
> 
> so it's UINT_MAX vs U64_MAX which is making the checker complain.

As far as I can see from arch/powerpc/include/asm/mmu-hash64.h, 
mm->context.id is an unsigned long, so pid should also be an unsigned 
long, not an unsigned int ?

Christophe

> 
> regards,
> dan carpenter
> 

Re: [bug report] powerpc/mm/radix: Add tlbflush routines

[Aneesh Kumar K.V]

On 01/31/2018 09:31 PM, Christophe LEROY wrote:
> 
> 
> Le 31/01/2018 à 16:45, Dan Carpenter a écrit :
>> Hello Aneesh Kumar K.V,
>>
>> The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
>> from Apr 29, 2016, leads to the following static checker warning:
>>
>>     arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
>>     warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
>>
>> arch/powerpc/mm/tlb_nohash.c
>>     211  void __local_flush_tlb_page(struct mm_struct *mm, unsigned 
>> long vmaddr,
>>     212                              int tsize, int ind)
>>     213  {
>>     214          unsigned int pid;
>>     215
>>     216          preempt_disable();
>>     217          pid = mm ? mm->context.id : 0;
>>     218          if (pid != MMU_NO_CONTEXT)
>>                      ^^^^^^^^^^^^^^^^^^^^^
>>     219                  _tlbil_va(vmaddr, pid, tsize, ind);
>>     220          preempt_enable();
>>     221  }
>>
>> I don't know very much about PowerPC.  The static checker is guessing
>> which headers to pull in instead of relying on the build system so there
>> are a lot of false positives.  It's apparently using the
>> arch/powerpc/include/asm/book3s/64/tlbflush.h header which does:
>>
>> #define MMU_NO_CONTEXT ~0UL
>>
>> so it's UINT_MAX vs U64_MAX which is making the checker complain.
> 
> As far as I can see from arch/powerpc/include/asm/mmu-hash64.h, 
> mm->context.id is an unsigned long, so pid should also be an unsigned 
> long, not an unsigned int ?
> 
> Christophe
> 

yes.

We did similar fixup for book3s radix
in 9690c15742688e9cb5ee4aa0b08e458551ceea13 (powerpc/mm/radix: Fix 
always false comparison against MMU_NO_CONTEXT
)

-aneesh

Re: [bug report] powerpc/mm/radix: Add tlbflush routines

[Michael Ellerman]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hello Aneesh Kumar K.V,
>
> The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
> from Apr 29, 2016, leads to the following static checker warning:
>
> 	arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
> 	warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
>
> arch/powerpc/mm/tlb_nohash.c
>    211  void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
>    212                              int tsize, int ind)
>    213  {
>    214          unsigned int pid;
>    215  
>    216          preempt_disable();
>    217          pid = mm ? mm->context.id : 0;
>    218          if (pid != MMU_NO_CONTEXT)
>                     ^^^^^^^^^^^^^^^^^^^^^
>    219                  _tlbil_va(vmaddr, pid, tsize, ind);
>    220          preempt_enable();
>    221  }
>
> I don't know very much about PowerPC.  The static checker is guessing
> which headers to pull in instead of relying on the build system so there
> are a lot of false positives.

O_o 

That's a bit nuts ... :)

> It's apparently using the arch/powerpc/include/asm/book3s/64/tlbflush.h header which does:
>
> #define MMU_NO_CONTEXT ~0UL
>
> so it's UINT_MAX vs U64_MAX which is making the checker complain.

That's the wrong header for that file.

It should be arch/powerpc/include/asm/tlbflush.h, which does one of:

  #define MMU_NO_CONTEXT      ((unsigned int)-1)
  #define MMU_NO_CONTEXT      (0)


So I think the code is OK in this case.

The clue is that the code is in tlb_nohash.c, but the header is
book3s/64, and Book3S doesn't use the nohash style MMUs.

Thanks for trying, powerpc has the unfortunate feature of supporting
feature of supporting about 5 different MMUs, which means this area of
the code is pretty gross.

cheers

Re: [bug report] powerpc/mm/radix: Add tlbflush routines

[Dan Carpenter]

On Wed, Jan 31, 2018 at 08:58:50PM -0800, Michael Ellerman wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > Hello Aneesh Kumar K.V,
> >
> > The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
> > from Apr 29, 2016, leads to the following static checker warning:
> >
> > 	arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
> > 	warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
> >
> > arch/powerpc/mm/tlb_nohash.c
> >    211  void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
> >    212                              int tsize, int ind)
> >    213  {
> >    214          unsigned int pid;
> >    215  
> >    216          preempt_disable();
> >    217          pid = mm ? mm->context.id : 0;
> >    218          if (pid != MMU_NO_CONTEXT)
> >                     ^^^^^^^^^^^^^^^^^^^^^
> >    219                  _tlbil_va(vmaddr, pid, tsize, ind);
> >    220          preempt_enable();
> >    221  }
> >
> > I don't know very much about PowerPC.  The static checker is guessing
> > which headers to pull in instead of relying on the build system so there
> > are a lot of false positives.
> 
> O_o 
> 
> That's a bit nuts ... :)
> 

Heh.

Thanks for looking into this.

regards,
dan carpenter