Re: usbtmc: stb race condition introduced by 4f3c8d6eddc272b386464524235440a418ed2029

[guido@kiener-muenchen.de]

>> More info see:
>> https://github.com/GuidoKiener/linux-usbtmc/blob/master/README.md
>>
>> > My USBTMC device has an interrupt endpoint with a 1ms interval.
>> >
>> > 1) A query is sent to the USBTMC device.
>> >
>> > 2) An SRQ is reported via the interrupt endpoint with MAV set.
>> >
>> > 3) Userspace performs a read to get the reply since MAV is set after
>> > being notified by poll().
>>
>> Did you start reading (read()) without checking the Status Byte after
>> poll() event?
>
> I check the status byte after poll(). The problem seems to be that I
> also check the status byte in a loop (until there is nothing to
> service) before calling poll again.

This is not a problem. You can check the status byte several times
as long as you like. When RQS Bit (0x40) is set, then you know it was
an intermediate SRQ, sent according to chapter 3.4.1
(USBTMC_usb488_subclass_1_00.pdf). Otherwise it is a requested status
byte according to chapter 3.4.2.

> As long as you only check the
> status byte when there is a cached value available it should be no
> problem. However if you call USBTMC488_IOCTL_READ_STB when there is
> not a cached SRQ value an SRQ can occur after the lock is released in
> usbtmc488_ioctl_read_stb() and cache an older value (which will be
> read by the next USBTMC488_IOCTL_READ_STB) than the one returned.

Yes, interrupts (here SRQ) can happen at any time. Therefore the user
can enable/disable interrupts (e.g. with SCPI command SRE) and
postpone interrupt handling.

The SRQ sends a status byte with RQS bit set (chapter 3.4.1) and the
request initiated by the client returns a status byte without RQS bit
(chapter 3.4.2).

> This
> is a race condition and sounds broken to me but if this is the
> intended behavior I can adjust my userspace code to only do
> USBTMC488_IOCTL_READ_STB once after poll indicates an SRQ and live
> with it.
> It doesn't seem right for USBTMC488_IOCTL_READ_STB to ever report an
> older value than what was reported on the previous call.

I agree that this can result in an odd behavior (E.g. the MAV bit
is unset with reading the subsequent cached status byte).
I was not aware about this problem and I need to check whether this is
a common problem in VISA implementations or whether we just need to fix
the kernel (e.g. drop requested status byte and return older cached value).

A workaround to avoid this odd behavior is to read the status byte
again with USBTMC488_IOCTL_READ_STB when your user application detects
the RQS bit.

BTW when you look at the old implementation
https://elixir.bootlin.com/linux/v4.7.10/source/drivers/usb/class/usbtmc.c#L1332
then you can see that you will never get a status byte with RQS bit set.
The status byte was never stored in data->bNotify2

Regards,

Guido