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

2015-11-28 Thread Andy Shevchenko 
On Sat, Nov 28, 2015 at 7:41 PM, Dave Penkler  wrote:
> On Sat, Nov 28, 2015 at 04:57:47PM +0200, Andy Shevchenko wrote:
>> On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
>> > On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  
>> >> >> 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  
>> >> >> >> wrote:
>>
>> Thank you for an update!
>>
>> >> >> >> > +   switch (status) {
>> >> >> >> > +   case 0: /* SUCCESS */
>> >> >> >> > +   if (data->iin_buffer[0] & 0x80) {
>> >> >> >> > +   /* check for valid STB notification */
>> >> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {
>>
>> How can I miss that there are two conditionals in a sequence and
>> moreover for the same data?!
>
> Sorry, my fault, it is the combination of patch 1 and 2
>
>> That might explain the optimization done by compiler.
>>
>> So, could it be transformed to simple one condition
>>if (data->iin_buffer[0] > 0x81 /* 129 */) {
>> ?
>
> OK so now for patch 1 and 2 we have:
>
> switch (status) {
> case 0: /* SUCCESS */
> /* PATCH 1 check for valid STB notification */
> if (data->iin_buffer[0] > 0x81) {
> data->bNotify1 = data->iin_buffer[0];
> data->bNotify2 = data->iin_buffer[1];
> atomic_set(>iin_data_valid, 1);
> wake_up_interruptible(>waitq);
> goto exit;
> }
> /* PATCH 2 check for SRQ notification */
> if (data->iin_buffer[0] == 0x81) {
> if (data->fasync)
> kill_fasync(>fasync,
> SIGIO, POLL_IN);
>
> atomic_set(>srq_asserted, 1);
> wake_up_interruptible(>waitq);
> goto exit;
> }
>
>
> I'll push a new set if you are OK with this.

Yes, fine by me, thanks for patience.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Dave Penkler 
On Sat, Nov 28, 2015 at 04:57:47PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
> > On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> >> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  
> >> >> 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  
> >> >> >> wrote:
> 
> Thank you for an update!
> 
> >> >> >> > +   switch (status) {
> >> >> >> > +   case 0: /* SUCCESS */
> >> >> >> > +   if (data->iin_buffer[0] & 0x80) {
> >> >> >> > +   /* check for valid STB notification */
> >> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {
> 
> How can I miss that there are two conditionals in a sequence and
> moreover for the same data?!

Sorry, my fault, it is the combination of patch 1 and 2

> That might explain the optimization done by compiler.
> 
> So, could it be transformed to simple one condition
>if (data->iin_buffer[0] > 0x81 /* 129 */) {
> ?

OK so now for patch 1 and 2 we have:

switch (status) {
case 0: /* SUCCESS */
/* PATCH 1 check for valid STB notification */
if (data->iin_buffer[0] > 0x81) {
data->bNotify1 = data->iin_buffer[0];
data->bNotify2 = data->iin_buffer[1];
atomic_set(>iin_data_valid, 1);
wake_up_interruptible(>waitq);
goto exit;
}
/* PATCH 2 check for SRQ notification */
if (data->iin_buffer[0] == 0x81) {
if (data->fasync)
kill_fasync(>fasync,
SIGIO, POLL_IN);

atomic_set(>srq_asserted, 1);
wake_up_interruptible(>waitq);
goto exit;
}


I'll push a new set if you are OK with this.
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Andy Shevchenko 
On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
> On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
>> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  
>> >> >> wrote:

Thank you for an update!

>> >> >> > +   switch (status) {
>> >> >> > +   case 0: /* SUCCESS */
>> >> >> > +   if (data->iin_buffer[0] & 0x80) {
>> >> >> > +   /* check for valid STB notification */
>> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {

How can I miss that there are two conditionals in a sequence and
moreover for the same data?!
That might explain the optimization done by compiler.

So, could it be transformed to simple one condition
   if (data->iin_buffer[0] > 0x81 /* 129 */) {
?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Dave Penkler 
On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  
> >> >> 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 
> > #include 
> > #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 = 
> > 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
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_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
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Dave Penkler 
On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  
> >> >> 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 
> > #include 
> > #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 = 
> > 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
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_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
leaq168(%r12), %rdi
movl$131073, %edx
movl$29, %esi
callkill_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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Dave Penkler 
On Sat, Nov 28, 2015 at 04:57:47PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
> > On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
> >> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  
> >> >> 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  
> >> >> >> wrote:
> 
> Thank you for an update!
> 
> >> >> >> > +   switch (status) {
> >> >> >> > +   case 0: /* SUCCESS */
> >> >> >> > +   if (data->iin_buffer[0] & 0x80) {
> >> >> >> > +   /* check for valid STB notification */
> >> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {
> 
> How can I miss that there are two conditionals in a sequence and
> moreover for the same data?!

Sorry, my fault, it is the combination of patch 1 and 2

> That might explain the optimization done by compiler.
> 
> So, could it be transformed to simple one condition
>if (data->iin_buffer[0] > 0x81 /* 129 */) {
> ?

OK so now for patch 1 and 2 we have:

switch (status) {
case 0: /* SUCCESS */
/* PATCH 1 check for valid STB notification */
if (data->iin_buffer[0] > 0x81) {
data->bNotify1 = data->iin_buffer[0];
data->bNotify2 = data->iin_buffer[1];
atomic_set(>iin_data_valid, 1);
wake_up_interruptible(>waitq);
goto exit;
}
/* PATCH 2 check for SRQ notification */
if (data->iin_buffer[0] == 0x81) {
if (data->fasync)
kill_fasync(>fasync,
SIGIO, POLL_IN);

atomic_set(>srq_asserted, 1);
wake_up_interruptible(>waitq);
goto exit;
}


I'll push a new set if you are OK with this.
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Andy Shevchenko 
On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
> On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
>> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  
>> >> >> wrote:

Thank you for an update!

>> >> >> > +   switch (status) {
>> >> >> > +   case 0: /* SUCCESS */
>> >> >> > +   if (data->iin_buffer[0] & 0x80) {
>> >> >> > +   /* check for valid STB notification */
>> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {

How can I miss that there are two conditionals in a sequence and
moreover for the same data?!
That might explain the optimization done by compiler.

So, could it be transformed to simple one condition
   if (data->iin_buffer[0] > 0x81 /* 129 */) {
?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-28 Thread Andy Shevchenko 
On Sat, Nov 28, 2015 at 7:41 PM, Dave Penkler  wrote:
> On Sat, Nov 28, 2015 at 04:57:47PM +0200, Andy Shevchenko wrote:
>> On Sat, Nov 28, 2015 at 1:55 PM, Dave Penkler  wrote:
>> > On Wed, Nov 25, 2015 at 10:38:39PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  
>> >> >> 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  
>> >> >> >> wrote:
>>
>> Thank you for an update!
>>
>> >> >> >> > +   switch (status) {
>> >> >> >> > +   case 0: /* SUCCESS */
>> >> >> >> > +   if (data->iin_buffer[0] & 0x80) {
>> >> >> >> > +   /* check for valid STB notification */
>> >> >> >> > +   if ((data->iin_buffer[0] & 0x7f) > 1) {
>>
>> How can I miss that there are two conditionals in a sequence and
>> moreover for the same data?!
>
> Sorry, my fault, it is the combination of patch 1 and 2
>
>> That might explain the optimization done by compiler.
>>
>> So, could it be transformed to simple one condition
>>if (data->iin_buffer[0] > 0x81 /* 129 */) {
>> ?
>
> OK so now for patch 1 and 2 we have:
>
> switch (status) {
> case 0: /* SUCCESS */
> /* PATCH 1 check for valid STB notification */
> if (data->iin_buffer[0] > 0x81) {
> data->bNotify1 = data->iin_buffer[0];
> data->bNotify2 = data->iin_buffer[1];
> atomic_set(>iin_data_valid, 1);
> wake_up_interruptible(>waitq);
> goto exit;
> }
> /* PATCH 2 check for SRQ notification */
> if (data->iin_buffer[0] == 0x81) {
> if (data->fasync)
> kill_fasync(>fasync,
> SIGIO, POLL_IN);
>
> atomic_set(>srq_asserted, 1);
> wake_up_interruptible(>waitq);
> goto exit;
> }
>
>
> I'll push a new set if you are OK with this.

Yes, fine by me, thanks for patience.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-25 Thread Andy Shevchenko 
On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  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 
> #include 
> #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 = 
> data->iin_buffer = (unsigned char *) malloc(8);
> for (v=1;v;v++) {

> data->iin_buffer[0] = v & 0x7f;

This line makes test fragile.

> if (TEST)
> s++;
> }
> printf("%s %x\n",xstr(TEST),s);
> }
> $ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
> $ time ./a.out
> data->iin_buffer[0] & 0x7e fc00
>
> real0m3.927s
> user0m3.920s
> sys 0m0.000s
> $ time ./a.out
> data->iin_buffer[0] & 0x7e fc00
>
> real0m3.925s
> user0m3.920s
> sys 0m0.000s
> $ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
> $ time ./a.out
> (data->iin_buffer[0] & 0x7f) > 1 fc00
>
> real0m2.638s
> user0m2.610s
> sys 0m0.000s
> $ time ./a.out
> (data->iin_buffer[0] & 0x7f) > 1 fc00
>
> real0m2.648s
> user0m2.620s
> sys 0m0.000s

Can you, please, check the assembly code in the real driver?
I can't do this right now, maybe tomorrow I will have few minutes to check that.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-25 Thread Dave Penkler 
On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler  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  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 
#include 
#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 = 
data->iin_buffer = (unsigned char *) malloc(8);
for (v=1;v;v++) {
data->iin_buffer[0] = v & 0x7f;
if (TEST)
s++;
}
printf("%s %x\n",xstr(TEST),s);
}
$ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00

real0m3.927s
user0m3.920s
sys 0m0.000s
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00

real0m3.925s
user0m3.920s
sys 0m0.000s
$ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00

real0m2.638s
user0m2.610s
sys 0m0.000s
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00

real0m2.648s
user0m2.620s
sys 0m0.000s

> >> > +   /* urb terminated, clean up */
> >> > +   dev_dbg(>intf->dev,
> >> > +   "%s - urb terminated, status: %d\n",
> >> > +   __func__, status);
> >>
> >> No need to print function here explicitly. Check Dynamic Debug framework.
> >
> > I am not using dynamic debug but when I enable static debug I get:
> >
> > [ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
> >
> > on the console log for
> >
> > dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> > data->iin_ep_present);
> >
> > So if I don't print the function it does not appear on the log.
> 
> Whatever maintainers prefer, though I think there are quite rare cases
> in USB when someone needs static debug. I'm pretty sure most of the
> developers all in favour of dynamic debug.
> 

I am happy to remove the func

> >> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >> >
> >> > retcode = usb_register_dev(intf, _class);
> >>
> >> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-25 Thread Andy Shevchenko 
On Wed, Nov 25, 2015 at 11:18 AM, Dave Penkler  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  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  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 
> #include 
> #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 = 
> data->iin_buffer = (unsigned char *) malloc(8);
> for (v=1;v;v++) {

> data->iin_buffer[0] = v & 0x7f;

This line makes test fragile.

> if (TEST)
> s++;
> }
> printf("%s %x\n",xstr(TEST),s);
> }
> $ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
> $ time ./a.out
> data->iin_buffer[0] & 0x7e fc00
>
> real0m3.927s
> user0m3.920s
> sys 0m0.000s
> $ time ./a.out
> data->iin_buffer[0] & 0x7e fc00
>
> real0m3.925s
> user0m3.920s
> sys 0m0.000s
> $ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
> $ time ./a.out
> (data->iin_buffer[0] & 0x7f) > 1 fc00
>
> real0m2.638s
> user0m2.610s
> sys 0m0.000s
> $ time ./a.out
> (data->iin_buffer[0] & 0x7f) > 1 fc00
>
> real0m2.648s
> user0m2.620s
> sys 0m0.000s

Can you, please, check the assembly code in the real driver?
I can't do this right now, maybe tomorrow I will have few minutes to check that.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-25 Thread Dave Penkler 
On Sun, Nov 22, 2015 at 12:32:41PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler  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  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 
#include 
#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 = 
data->iin_buffer = (unsigned char *) malloc(8);
for (v=1;v;v++) {
data->iin_buffer[0] = v & 0x7f;
if (TEST)
s++;
}
printf("%s %x\n",xstr(TEST),s);
}
$ cc -O2 tp.c -DTEST='data->iin_buffer[0] & 0x7e'
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00

real0m3.927s
user0m3.920s
sys 0m0.000s
$ time ./a.out
data->iin_buffer[0] & 0x7e fc00

real0m3.925s
user0m3.920s
sys 0m0.000s
$ cc -O2 tp.c -DTEST='(data->iin_buffer[0] & 0x7f) > 1'
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00

real0m2.638s
user0m2.610s
sys 0m0.000s
$ time ./a.out
(data->iin_buffer[0] & 0x7f) > 1 fc00

real0m2.648s
user0m2.620s
sys 0m0.000s

> >> > +   /* urb terminated, clean up */
> >> > +   dev_dbg(>intf->dev,
> >> > +   "%s - urb terminated, status: %d\n",
> >> > +   __func__, status);
> >>
> >> No need to print function here explicitly. Check Dynamic Debug framework.
> >
> > I am not using dynamic debug but when I enable static debug I get:
> >
> > [ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
> >
> > on the console log for
> >
> > dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> > data->iin_ep_present);
> >
> > So if I don't print the function it does not appear on the log.
> 
> Whatever maintainers prefer, though I think there are quite rare cases
> in USB when someone needs static debug. I'm pretty sure most of the
> developers all in favour of dynamic debug.
> 

I am happy to remove the func

> >> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >> >
> >> > retcode = usb_register_dev(intf, _class);
> >>
> >> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-22 Thread Andy Shevchenko 
On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler  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  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.

>> > +   /* urb terminated, clean up */
>> > +   dev_dbg(>intf->dev,
>> > +   "%s - urb terminated, status: %d\n",
>> > +   __func__, status);
>>
>> No need to print function here explicitly. Check Dynamic Debug framework.
>
> I am not using dynamic debug but when I enable static debug I get:
>
> [ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
>
> on the console log for
>
> dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> data->iin_ep_present);
>
> So if I don't print the function it does not appear on the log.

Whatever maintainers prefer, though I think there are quite rare cases
in USB when someone needs static debug. I'm pretty sure most of the
developers all in favour of dynamic debug.

>> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
>> >
>> > retcode = usb_register_dev(intf, _class);
>>
>> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-22 Thread Dave Penkler 
On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler  wrote:
> > Background:
> > When performing a read on an instrument that is executing a function
> > that runs longer than the USB timeout the instrument may hang and
> > require a device reset to recover. The READ_STATUS_BYTE operation
> > always returns even when the instrument is busy permitting to poll
> > for the appropriate condition. This capability is referred to in
> > instrument application notes on synchronizing acquisitions for other
> > platforms.
> >
> 
> First of all, please be patient and do not send patches immediately
> when you answered to someone's review. It increases redundant noise
> and burden on reviewer. Wait at least for 24h especially if there are
> topics still to discuss.
> 

OK, apologies.

snip

> > +
> > +   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)

?

> > +   data->bNotify1 = data->iin_buffer[0];
> > +   data->bNotify2 = data->iin_buffer[1];
> > +   atomic_set(>iin_data_valid, 1);
> > +   wake_up_interruptible(>waitq);
> > +   goto exit;
> > +   }
> > +   }

snip

> > +   /* urb terminated, clean up */
> > +   dev_dbg(>intf->dev,
> > +   "%s - urb terminated, status: %d\n",
> > +   __func__, status);
> 
> No need to print function here explicitly. Check Dynamic Debug framework.

I am not using dynamic debug but when I enable static debug I get:

[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1

on the console log for
 
dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
data->iin_ep_present);

So if I don't print the function it does not appear on the log.

> 
> > +   return;
> > +   default:
> > +   dev_err(>intf->dev,
> > +   "%s - unknown status received: %d\n",
> > +   __func__, status);
> > +   }
> > +exit:
> > +   rv = usb_submit_urb(urb, GFP_ATOMIC);
> > +   if (rv) {
> > +   dev_err(>intf->dev, "%s - usb_submit_urb failed: 
> > %d\n",
> > +   __func__, rv);
> > +   }
> > +}

snip

> > +
> > +   /* fill interrupt urb */
> > +   usb_fill_int_urb(data->iin_urb, data->usb_dev,
> > +   usb_rcvintpipe(data->usb_dev, data->iin_ep),
> > +   data->iin_buffer, data->iin_wMaxPacketSize,
> > +   usbtmc_interrupt,
> > +   data, data->iin_interval);
> > +
> > +   if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
> 
> Does it return a proper error code?
> 

Yes, will propagate it.

> > +   retcode = -EIO;
> > +   dev_err(>dev, "Failed to submit iin_urb\n");
> > +   goto error_register;
> > +   }
> > +   }
> > +
> 
> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >
> > retcode = usb_register_dev(intf, _class);
> 
> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
> 
> 
> > @@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
> >  error_register:
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > +   usbtmc_free_int(data);
> > kref_put(>kref, usbtmc_delete);
> > return retcode;
> >  }
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-22 Thread Dave Penkler 
On Wed, Nov 18, 2015 at 11:55:27AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler  wrote:
> > Background:
> > When performing a read on an instrument that is executing a function
> > that runs longer than the USB timeout the instrument may hang and
> > require a device reset to recover. The READ_STATUS_BYTE operation
> > always returns even when the instrument is busy permitting to poll
> > for the appropriate condition. This capability is referred to in
> > instrument application notes on synchronizing acquisitions for other
> > platforms.
> >
> 
> First of all, please be patient and do not send patches immediately
> when you answered to someone's review. It increases redundant noise
> and burden on reviewer. Wait at least for 24h especially if there are
> topics still to discuss.
> 

OK, apologies.

snip

> > +
> > +   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)

?

> > +   data->bNotify1 = data->iin_buffer[0];
> > +   data->bNotify2 = data->iin_buffer[1];
> > +   atomic_set(>iin_data_valid, 1);
> > +   wake_up_interruptible(>waitq);
> > +   goto exit;
> > +   }
> > +   }

snip

> > +   /* urb terminated, clean up */
> > +   dev_dbg(>intf->dev,
> > +   "%s - urb terminated, status: %d\n",
> > +   __func__, status);
> 
> No need to print function here explicitly. Check Dynamic Debug framework.

I am not using dynamic debug but when I enable static debug I get:

[ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1

on the console log for
 
dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
data->iin_ep_present);

So if I don't print the function it does not appear on the log.

> 
> > +   return;
> > +   default:
> > +   dev_err(>intf->dev,
> > +   "%s - unknown status received: %d\n",
> > +   __func__, status);
> > +   }
> > +exit:
> > +   rv = usb_submit_urb(urb, GFP_ATOMIC);
> > +   if (rv) {
> > +   dev_err(>intf->dev, "%s - usb_submit_urb failed: 
> > %d\n",
> > +   __func__, rv);
> > +   }
> > +}

snip

> > +
> > +   /* fill interrupt urb */
> > +   usb_fill_int_urb(data->iin_urb, data->usb_dev,
> > +   usb_rcvintpipe(data->usb_dev, data->iin_ep),
> > +   data->iin_buffer, data->iin_wMaxPacketSize,
> > +   usbtmc_interrupt,
> > +   data, data->iin_interval);
> > +
> > +   if (usb_submit_urb(data->iin_urb, GFP_KERNEL)) {
> 
> Does it return a proper error code?
> 

Yes, will propagate it.

> > +   retcode = -EIO;
> > +   dev_err(>dev, "Failed to submit iin_urb\n");
> > +   goto error_register;
> > +   }
> > +   }
> > +
> 
> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
> >
> > retcode = usb_register_dev(intf, _class);
> 
> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.
> 
> 
> > @@ -1185,6 +1395,7 @@ static int usbtmc_probe(struct usb_interface *intf,
> >  error_register:
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > sysfs_remove_group(>dev.kobj, _attr_grp);
> > +   usbtmc_free_int(data);
> > kref_put(>kref, usbtmc_delete);
> > return retcode;
> >  }
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
cheers,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-22 Thread Andy Shevchenko 
On Sun, Nov 22, 2015 at 11:19 AM, Dave Penkler  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  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.

>> > +   /* urb terminated, clean up */
>> > +   dev_dbg(>intf->dev,
>> > +   "%s - urb terminated, status: %d\n",
>> > +   __func__, status);
>>
>> No need to print function here explicitly. Check Dynamic Debug framework.
>
> I am not using dynamic debug but when I enable static debug I get:
>
> [ 1438.562458] usbtmc 1-1:1.0: Enter ioctl_read_stb iin_ep_present: 1
>
> on the console log for
>
> dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> data->iin_ep_present);
>
> So if I don't print the function it does not appear on the log.

Whatever maintainers prefer, though I think there are quite rare cases
in USB when someone needs static debug. I'm pretty sure most of the
developers all in favour of dynamic debug.

>> > retcode = sysfs_create_group(>dev.kobj, _attr_grp);
>> >
>> > retcode = usb_register_dev(intf, _class);
>>
>> Hmm??? Unrelated to this patch, but notice that retcode is overridden here.



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-11-18 Thread Andy Shevchenko 
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler  wrote:
> Background:
> When performing a read on an instrument that is executing a function
> that runs longer than the USB timeout the instrument may hang and
> require a device reset to recover. The READ_STATUS_BYTE operation
> always returns even when the instrument is busy permitting to poll
> for the appropriate condition. This capability is referred to in
> instrument application notes on synchronizing acquisitions for other
> platforms.
>

First of all, please be patient and do not send patches immediately
when you answered to someone's review. It increases redundant noise
and burden on reviewer. Wait at least for 24h especially if there are
topics still to discuss.

[]

> +static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
> +   unsigned long arg)
> +{
> +   u8 *buffer;
> +   struct device *dev = >intf->dev;
> +   int rv;
> +   u8 tag, stb;

If you rearrange this like

+   struct device *dev = >intf->dev;
+   u8 *buffer;
+   u8 tag, stb;
+   int rv;

it will look better. Similar to check in all your patches.

> +
> +   dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> +   data->iin_ep_present);
> +
> +   buffer = kmalloc(8, GFP_KERNEL);
> +   if (!buffer)
> +   return -ENOMEM;
> +
> +   atomic_set(>iin_data_valid, 0);
> +
> +   rv = usb_control_msg(data->usb_dev,
> +   usb_rcvctrlpipe(data->usb_dev, 0),
> +   USBTMC488_REQUEST_READ_STATUS_BYTE,
> +   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +   data->iin_bTag,
> +   data->ifnum,
> +   buffer, 0x03, USBTMC_TIMEOUT);
> +

Redundant empty line.

> +   if (rv < 0) {
> +   dev_err(dev, "stb usb_control_msg returned %d\n", rv);
> +   goto exit;
> +   }
> +
> +   if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> +   dev_err(dev, "control status returned %x\n", buffer[0]);
> +   rv = -EIO;
> +   goto exit;
> +   }
> +
> +   if (data->iin_ep_present) {
> +   rv = wait_event_interruptible_timeout(
> +   data->waitq,
> +   atomic_read(>iin_data_valid) != 0,
> +   USBTMC_TIMEOUT);
> +

Ditto.

> +   if (rv < 0) {
> +   dev_dbg(dev, "wait interrupted %d\n", rv);
> +   goto exit;
> +   }
> +
> +   if (rv == 0) {
> +   dev_dbg(dev, "wait timed out\n");
> +   rv = -ETIME;
> +   goto exit;
> +   }
> +
> +   tag = data->bNotify1 & 0x7f;
> +

You may remove empty line, though it's minor nitpick here.

> +   if (tag != data->iin_bTag) {
> +   dev_err(dev, "expected bTag %x got %x\n",
> +   data->iin_bTag, tag);
> +   }
> +
> +   stb = data->bNotify2;
> +   } else {
> +   stb = buffer[2];
> +   }
> +
> +   rv = copy_to_user((void __user *)arg, , sizeof(stb));
> +   if (rv)
> +   rv = -EFAULT;
> +
> + exit:
> +   /* bump interrupt bTag */
> +   data->iin_bTag += 1;
> +   if (data->iin_bTag > 127)
> +   /* 1 is for SRQ see USBTMC-USB488 subclass specification 
> 4.3.1*/

Missed space before asterisk.

> +   data->iin_bTag = 2;
> +
> +   kfree(buffer);
> +   return rv;
> +}

[]

> +static void usbtmc_interrupt(struct urb *urb)
> +{
> +   struct usbtmc_device_data *data = urb->context;
> +   int status = urb->status;
> +   int rv;
> +
> +   dev_dbg(>intf->dev, "int status: %d len %d\n",
> +   status, urb->actual_length);
> +
> +   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.

> +   data->bNotify1 = data->iin_buffer[0];
> +   data->bNotify2 = data->iin_buffer[1];
> +   atomic_set(>iin_data_valid, 1);
> +   wake_up_interruptible(>waitq);
> +   goto exit;
> +   }
> +   }
> +   dev_warn(>intf->dev, "invalid notification: %x\n",
> +   data->iin_buffer[0]);
> +   break;
> +   case -EOVERFLOW:
> +   dev_err(>intf->dev,
> +   "%s - overflow with length %d,

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

2015-11-18 Thread Dave Penkler 
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.

Signed-off-by: Dave Penkler 
---
 drivers/usb/class/usbtmc.c   | 212 +++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 214 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..60a71cb 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read;  /* needed for abort */
 
+   /* data for interrupt in endpoint handling */
+   u8 bNotify1;
+   u8 bNotify2;
+   u16ifnum;
+   u8 iin_bTag;
+   u8*iin_buffer;
+   atomic_t   iin_data_valid;
+   unsigned int   iin_ep;
+   intiin_ep_present;
+   intiin_interval;
+   struct urb*iin_urb;
+   u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
 
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities  capabilities;
struct kref kref;
struct mutex io_mutex;  /* only one i/o function running at a time */
+   wait_queue_head_t waitq;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
 
@@ -373,6 +387,86 @@ exit:
return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+   unsigned long arg)
+{
+   u8 *buffer;
+   struct device *dev = >intf->dev;
+   int rv;
+   u8 tag, stb;
+
+   dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+   data->iin_ep_present);
+
+   buffer = kmalloc(8, GFP_KERNEL);
+   if (!buffer)
+   return -ENOMEM;
+
+   atomic_set(>iin_data_valid, 0);
+
+   rv = usb_control_msg(data->usb_dev,
+   usb_rcvctrlpipe(data->usb_dev, 0),
+   USBTMC488_REQUEST_READ_STATUS_BYTE,
+   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+   data->iin_bTag,
+   data->ifnum,
+   buffer, 0x03, USBTMC_TIMEOUT);
+
+   if (rv < 0) {
+   dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+   goto exit;
+   }
+
+   if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+   dev_err(dev, "control status returned %x\n", buffer[0]);
+   rv = -EIO;
+   goto exit;
+   }
+
+   if (data->iin_ep_present) {
+   rv = wait_event_interruptible_timeout(
+   data->waitq,
+   atomic_read(>iin_data_valid) != 0,
+   USBTMC_TIMEOUT);
+
+   if (rv < 0) {
+   dev_dbg(dev, "wait interrupted %d\n", rv);
+   goto exit;
+   }
+
+   if (rv == 0) {
+   dev_dbg(dev, "wait timed out\n");
+   rv = -ETIME;
+   goto exit;
+   }
+
+   tag = data->bNotify1 & 0x7f;
+
+   if (tag != data->iin_bTag) {
+   dev_err(dev, "expected bTag %x got %x\n",
+   data->iin_bTag, tag);
+   }
+
+   stb = data->bNotify2;
+   } else {
+   stb = buffer[2];
+   }
+
+   rv = copy_to_user((void __user *)arg, , sizeof(stb));
+   if (rv)
+   rv = -EFAULT;
+
+ exit:
+   /* bump interrupt bTag */
+   data->iin_bTag += 1;
+   if (data->iin_bTag > 127)
+   /* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/
+   data->iin_bTag = 2;
+
+   kfree(buffer);
+   return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1163,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+   case USBTMC488_IOCTL_READ_STB:
+   retval = usbtmc488_ioctl_read_stb(data, arg);
+   break;
}
 
 skip_io_on_zombie:
@@ -1092,6 +1190,66 @@ static struct usb_class_driver usbtmc_class = {
.minor_base =   USBTMC_MINOR_BASE,
 };
 
+static void usbtmc_interrupt(struct urb *urb)
+{
+   struct

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

2015-11-18 Thread Andy Shevchenko 
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler  wrote:
> Background:
> When performing a read on an instrument that is executing a function
> that runs longer than the USB timeout the instrument may hang and
> require a device reset to recover. The READ_STATUS_BYTE operation
> always returns even when the instrument is busy permitting to poll
> for the appropriate condition. This capability is referred to in
> instrument application notes on synchronizing acquisitions for other
> platforms.
>

First of all, please be patient and do not send patches immediately
when you answered to someone's review. It increases redundant noise
and burden on reviewer. Wait at least for 24h especially if there are
topics still to discuss.

[]

> +static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
> +   unsigned long arg)
> +{
> +   u8 *buffer;
> +   struct device *dev = >intf->dev;
> +   int rv;
> +   u8 tag, stb;

If you rearrange this like

+   struct device *dev = >intf->dev;
+   u8 *buffer;
+   u8 tag, stb;
+   int rv;

it will look better. Similar to check in all your patches.

> +
> +   dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> +   data->iin_ep_present);
> +
> +   buffer = kmalloc(8, GFP_KERNEL);
> +   if (!buffer)
> +   return -ENOMEM;
> +
> +   atomic_set(>iin_data_valid, 0);
> +
> +   rv = usb_control_msg(data->usb_dev,
> +   usb_rcvctrlpipe(data->usb_dev, 0),
> +   USBTMC488_REQUEST_READ_STATUS_BYTE,
> +   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +   data->iin_bTag,
> +   data->ifnum,
> +   buffer, 0x03, USBTMC_TIMEOUT);
> +

Redundant empty line.

> +   if (rv < 0) {
> +   dev_err(dev, "stb usb_control_msg returned %d\n", rv);
> +   goto exit;
> +   }
> +
> +   if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> +   dev_err(dev, "control status returned %x\n", buffer[0]);
> +   rv = -EIO;
> +   goto exit;
> +   }
> +
> +   if (data->iin_ep_present) {
> +   rv = wait_event_interruptible_timeout(
> +   data->waitq,
> +   atomic_read(>iin_data_valid) != 0,
> +   USBTMC_TIMEOUT);
> +

Ditto.

> +   if (rv < 0) {
> +   dev_dbg(dev, "wait interrupted %d\n", rv);
> +   goto exit;
> +   }
> +
> +   if (rv == 0) {
> +   dev_dbg(dev, "wait timed out\n");
> +   rv = -ETIME;
> +   goto exit;
> +   }
> +
> +   tag = data->bNotify1 & 0x7f;
> +

You may remove empty line, though it's minor nitpick here.

> +   if (tag != data->iin_bTag) {
> +   dev_err(dev, "expected bTag %x got %x\n",
> +   data->iin_bTag, tag);
> +   }
> +
> +   stb = data->bNotify2;
> +   } else {
> +   stb = buffer[2];
> +   }
> +
> +   rv = copy_to_user((void __user *)arg, , sizeof(stb));
> +   if (rv)
> +   rv = -EFAULT;
> +
> + exit:
> +   /* bump interrupt bTag */
> +   data->iin_bTag += 1;
> +   if (data->iin_bTag > 127)
> +   /* 1 is for SRQ see USBTMC-USB488 subclass specification 
> 4.3.1*/

Missed space before asterisk.

> +   data->iin_bTag = 2;
> +
> +   kfree(buffer);
> +   return rv;
> +}

[]

> +static void usbtmc_interrupt(struct urb *urb)
> +{
> +   struct usbtmc_device_data *data = urb->context;
> +   int status = urb->status;
> +   int rv;
> +
> +   dev_dbg(>intf->dev, "int status: %d len %d\n",
> +   status, urb->actual_length);
> +
> +   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.

> +   data->bNotify1 = data->iin_buffer[0];
> +   data->bNotify2 = data->iin_buffer[1];
> +   atomic_set(>iin_data_valid, 1);
> +   wake_up_interruptible(>waitq);
> +   goto exit;
> +   }
> +   }
> +   dev_warn(>intf->dev, "invalid notification: %x\n",
> +   data->iin_buffer[0]);
> +   break;
> +   case -EOVERFLOW:
> +   dev_err(>intf->dev,
> +   "%s - overflow

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

2015-11-18 Thread Dave Penkler 
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.

Signed-off-by: Dave Penkler 
---
 drivers/usb/class/usbtmc.c   | 212 +++
 include/uapi/linux/usb/tmc.h |   2 +
 2 files changed, 214 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..60a71cb 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read;  /* needed for abort */
 
+   /* data for interrupt in endpoint handling */
+   u8 bNotify1;
+   u8 bNotify2;
+   u16ifnum;
+   u8 iin_bTag;
+   u8*iin_buffer;
+   atomic_t   iin_data_valid;
+   unsigned int   iin_ep;
+   intiin_ep_present;
+   intiin_interval;
+   struct urb*iin_urb;
+   u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
 
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities  capabilities;
struct kref kref;
struct mutex io_mutex;  /* only one i/o function running at a time */
+   wait_queue_head_t waitq;
 };
 #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
 
@@ -373,6 +387,86 @@ exit:
return rv;
 }
 
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+   unsigned long arg)
+{
+   u8 *buffer;
+   struct device *dev = >intf->dev;
+   int rv;
+   u8 tag, stb;
+
+   dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+   data->iin_ep_present);
+
+   buffer = kmalloc(8, GFP_KERNEL);
+   if (!buffer)
+   return -ENOMEM;
+
+   atomic_set(>iin_data_valid, 0);
+
+   rv = usb_control_msg(data->usb_dev,
+   usb_rcvctrlpipe(data->usb_dev, 0),
+   USBTMC488_REQUEST_READ_STATUS_BYTE,
+   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+   data->iin_bTag,
+   data->ifnum,
+   buffer, 0x03, USBTMC_TIMEOUT);
+
+   if (rv < 0) {
+   dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+   goto exit;
+   }
+
+   if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+   dev_err(dev, "control status returned %x\n", buffer[0]);
+   rv = -EIO;
+   goto exit;
+   }
+
+   if (data->iin_ep_present) {
+   rv = wait_event_interruptible_timeout(
+   data->waitq,
+   atomic_read(>iin_data_valid) != 0,
+   USBTMC_TIMEOUT);
+
+   if (rv < 0) {
+   dev_dbg(dev, "wait interrupted %d\n", rv);
+   goto exit;
+   }
+
+   if (rv == 0) {
+   dev_dbg(dev, "wait timed out\n");
+   rv = -ETIME;
+   goto exit;
+   }
+
+   tag = data->bNotify1 & 0x7f;
+
+   if (tag != data->iin_bTag) {
+   dev_err(dev, "expected bTag %x got %x\n",
+   data->iin_bTag, tag);
+   }
+
+   stb = data->bNotify2;
+   } else {
+   stb = buffer[2];
+   }
+
+   rv = copy_to_user((void __user *)arg, , sizeof(stb));
+   if (rv)
+   rv = -EFAULT;
+
+ exit:
+   /* bump interrupt bTag */
+   data->iin_bTag += 1;
+   if (data->iin_bTag > 127)
+   /* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/
+   data->iin_bTag = 2;
+
+   kfree(buffer);
+   return rv;
+}
+
 /*
  * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
  * @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1163,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+   case USBTMC488_IOCTL_READ_STB:
+   retval = usbtmc488_ioctl_read_stb(data, arg);
+   break;
}
 
 skip_io_on_zombie:
@@ -1092,6 +1190,66 @@ static struct usb_class_driver usbtmc_class = {
.minor_base =   USBTMC_MINOR_BASE,
 };
 
+static void usbtmc_interrupt(struct urb *urb)
+{
+