Re: [PATCH 1/3] s390/kernel: add system calls for access PCI memory

[Martin Schwidefsky <schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>]

On Sun, 12 Oct 2014 11:52:55 +0000
Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:

> > +	switch (length) {
> > +	case 1:
> > +		ret = get_user(value.buf8, ((u8 *)user_buffer));
> 
> This cast (and similar casts across the code) kills the __user
> annotation of the user buffer pointer.
> First - fix this to help various static verification tools such
> as sparse work on your code.
> Second - are you sure this switch-case block achieves any
> performance gain compared to always using copy_from_user?
> If so, why not just push it into the S390 copy from user code?

The __user annotation is indeed missing. If the switch is improving
performance needs to be seen, with the compile options set for z10
the get_user is inlined while the copy_from_user calls a function.
For compiles < z10 all 5 switch cases will call the same
__copy_from_user function. So it depends, as long as the switch is
correct I am ok the code block for now.
 
> > +	switch (length) {
> > +	case 1:
> > +		value.buf8 = __raw_readb(io_addr);
> > +		ret = put_user(value.buf8, ((u8 *)user_buffer));
> 
> Add __user annotations in this code block as well.

Yes, please add.

> Generally speaking, looks OK once the __user annotation is added.
> 
> I suspect you might need ack/review from the S390 maintainer as
> well for this to be pushed, as the syscall is generic to the
> entire S390 subsystem.

With the missing __user annotations added:
Acked-by: Martin Schwidefsky <schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html