On Sun, 12 Oct 2014 11:52:55 +0000
Shachar Raindel <raindel@xxxxxxxxxxxx> 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@xxxxxxxxxx>
--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
