Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.

[Dave Penkler <dpenkler@gmail.com>]

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@gmail.com> 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@gmail.com> 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@gmail.com> 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