Re: Question regarding anotation

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Mon, Aug 22, 2016 at 05:25:50PM +0000, Nicholas Mc Guire wrote:
> 
> Hi !
> 
>  A probably very basic sparse question - but I could not figure out a
>  satisfying answer. while compile testing a patch for ntb_transfer.c I got
> 
>   CHECK   drivers/ntb/ntb_transport.c
> drivers/ntb/ntb_transport.c:1583:43: warning: incorrect type in argument 1 (different address spaces)
> drivers/ntb/ntb_transport.c:1583:43:    expected void *dst
> drivers/ntb/ntb_transport.c:1583:43:    got void [noderef] <asn:2>*offset
> drivers/ntb/ntb_transport.c:1583:56: warning: incorrect type in argument 2 (different address spaces)
> drivers/ntb/ntb_transport.c:1583:56:    expected void const [noderef] <asn:1>*src
> drivers/ntb/ntb_transport.c:1583:56:    got void *buf
> 
>  looking at the offending line;
> 
> static void ntb_memcpy_tx(struct ntb_queue_entry *entry, void __iomem *offset)
> {
> #ifdef ARCH_HAS_NOCACHE_UACCESS
>         /*
>          * Using non-temporal mov to improve performance on non-cached
>          * writes, even though we aren't actually copying from user space.
>          */
>         __copy_from_user_inatomic_nocache(offset, entry->buf, entry->len);
> 
>  the absence of a __user in the buf argument seems intentional and the
>  dst is not a kernel but __iomem address which triggers the second warning
>  So the first warning seems to be a false positive here, the second one
>  Im not clear about, but I guess its also a false positive. The question
>  is if there is a clean way to anotate this to make sparse happy without 
>  any unwanted sideffects ?
> 
>  For the first warning using  (void __user *) entry->buf  should do
>  and be side-effect free, but how could the second warning be fixed ?

So the original code, now in the #else part was:
	memcpy_toio(offset, entry->buf, entry->len);
which is correct regarding the annotation/extended typing:
	- offset points to IO memory (annotated as void __iomem *)
	- entry->buff is normal kernel memory (no annotation needed)

I have no idea if replacing this by __copy_from_user_inatomic_nocache()
does indeed improve performance or not, but it's a complete abuse of this
function's typing and intent and sparse rightfully complains about both case.

So, I really think that these warnings don't need any fixing and that hiding
them behind a cast is not a good idea at all.
Independently of the validity of using __copy_from_user_inatomic_nocache()
here it would be much better to have something like memcpy_toio_nocache().
Doesn't something like this already exist?
Why not make something using the primitives already used by 
__copy_from_user_inatomic_nocache() (where, if really needed, using a cast
to "adjust" the types is much more justified)?


Regards,
Luc Van Oostenryck