On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler <dpenkler@xxxxxxxxx> wrote:
> > On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> >> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler <dpenkler@xxxxxxxxx> wrote:
> >> > On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> >> >> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler <dpenkler@xxxxxxxxx> wrote:
> >>
> >> >> > + switch (status) {
> >> >> > + case 0: /* SUCCESS */
> >> >> > + if (data->iin_buffer[0] & 0x80) {
> >> >> > + /* check for valid STB notification */
> >> >> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
> >> >>
> >> >> Despite your answer to my comment code is quite understandable even with & 0x7e.
> >> >> You already put comment line about this, you may add that you validate
> >> >> the value to be 127 >= value >= 2.
> >> >>
> >> >
> >> > Yes it is quite understandable but it is less clear. I repeat my comment here:
> >> > When reading the spec and the code it is more obvious that here
> >> > we are testing for the value in bits D6..D0 to be a valid iin_bTag return.
> >> > (See Table 7 in the USBTMC-USB488 spec.)
> >> >
> >> > What is your motivation for
> >> >
> >> > if (data->iin_buffer[0] & 0x7e)
> >> >
> >> > ?
> >>
> >> In non-optimized variant it will certainly generate less code. You may
> >> have check assembly code with -O2 and compare. I don't know if
> >> compiler is clever enough to do the same by itself.
> >>
> >
> > I tested out both variants, and the explicit test is actually faster on by box:
> >
> > $ cat tp.c
> > #include <stdlib.h>
> > #include <stdio.h>
> > #define xstr(s) str(s)
> > #define str(s) #s
> > main() {
> > unsigned int v,s=0;
> > struct recs {
> > unsigned char *iin_buffer;
> > } rec;
> > struct recs *data = &rec;
> > data->iin_buffer = (unsigned char *) malloc(8);
> > for (v=1;v;v++) {
>
> > data->iin_buffer[0] = v & 0x7f;
>
> This line makes test fragile.
You are right, ignore this test.
snip
> Can you, please, check the assembly code in the real driver?
Below are the generated assembly code fragments using the standard
kernel makefile flags. The opcodes for the relevant instructions
are in capital letters. Comments added to show correspondence with C code.
Note that it is the combination of the two tests that must be considered:
6 instructions for the 0x7e version and 5 for the original.
Performance is the same so I guess we can stick with the original ?
#### Assembly for (data->iin_buffer[0] & 0x7e) version
.L258:
TESTB $126, %dl # if (data->iin_buffer[0] & 0x7e) goto moveit
JNE .L260
MOVL %edx, %eax # if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
ANDL $127, %eax
CMPB $1, %al
JNE .L234 # else goto .L234
tfasync cmpq $0, 168(%r12)
je .L237
leaq 168(%r12), %rdi
movl $131073, %edx
movl $29, %esi
call kill_fasync
.L237:
movl $1, 84(%r12)
.L255:
[snip]
.L260:
moveit movb %dl, 35(%r12)
movzbl 1(%rax), %eax
movl $1, 56(%r12)
movb %al, 36(%r12)
jmp .L255
#### Assembly for ((data->iin_buffer[0] & 0x7f) > 1) version
.L258:
MOVL %edx, %ecx # if ((data->iin_buffer[0] & 0x7f) > 1) goto moveit
ANDL $127, %ecx
CMPB $1, %cl
JBE .L235 # else goto .L235
moveit movb %dl, 35(%r12)
movzbl 1(%rax), %eax
movl $1, 56(%r12)
movb %al, 36(%r12)
.L255:
[snip]
.L235:
JNE .L234 # if ((data->iin_buffer[0] & 0x7f) == 1) goto tfasync
# else goto .L234
tfasync cmpq $0, 168(%r12)
je .L237
leaq 168(%r12), %rdi
movl $131073, %edx
movl $29, %esi
call kill_fasync
.L237:
movl $1, 84(%r12)
jmp .L255
> I can't do this right now, maybe tomorrow I will have few minutes to check that.
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
