Lockup for read on USB-DUX

[Jan-Matthias Braun]

Hi all,

thanks for all help so far. My latest Problem is triggered by a read() on a
comedi device representing an USB-DUX sampling with a given frequency but
without data in the queue. Actually my program is suspended as expected, but
never again woken up on the arrival of new data.

Attached are two patches that are each fixing the Problem and I would be
interested in an explanation on how to fix it best.

1. As the wake_up_interruptible call in comedi_fops.c comedi_event(...) is
never reached, because of
s->async->events == 0
if called from usbdux.c, the simplest approach is to call
wake_up_interruptible from usbdux.c in usbduxsub_ai_IsocIrq(...).

2. The other patch is setting the events bitfield in usbduxsub_ai_IsocIrq(...)
to |= COMEDI_CB_EOS and extends the cb_mask to account for this bit, if the
commands scan_begin_src field == TRIG_TIMER in comedi_fops.c
do_cmd_ioctl(...), regardless of other flags. Although I'm quite unsure if I
chose the correct flag and what side effects this change might have on other
drivers, the wake_up_interruptible is now reached if new data arrives.

With either solution my program returns safely from suspend an arrival of new
data, but how is it supposed to be done?

Cheers,
Jan

[Ian Abbott]

I think usbduxsub_ai_IsocIrq() just needs to set s->async->events |=
COMEDI_CB_BLOCK before it calls comedi_event(). That should cause
comedi_event() to wake up the reader.

--
-=( Ian Abbott @ MEV Ltd. E-mail: <abb...@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-

[Ian Abbott]

On 23/11/09 15:57, Ian Abbott wrote:
> On 20/11/09 13:15, Jan-Matthias Braun wrote:
>> Hi all,
>>
>> thanks for all help so far. My latest Problem is triggered by a read() on a
>> comedi device representing an USB-DUX sampling with a given frequency but
>> without data in the queue. Actually my program is suspended as expected, but
>> never again woken up on the arrival of new data.

> I think usbduxsub_ai_IsocIrq() just needs to set s->async->events |=
> COMEDI_CB_BLOCK before it calls comedi_event(). That should cause
> comedi_event() to wake up the reader.

On closer inspection, since usbduxsub_ai_IsocIrq() transfers a whole
scan at once, it should probably set s->async->events |= COMEDI_CB_BLOCK
| COMEDI_CB_EOS before the call to comedi_event(). Either way, the
COMEDI_CB_BLOCK event should ensure the reader is woken.

(This is the comedi_event() call at the end of usbduxsub_ai_IsocIrq() by
the way.)

[Jan-Matthias Braun]

On Monday 23. November 2009 17:18:15 Ian Abbott wrote:
> On 23/11/09 15:57, Ian Abbott wrote:
> > On 20/11/09 13:15, Jan-Matthias Braun wrote:
> >> thanks for all help so far. My latest Problem is triggered by a read()
> >> on a comedi device representing an USB-DUX sampling with a given
> >> frequency but without data in the queue. Actually my program is
> >> suspended as expected, but never again woken up on the arrival of new
> >> data.
> >
> > I think usbduxsub_ai_IsocIrq() just needs to set s->async->events |=
> > COMEDI_CB_BLOCK before it calls comedi_event(). That should cause
> > comedi_event() to wake up the reader.
>
> On closer inspection, since usbduxsub_ai_IsocIrq() transfers a whole
> scan at once, it should probably set s->async->events |= COMEDI_CB_BLOCK
> | COMEDI_CB_EOS before the call to comedi_event(). Either way, the
> COMEDI_CB_BLOCK event should ensure the reader is woken.

K, checked this and it works. But in comedi.h I found:
#define COMEDI_CB_BLOCK 4 /* DEPRECATED: convenient block size */

so, what is the best non deprecated way? The ..._EOS-Event is normally not
checked in comedi_event() and would thus still lead to an ever sleeping
userspace program. (See the second patch from my previous mail.)

Thanks for inspecting the issue!

Jan

[Ian Abbott]

TBH, I don't know why COMEDI_CB_BLOCK is marked deprecated, but lots of
drivers use it!

Setting both COMEDI_CB_BLOCK and COMEDI_CB_EOS (rather than just
COMEDI_CB_BLOCK) has no benefit for waking up reader tasks, but it may
be of benefit to kernel code using the kcomedilib functions that have
registered an event callback function via comedi_register_callback() in
kcomedilib/kcomedilib_main.c.

[Bernd Porr]

This has been pointed out earlier by my colleague Nick Bailey. I think
just now there is no common solution for that execpt of the depricated
define. The workaround is simply that you check the buffer contents
before you do your read (see source code of comedirecord).

Ian (and Frank and Dave and...), shouldn't here be by default no
blocking behaviour of the read command? I mean if *some* drivers have
COMEDI_CB_BLOCK and some not it causes a bit of an inconsistency. Or to
reprase the question: does it make sense to force the user to wait in
any reasonable situation?

/Bernd

www: http://www.berndporr.me.uk/
http://www.linux-usb-daq.co.uk/
Mobile: +44 (0)7840 340069
Work: +44 (0)141 330 5237
University of Glasgow
Department of Electronics & Electrical Engineering
72 Oakfield Avenue (for deliveries: Rankine Building)
Glasgow, G12 8LT

[Frank Mori Hess]

On Monday 23 November 2009, Bernd Porr wrote:
> Ian (and Frank and Dave and...), shouldn't here be by default no
> blocking behaviour of the read command? I mean if *some* drivers have
> COMEDI_CB_BLOCK and some not it causes a bit of an inconsistency. Or to
> reprase the question: does it make sense to force the user to wait in
> any reasonable situation?

I'm not sure what you're suggesting. You want to drop support for blocking
read() (and by extension fread())? What is wrong with supporting it?
BTW, just to rule out any possible confusion the "block" in
COMEDI_CB_BLOCK refers to an arbitrary sized block of data, not blocking
reads/writes. I don't know why dave marked COMEDI_CB_BLOCK as deprecated
in the header.

[Bernd Porr]

Ah. So, that makes things clearer. Forget my suggestion.

/Bernd

[Ian Abbott]

Hi Bernd,

On 24/11/09 09:49, Bernd Porr wrote:
> Ah. So, that makes things clearer. Forget my suggestion.
>
> /Bernd

So getting back to the original problem, does the attached patch look
reasonable for the CVS version?

(I'll submit "staging" patches in a few days - when I find time to
update my git repositories!)

[Bernd Porr]

Hi Ian,

looks OK but didn't have a chance to check it but will do today or
tomorrow. And there's also the USBDUXfast which might also benefit from
another flag in the interrupt routine.

/Bernd

Ian Abbott wrote:
> Hi Bernd,
>
> On 24/11/09 09:49, Bernd Porr wrote:
>> Ah. So, that makes things clearer. Forget my suggestion.
>>
>> /Bernd
>
> So getting back to the original problem, does the attached patch look
> reasonable for the CVS version?
>
> (I'll submit "staging" patches in a few days - when I find time to
> update my git repositories!)
>
>

--

[Ian Abbott]

On 26/11/09 17:32, Bernd Porr wrote:
> Hi Ian,
>
> looks OK but didn't have a chance to check it but will do today or
> tomorrow. And there's also the USBDUXfast which might also benefit from
> another flag in the interrupt routine.
>
> /Bernd

Hi Bernd,

For USBDUXfast, the cfc_write_array_to_buffer() call already takes care
of setting the COMEDI_CB_BLOCK and COMEDI_CB_EOS event flags, leaving it
up to the driver to call comedi_event() afterwards.

[Bernd Porr]

Hi!

I just learned that while grepping for COMEDI_CB_BLOCK. ;) thanks for
letting me know anyway. I get back to you later.

/Bernd

Ian Abbott wrote:
> On 26/11/09 17:32, Bernd Porr wrote:
>> Hi Ian,
>>
>> looks OK but didn't have a chance to check it but will do today or
>> tomorrow. And there's also the USBDUXfast which might also benefit from
>> another flag in the interrupt routine.
>>
>> /Bernd
>
> Hi Bernd,
>
> For USBDUXfast, the cfc_write_array_to_buffer() call already takes care
> of setting the COMEDI_CB_BLOCK and COMEDI_CB_EOS event flags, leaving it
> up to the driver to call comedi_event() afterwards.
>

--

[Bernd Porr]

And because we are patching happily away can we remove that word
"deprecated" from the comedi CVS for COMEDI_CB_BLOCK and add perhaps a
meaningful comment for it. I can do it.

/Bernd

Ian Abbott wrote:
> On 26/11/09 17:32, Bernd Porr wrote:
>> Hi Ian,
>>
>> looks OK but didn't have a chance to check it but will do today or
>> tomorrow. And there's also the USBDUXfast which might also benefit from
>> another flag in the interrupt routine.
>>
>> /Bernd
>
> Hi Bernd,
>
> For USBDUXfast, the cfc_write_array_to_buffer() call already takes care
> of setting the COMEDI_CB_BLOCK and COMEDI_CB_EOS event flags, leaving it
> up to the driver to call comedi_event() afterwards.
>

--

[Ian Abbott]

On 26/11/09 17:48, Bernd Porr wrote:
> And because we are patching happily away can we remove that word
> "deprecated" from the comedi CVS for COMEDI_CB_BLOCK and add perhaps a
> meaningful comment for it. I can do it.

I don't see why not, since none of us seem to know why it was marked
deprecated in the first place!

[Bernd Porr]

Ian,

are you sending these patches to Greg as part of a bigger package of
patches or should I do it?

/Bernd

www: http://www.berndporr.me.uk/
http://www.linux-usb-daq.co.uk/

http://www.imdb.com/name/nm3293421/

Mobile: +44 (0)7840 340069
Work: +44 (0)141 330 5237
University of Glasgow
Department of Electronics & Electrical Engineering

Room 519, Rankine Building, Oakfield Avenue,
Glasgow, G12 8LT

[Bernd Porr]

Jan-Matthias,

I've committed these changes to the CVS. Can you check, please.

/Bernd

www: http://www.berndporr.me.uk/
http://www.linux-usb-daq.co.uk/

http://www.imdb.com/name/nm3293421/

Mobile: +44 (0)7840 340069
Work: +44 (0)141 330 5237
University of Glasgow
Department of Electronics & Electrical Engineering

Room 519, Rankine Building, Oakfield Avenue,
Glasgow, G12 8LT


Ian Abbott wrote:

[Ian Abbott]

On 26/11/09 20:20, Bernd Porr wrote:
> Ian,
>
> are you sending these patches to Greg as part of a bigger package of
> patches or should I do it?
>
> /Bernd

I see you've sent the other patches to Greg, but I suspect he'll come
back with a request for one patch per email and addition of
'Signed-off-by:' lines. The official place to submit staging driver
patches is <de...@linuxdriverproject.org>.

The only other outstanding comedi change yet to be applied to the
staging tree that I know of is the jr3_pci ioremap changes, which I plan
to submit in the next few days.

[Bernd Porr]

Thanks. I'll submit that separately.

/Bernd

Ian Abbott wrote:
> On 26/11/09 20:20, Bernd Porr wrote:
>> Ian,
>>
>> are you sending these patches to Greg as part of a bigger package of
>> patches or should I do it?
>>
>> /Bernd
>
> I see you've sent the other patches to Greg, but I suspect he'll come
> back with a request for one patch per email and addition of
> 'Signed-off-by:' lines. The official place to submit staging driver
> patches is <de...@linuxdriverproject.org>.
>
> The only other outstanding comedi change yet to be applied to the
> staging tree that I know of is the jr3_pci ioremap changes, which I plan
> to submit in the next few days.
>

--

www: http://www.berndporr.me.uk/
http://www.linux-usb-daq.co.uk/

Mobile: +44 (0)7840 340069
Work: +44 (0)141 330 5237
University of Glasgow
Department of Electronics & Electrical Engineering

72 Oakfield Avenue (Rankine Building for deliveries)
Glasgow, G12 8LT

[Jan-Matthias Braun]

Hi!

So, will this get fixed? Because as long as O_NONBLOCK isn't set on file
descriptor I would expect a read to be blocking and thus I would consider the
current behaviour a bug.

Sincerely,
Jan-Matthias Braun

[Jan-Matthias Braun]

Hi Bernd!

On Thursday 26. November 2009 21:21:30 Bernd Porr wrote:
> I've committed these changes to the CVS. Can you check, please.

Works like a charm for me. Thanks for the efforts!

Cheers,
Jan