[18/39] firewire-lib: Add a fallback at RCODE_CANCELLED
diff mbox

Message ID 1393558072-25926-19-git-send-email-o-takashi@sakamocchi.jp
State Changes Requested
Delegated to: Takashi Iwai
Headers show

Commit Message

Takashi Sakamoto Feb. 28, 2014, 3:27 a.m. UTC
I confirm that some devices often send no response against driver's
request even if the devices handle the request. In this case, drivers
should not retry the request because it's not clear whether the devices
already handled an operation of the request or not. The second request
may bring errors or make no sence if the request was handled.

This patch adds a flag, FW_RETURN_TIMEOUT, for snd_fw_transaction().
With this flag, this function returns -ETIMEOUT when fw_run_transaction()
returns RCODE_CALCELLED.

For FCP, this patch modifies fcp_avc_transaction() with this flag. The
-ETIMEDOUT is handled as success. Linux Firewire Subsystem currently gives
200msec for default timeout (See DEFAULT_SPLIT_TIMEOUT in core-card.c).
firewire-lib gives 125msec for wait_event_timeout(). As a result, in worst
case that all of three retries are timeout and devices don't send responses,
fcp_avc_transaction() returns 1sec later ((200 + 125) msec * 3 times). But
this is rare.

For CMP, this patch modifies pcr_modify() with this flag. The -ETIMEOUT
generates another read transaction to check current value of PCR. If this
read transaction returns errors, then pcr_modify() returns the error code.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c | 23 +++++++++++++++++++++--
 sound/firewire/fcp.c |  5 +++--
 sound/firewire/lib.c |  6 +++++-
 sound/firewire/lib.h |  1 +
 4 files changed, 30 insertions(+), 5 deletions(-)

Comments

Stefan Richter Feb. 28, 2014, 8:25 p.m. UTC | #1
On Feb 28 Takashi Sakamoto wrote:
> Linux Firewire Subsystem currently gives
> 200msec for default timeout (See DEFAULT_SPLIT_TIMEOUT in core-card.c).

It's 2000 ms actually.

And this is the IEEE 1394 split transaction timeout of course.  FCP
transactions on the other hand are transported by means of two IEEE 1394
transactions:  One in which the FCP requester is IEEE 1394 requester, and
another one in which the FCP responder is IEEE 1394 requester.

firewire-core's split transaction timeout is only relevant to the request
subaction of an FCP transaction (i.e. the first one of the two IEEE 1394
transaction), and only if this one is being performed as a split
transaction rather than a unified transaction, which depends on the FCP
responder hardware.  (I am not aware of any hardware which handles
FCP request subactions as unified transactions.)

IEC 61883-1 does not specify any FCP transaction timeout.

The 1394 TA document "AV/C Digital Interface Command Set, Rev. 1.0,
1996" defines FCP transactions (for use with AV/C command set) much more
precisely than IEC 61883-1 does:

  - To each FCP request, there may be 0..n FCP responses.

  - An AV/C target may emit a final response (one with response code <
    0xf) up to 100 ms after reception of an FCP request.

  - Or the AV/C target may emit an intermediate response (one with
    response code 0xf for interim response) up to 100 ms after reception
    of an FCP request.  The target shall emit no additional interim
    response in this transaction, but one final response.  There is no
    timeout specified for this final-after-interim response, i.e. it may
    take an undefined, possibly very long time.  (The AV/C
    specification v4.0, 2001 adds that subunit specifications may define
    such timeouts for certain commands.)

  - Or the AV/C target may still be busy processing a previous command.
    In this case, the specification accepts that the target does not send
    any response to subsequent requests at all until it is done with the
    previous work.  The requester may the retry such a failed request after
    the mentioned 100 ms timeout.  (Plus IEEE 1394 transport overhead, but
    the AV/C specification v1.0 does not mention this.  AV/C v4.0 explains
    that the requester should take certain transport dependent delays
    into account in addition to the 100 ms FCP timeout, e.g. physical layer
    repeater delays, or busy retry delays.  Furthermore, AV/C v4.0
    recommends that a busy target returns certain acknowledge codes or
    response codes to pipelined requests that exceed its capacity
    to handle concurrent FCP transactions.)

  - An "IEEE 1394.0 reset" (sic) terminates any outstanding FCP transaction.
    (I suppose a 1394 bus reset is meant by this.  AV/C v4.0 clarifies this
    to be an IEEE 1394 bus reset indeed.)

From a quick glance, the FCP transaction specification looks unchanged
from AV/C 4.0, 2001 to v4.1, 2001 and to v4.2, 2004.
Stefan Richter Feb. 28, 2014, 8:39 p.m. UTC | #2
On Feb 28 Stefan Richter wrote:
>   - Or the AV/C target may still be busy processing a previous command.
>     In this case, the specification accepts that the target does not send
>     any response to subsequent requests at all until it is done with the
>     previous work.

To be more precise:  The specification accepts that the target *ignores*
incoming AV/C command frames while processing an AV/C command.  This is
indicated to the requester by lack of a response to any ignored request.

To me, this implies that the target must send a response to any request
that it did not ignore, i.e. to any command that it executed.  On the
other hand, I don't think there is any rollback behavior specified or
practical in cases when the target is unable to deliver a response to the
initiator.
Takashi Sakamoto March 1, 2014, 3:18 a.m. UTC | #3
Stefan,

At first, I introduce this is for a patch below. You can see actual logs:
[alsa-devel] [PATCH 39/39] bebob: Add support for M-Audio special 
Firewire series
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-February/073484.html

> It's 2000 ms actually.

Oops. Here I made a mistake to refer to unrelated value here. I might 
consider about the other thing when writing this commit, perhaps because 
of noon in Friday and a bit tired...

Well, yes. It's 2000 msec.

(Feb 28 2014 12:27), Takashi Sakamoto wrote:
 > As a result, in worst case that all of three retries are timeout
 > and devices don't send responses, fcp_avc_transaction() returns 1sec
 > later ((200 + 125) msec * 3 times).

So in this case, this function returns 6,375msec later. (but it's rare, 
again.)

> And this is the IEEE 1394 split transaction timeout of course.  FCP
> transactions on the other hand are transported by means of two IEEE 1394
> transactions:  One in which the FCP requester is IEEE 1394 requester, and
> another one in which the FCP responder is IEEE 1394 requester.
>
> firewire-core's split transaction timeout is only relevant to the request
> subaction of an FCP transaction (i.e. the first one of the two IEEE 1394
> transaction),

OK.

> and only if this one is being performed as a split
> transaction rather than a unified transaction, which depends on the FCP
> responder hardware.  (I am not aware of any hardware which handles
> FCP request subactions as unified transactions.)

As You explained in this message:
[alsa-devel] Echo Fireworks control protocol (was Re: Sample program for 
hwdep interface)
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-February/071978.html

> IEC 61883-1 does not specify any FCP transaction timeout.

I've confirmed it.

> The 1394 TA document "AV/C Digital Interface Command Set, Rev. 1.0,
> 1996" defines FCP transactions (for use with AV/C command set) much more
> precisely than IEC 61883-1 does:
>
>    - To each FCP request, there may be 0..n FCP responses.
>
>    - An AV/C target may emit a final response (one with response code <
>      0xf) up to 100 ms after reception of an FCP request.
>
>    - Or the AV/C target may emit an intermediate response (one with
>      response code 0xf for interim response) up to 100 ms after reception
>      of an FCP request.  The target shall emit no additional interim
>      response in this transaction, but one final response.  There is no
>      timeout specified for this final-after-interim response, i.e. it may
>      take an undefined, possibly very long time.  (The AV/C
>      specification v4.0, 2001 adds that subunit specifications may define
>      such timeouts for certain commands.)
>
>    - Or the AV/C target may still be busy processing a previous command.
>      In this case, the specification accepts that the target does not send
>      any response to subsequent requests at all until it is done with the
>      previous work.  The requester may the retry such a failed request after
>      the mentioned 100 ms timeout.  (Plus IEEE 1394 transport overhead, but
>      the AV/C specification v1.0 does not mention this.  AV/C v4.0 explains
>      that the requester should take certain transport dependent delays
>      into account in addition to the 100 ms FCP timeout, e.g. physical layer
>      repeater delays, or busy retry delays.  Furthermore, AV/C v4.0
>      recommends that a busy target returns certain acknowledge codes or
>      response codes to pipelined requests that exceed its capacity
>      to handle concurrent FCP transactions.)
>
>    - An "IEEE 1394.0 reset" (sic) terminates any outstanding FCP transaction.
>      (I suppose a 1394 bus reset is meant by this.  AV/C v4.0 clarifies this
>      to be an IEEE 1394 bus reset indeed.)

Additionally, there are two types of AV/C transactions, 'immediate 
transaction' and 'deffered transaction'. Current 'firewire-lib' don't 
implement 'deffered transaction' so fcp_avc_transaction() always returns 
first AV/C response frame. This will be response=INTERIM against 
ctype=STATUS/NOTIFY.  The gap between a first response and a second 
response of 'deffered transaction' is defined as 'unspecified interval'.

But this is not related to an issue about which this patch mentions.

(Mar 01 2014 05:39), Stefan Richter wrote:
> To be more precise:  The specification accepts that the target*ignores*
> incoming AV/C command frames while processing an AV/C command.  This is
> indicated to the requester by lack of a response to any ignored request.
>
> To me, this implies that the target must send a response to any request
> that it did not ignore, i.e. to any command that it executed.  On the
> other hand, I don't think there is any rollback behavior specified or
> practical in cases when the target is unable to deliver a response to the
> initiator.
 >
>>From a quick glance, the FCP transaction specification looks unchanged
> from AV/C 4.0, 2001 to v4.1, 2001 and to v4.2, 2004.

I cannot find some sections about such ignorance in 'AV/C Digital 
Interface Command Set General Specification Version 4.2 (Sep 01, 2004, 
1394TA)' and 'IEC 61883-1 ed2'.

Can I request you any pointers about this ignorance?


Thanks

Takashi Sakamoto
o-takashi@sakamocchi.jp
Stefan Richter March 1, 2014, 10:10 a.m. UTC | #4
On Mar 01 Takashi Sakamoto wrote:
> Stefan,
> 
> At first, I introduce this is for a patch below. You can see actual logs:
> [alsa-devel] [PATCH 39/39] bebob: Add support for M-Audio special 
> Firewire series
> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-February/073484.html

Right, I saw it (and your note that the iso resource management related
transactions still need a similar workaround).

> > It's 2000 ms actually.
> 
> Oops. Here I made a mistake to refer to unrelated value here. I might 
> consider about the other thing when writing this commit, perhaps because 
> of noon in Friday and a bit tired...
> 
> Well, yes. It's 2000 msec.
> 
> (Feb 28 2014 12:27), Takashi Sakamoto wrote:
>  > As a result, in worst case that all of three retries are timeout
>  > and devices don't send responses, fcp_avc_transaction() returns 1sec
>  > later ((200 + 125) msec * 3 times).
> 
> So in this case, this function returns 6,375msec later. (but it's rare, 
> again.)

This 125 ms part of the calculation made me wonder what the split
transaction timeout of FCP transactions is (I didn't remember), so I
looked it up and wrote down what I found.  So, all of the rest of my
reply was not really a direct comment to your patch but merely a loosely
related side note.

> > And this is the IEEE 1394 split transaction timeout of course.  FCP
> > transactions on the other hand are transported by means of two IEEE 1394
> > transactions:  One in which the FCP requester is IEEE 1394 requester, and
> > another one in which the FCP responder is IEEE 1394 requester.
> >
> > firewire-core's split transaction timeout is only relevant to the request
> > subaction of an FCP transaction (i.e. the first one of the two IEEE 1394
> > transaction),
> 
> OK.

(Per IEEE 1394 specification, it should in fact also be the upper bound of
our own 1394 response timing, e.g. when we respond to FCP response frame
reception.  But we haven't implemented such a time limit for outbound 1394
response packets yet.)

> > and only if this one is being performed as a split
> > transaction rather than a unified transaction, which depends on the FCP
> > responder hardware.  (I am not aware of any hardware which handles
> > FCP request subactions as unified transactions.)
> 
> As You explained in this message:
> [alsa-devel] Echo Fireworks control protocol (was Re: Sample program for 
> hwdep interface)
> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-February/071978.html
> 
> > IEC 61883-1 does not specify any FCP transaction timeout.
> 
> I've confirmed it.
> 
> > The 1394 TA document "AV/C Digital Interface Command Set, Rev. 1.0,
> > 1996" defines FCP transactions (for use with AV/C command set) much more
> > precisely than IEC 61883-1 does:
> >
> >    - To each FCP request, there may be 0..n FCP responses.

Together with the rest of the spec, it's 0..2 FCP responses, precisely.

> >    - An AV/C target may emit a final response (one with response code <
> >      0xf) up to 100 ms after reception of an FCP request.
> >
> >    - Or the AV/C target may emit an intermediate response (one with
> >      response code 0xf for interim response) up to 100 ms after reception
> >      of an FCP request.  The target shall emit no additional interim
> >      response in this transaction, but one final response.  There is no
> >      timeout specified for this final-after-interim response, i.e. it may
> >      take an undefined, possibly very long time.  (The AV/C
> >      specification v4.0, 2001 adds that subunit specifications may define
> >      such timeouts for certain commands.)
> >
> >    - Or the AV/C target may still be busy processing a previous command.
> >      In this case, the specification accepts that the target does not send
> >      any response to subsequent requests at all until it is done with the
> >      previous work.  The requester may the retry such a failed request after
> >      the mentioned 100 ms timeout.  (Plus IEEE 1394 transport overhead, but
> >      the AV/C specification v1.0 does not mention this.  AV/C v4.0 explains
> >      that the requester should take certain transport dependent delays
> >      into account in addition to the 100 ms FCP timeout, e.g. physical layer
> >      repeater delays, or busy retry delays.  Furthermore, AV/C v4.0
> >      recommends that a busy target returns certain acknowledge codes or
> >      response codes to pipelined requests that exceed its capacity
> >      to handle concurrent FCP transactions.)
> >
> >    - An "IEEE 1394.0 reset" (sic) terminates any outstanding FCP transaction.
> >      (I suppose a 1394 bus reset is meant by this.  AV/C v4.0 clarifies this
> >      to be an IEEE 1394 bus reset indeed.)
> 
> Additionally, there are two types of AV/C transactions, 'immediate 
> transaction' and 'deffered transaction'.

These are the first two cases in my list above.  Put into other words, here
are all possible cases of 0, 1, or 2 responses to one and the same request:

Cases with 0 responses:
  a) The target ignored a command with reserved ctype value.
  b) The target ignored a command that it received before it finished
     processing a prior command.
  c) A bus reset happened before the target had a chance to send the first
     response.  The FCP transaction specification part of the AV/C spec
     does not say what kind of effects such an aborted transaction may
     have.  Perhaps some of the various command related definitions specify
     this, I haven't looked.
  d) The specification of a particular command defines that handling of
     this command shall (or may) not be followed up by a response.  I
     haven't looked whether such commands actually exist.
Cases with 1 response:
  e) The target sends a final response as first response.
  f) The target sent an interim response as first response, but a bus reset
     happened before it could send the final response.
Cases with 2 responses:
  g) The target sends an interim response, and then a final response.  This
     is only possible with CONTROL and NOTIFY commands.

> Current 'firewire-lib' don't 
> implement 'deffered transaction' so fcp_avc_transaction() always returns 
> first AV/C response frame. This will be response=INTERIM against 
> ctype=STATUS/NOTIFY.

Correction:  CONTROL or NOTIFY.  (In contrast, STATUS or SPECIFIC INQUIRY
or GENERAL INQUIRY commands can only be performed in immediate
transactions.  See AV/C v4.2 table 19.  Likewise, already AV/C v1.0 allows
INTERIM response only to CONTROL or NOTIFY commands.)

> The gap between a first response and a second 
> response of 'deffered transaction' is defined as 'unspecified interval'.

I wonder:  Now that firewire-lib's use cases are expanded far beyond LaCie
Speakers and Griffin FireWave, do we need to implement deferred
transactions in firewire-lib too?

As far as I can tell, FFADO simply ignores INTERIM responses, and treats
deferred transactions which are completed within 200...400 ms the same way
as if it was an immediate transaction.  (This is 200 ms from
IEEE1394SERVICE_FCP_RESPONSE_TIMEOUT_USEC and 400 ms from
IEEE1394SERVICE_FCP_POLL_TIMEOUT_MSEC in the FFADO code.)

(By the way, there is another FCP transaction layer implementation
existing in the kernel:  drivers/media/firewire/firedtv-avc.c.  This one
does in fact implement deferred transactions, but only for one particular
command, SFE_VENDOR_OPCODE_REGISTER_REMOTE_CONTROL.)

A related question:  Since FFADO applies 200 ms or more as FCP transaction
timeout, shouldn't firewire-lib's fcp.c increase FCP_TIMEOUT_MS from 125
to 200 or more as well?

> But this is not related to an issue about which this patch mentions.

Right.

> (Mar 01 2014 05:39), Stefan Richter wrote:
> > To be more precise:  The specification accepts that the target*ignores*
> > incoming AV/C command frames while processing an AV/C command.  This is
> > indicated to the requester by lack of a response to any ignored request.
> >
> > To me, this implies that the target must send a response to any request
> > that it did not ignore, i.e. to any command that it executed.  On the
> > other hand, I don't think there is any rollback behavior specified or
> > practical in cases when the target is unable to deliver a response to the
> > initiator.
>  >
> >>From a quick glance, the FCP transaction specification looks unchanged
> > from AV/C 4.0, 2001 to v4.1, 2001 and to v4.2, 2004.
> 
> I cannot find some sections about such ignorance in 'AV/C Digital 
> Interface Command Set General Specification Version 4.2 (Sep 01, 2004, 
> 1394TA)' and 'IEC 61883-1 ed2'.
> 
> Can I request you any pointers about this ignorance?

This is about case b) and, now that I think about it, also about case a)
in my list further above.

AV/C v1.0 says about case a)...

    If the AV/C command frame contains a reserved value in the ctype
    field, the target shall ignore the command and shall not generate a
    response frame.

...and about case b):

    If the target is already occupied with a previous command, it may
    ignore any AV/C command frames received. Note that the receipt of an
    AV/C command frame shall always be acknowledged by a target. The
    target ignores a command frame by a failure to return a response frame.
    [...]
    If the target is not occupied with a previous command, it shall create
    an AV/C response frame [...]

AV/C v4.2 says about case a)...

    If the AV/C command frame contains a reserved value in the ctype
    field, the target shall ignore the command and shall not generate a
    response frame.

...and about case b):

    Before sending a first response to a command, the number of AV/C
    commands that a target can process is implementation dependent.  If the
    target cannot process an additional command, it should return a 1394
    ack_busy, ack_conflict_error, or resp_conflict_error to the additional
    command.
    NOTE – Some targets that are compliant to the previous version of this
    specification may ignore the additional command in the above case.
Stefan Richter March 1, 2014, 10:34 a.m. UTC | #5
On Feb 28 Takashi Sakamoto wrote:
> --- a/sound/firewire/lib.c
> +++ b/sound/firewire/lib.c
> @@ -22,7 +22,7 @@
>   * @length: length of @buffer
>   * @flags: use %FW_FIXED_GENERATION and add the generation value to attempt the
>   *         request only in that generation; use %FW_QUIET to suppress error
> - *         messages
> + *         messages: use %FW_RETURN_TIMEOUT to avoid retry at RCODE_CANCELLED

For clarity, this should be a semicolon instead of a colon, i.e.

              messages; use %FW_RETURN_TIMEOUT to avoid retry at RCODE_CANCELLED
Takashi Sakamoto March 1, 2014, 12:22 p.m. UTC | #6
Hi Stefan,

 > This 125 ms part of the calculation made me wonder what the split
 > transaction timeout of FCP transactions is (I didn't remember), so I
 > looked it up and wrote down what I found.  So, all of the rest of my
 > reply was not really a direct comment to your patch but merely a loosely
 > related side note.

I think it's not a proper action in this ML...

My intent of this comment is for a notice about time till returining 
from fcp_avc_transaction().
It might be much time against developer's expectation.


 > Correction:  CONTROL or NOTIFY.  (In contrast, STATUS or SPECIFIC INQUIRY
 > or GENERAL INQUIRY commands can only be performed in immediate
 > transactions.  See AV/C v4.2 table 19.  Likewise, already AV/C v1.0 
allows
 > INTERIM response only to CONTROL or NOTIFY commands.)

I confirm my mistake, thanks for your correction ;)


 > I wonder:  Now that firewire-lib's use cases are expanded far beyond 
LaCie
 > Speakers and Griffin FireWave, do we need to implement deferred
 > transactions in firewire-lib too?

Yes. I confirmed some devices use deffered transaction for AV/C CONTROL 
request.
But I still pending this issue, because of the way to handle the 
'unspecified interval'.

I plan to solve this issue for my future work.
This series of patch is the most I can do, now.


 > A related question:  Since FFADO applies 200 ms or more as FCP 
transaction
 > timeout, shouldn't firewire-lib's fcp.c increase FCP_TIMEOUT_MS from 125
 > to 200 or more as well?

For this developing, I've spent much time with my test devices.
But I've never experienced disadvantages under FCP_TIMEOUT_MS=125msec.
So feel no importance.

If you feel this importance, please post your patch with proper reasons.


 > This is about case b) and, now that I think about it, also about case a)
 > in my list further above.

The case a) is for ctype=reserved. Current ALSA Firewire drivers don't 
utilize it.
So I have never considered about this case.

For the case b), I'm optimistic because current fcp_avc_transaction() 
don't return
till the first AV/C response comes (or AV/C request is error). This 
means that
the driver basically don't send any AV/C transaction at the same time.

Furthermore, my drivers basically generate some AV/C transaction in driver's
probe/remove state or starting/stopping playback/capture of PCM samples/MIDI
messages. So I believe that FFADO/ALSA rarely transfer some AV/C 
transactions
at the same time.

In these reasons, I'm optimistic for device's ignorance of AV/C request.

Anyway, I appreciate your advices. Thank you to spend time for my 
developing.


Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Sakamoto March 1, 2014, 12:23 p.m. UTC | #7
(Mar 02 2014 19:34), Stefan Richter wrote:
>> - *         messages
>> + *         messages: use %FW_RETURN_TIMEOUT to avoid retry at RCODE_CANCELLED
>
> For clarity, this should be a semicolon instead of a colon, i.e.
>
>                messages; use %FW_RETURN_TIMEOUT to avoid retry at RCODE_CANCELLED

Exactly. Thank you.


Takashi Sakamoto
o-takashi@sakamocchi.jp
Stefan Richter March 1, 2014, 2:20 p.m. UTC | #8
On Mar 01 Takashi Sakamoto wrote:
[...]
>> I wonder:  Now that firewire-lib's use cases are expanded far
>> beyond LaCie Speakers and Griffin FireWave, do we need to implement
>> deferred transactions in firewire-lib too?
> 
> Yes. I confirmed some devices use deffered transaction for AV/C 
> CONTROL request.
> But I still pending this issue, because of the way to handle the 
> 'unspecified interval'.

Are these devices already supported or reported to work with FFADO?
If yes, then the interval is probably typically less than FFADO's built-in
FCP transaction timeout, i.e. 200 (or 400?) ms.

> I plan to solve this issue for my future work.

OK.

>> A related question:  Since FFADO applies 200 ms or more as FCP
>> transaction timeout, shouldn't firewire-lib's fcp.c increase
>> FCP_TIMEOUT_MS from 125 to 200 or more as well?
> 
> For this developing, I've spent much time with my test devices.
> But I've never experienced disadvantages under FCP_TIMEOUT_MS=125msec.
> So feel no importance.
> 
> If you feel this importance, please post your patch with proper reasons.

This is mostly a question to the ffado-devel subscribers.  125 ms is of
course enough for devices which comply with the specification in this
regard.  The question is whether FFADO developers know of devices (or
suspect devices) which exceed the standard 100 ms and need more like 200
ms.

In any case, this question is unrelated to your patch (apart from the
unlikely worst-case timeout which you noted in the changelog).

>> This is about case b) and, now that I think about it, also about case
>> a) in my list further above.
> 
> The case a) is for ctype=reserved. Current ALSA Firewire drivers don't 
> utilize it.
> So I have never considered about this case.

Of course; the kernel is never going to emit a reserved ctype.  And
if malicious or faulty userspace does, the specified target behavior is
exactly the desired behavior.

> For the case b), I'm optimistic because current fcp_avc_transaction() 
> don't return till the first AV/C response comes (or AV/C request is
> error). This means that the driver basically don't send any AV/C
> transaction at the same time.
> 
> Furthermore, my drivers basically generate some AV/C transaction in
> driver's probe/remove state or starting/stopping playback/capture of
> PCM samples/MIDI messages. So I believe that FFADO/ALSA rarely transfer
> some AV/C transactions at the same time.
> 
> In these reasons, I'm optimistic for device's ignorance of AV/C request.

From what I understood so far, I agree.  (And besides, the specified
behavior of ignoring the command and not sending a response is of course
better than, for example, execution of a command but failure to send
the response.)
Jonathan Woithe March 4, 2014, 1:35 a.m. UTC | #9
On Sat, Mar 01, 2014 at 03:20:50PM +0100, Stefan Richter wrote:
> On Mar 01 Takashi Sakamoto wrote:
> >> A related question:  Since FFADO applies 200 ms or more as FCP
> >> transaction timeout, shouldn't firewire-lib's fcp.c increase
> >> FCP_TIMEOUT_MS from 125 to 200 or more as well?
> > 
> > For this developing, I've spent much time with my test devices.
> > But I've never experienced disadvantages under FCP_TIMEOUT_MS=125msec.
> > So feel no importance.
> > 
> > If you feel this importance, please post your patch with proper reasons.
> 
> This is mostly a question to the ffado-devel subscribers.  125 ms is of
> course enough for devices which comply with the specification in this
> regard.  The question is whether FFADO developers know of devices (or
> suspect devices) which exceed the standard 100 ms and need more like 200
> ms.

I've personally had nothing to do with devices utilising FCP transactions so
unfortunately I don't really know.  My feeling is that the authors of the
respective FFADO drivers would not have applied an FCP timeout of 200 ms if
there was no demonstrated need for it.  Therefore in the absence of other
evidence I would be assuming that there are devices which require the higher
timeout allowed for in FFADO's streaming code.  However, I don't know which
specific devices these might be.

Regards
  jonathan
Stefan Richter March 4, 2014, 8:33 a.m. UTC | #10
On Mar 04 Jonathan Woithe wrote:
> On Sat, Mar 01, 2014 at 03:20:50PM +0100, Stefan Richter wrote:
> > >> Since FFADO applies 200 ms or more as FCP
> > >> transaction timeout, shouldn't firewire-lib's fcp.c increase
> > >> FCP_TIMEOUT_MS from 125 to 200 or more as well?
[...]
> I've personally had nothing to do with devices utilising FCP transactions so
> unfortunately I don't really know.  My feeling is that the authors of the
> respective FFADO drivers would not have applied an FCP timeout of 200 ms if
> there was no demonstrated need for it.  Therefore in the absence of other
> evidence I would be assuming that there are devices which require the higher
> timeout allowed for in FFADO's streaming code.  However, I don't know which
> specific devices these might be.

./libffado/config.h.in:
#define IEEE1394SERVICE_FCP_RESPONSE_TIMEOUT_USEC       200000

was added in this FFADO revision:
http://subversion.ffado.org/changeset/1371
Timestamp:
    10/23/08 09:00:47 (5 years ago)
Author:
    ppalmers
Message:
    * implement our own code to do FCP transactions. the code from libavc
      had too much side-effects.
    * remove libavc1394 as a dependency
    * set the SPLIT_TIMEOUT value for the host controller such that late
      responses by the DM1x00 based devices are not discarded. Should fix
      the issues with FA-101 discovery. (re: #155, #162)

I will look through the libavc1394 source history.

Patch
diff mbox

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index a8199fd..73f5f26 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -89,13 +89,32 @@  static int pcr_modify(struct cmp_connection *c,
 		err = snd_fw_transaction(
 				c->resources.unit, TCODE_LOCK_COMPARE_SWAP,
 				get_offset(c, false), buffer, 8,
-				FW_FIXED_GENERATION | c->resources.generation);
+				FW_FIXED_GENERATION | c->resources.generation |
+				FW_RETURN_TIMEOUT);
 
 		if (err < 0) {
 			if (err == -EAGAIN &&
 			    bus_reset_handling == SUCCEED_ON_BUS_RESET)
 				err = 0;
-			return err;
+
+			if (err != -ETIMEDOUT)
+				return err;
+
+			/* Check current PCR. */
+			err = snd_fw_transaction(
+				c->resources.unit, TCODE_READ_QUADLET_REQUEST,
+				get_offset(c, false), buffer, 4,
+				FW_FIXED_GENERATION | c->resources.generation);
+			if (err < 0)
+				return err;
+
+			/* The lock transaction may be failed, retry */
+			if (buffer[0] != buffer[1]) {
+				buffer[0] = c->last_pcr_value;
+				continue;
+			}
+
+			break;
 		}
 
 		if (buffer[0] == old_arg) /* success? */
diff --git a/sound/firewire/fcp.c b/sound/firewire/fcp.c
index e903ddb..6fc813f 100644
--- a/sound/firewire/fcp.c
+++ b/sound/firewire/fcp.c
@@ -245,8 +245,9 @@  int fcp_avc_transaction(struct fw_unit *unit,
 					  : TCODE_WRITE_BLOCK_REQUEST;
 		ret = snd_fw_transaction(t.unit, tcode,
 					 CSR_REGISTER_BASE + CSR_FCP_COMMAND,
-					 (void *)command, command_size, 0);
-		if (ret < 0)
+					 (void *)command, command_size,
+					 FW_RETURN_TIMEOUT);
+		if ((ret < 0) && (ret != -ETIMEDOUT))
 			break;
 
 		wait_event_timeout(t.wait, t.state != STATE_PENDING,
diff --git a/sound/firewire/lib.c b/sound/firewire/lib.c
index 7409edb..6c3278b 100644
--- a/sound/firewire/lib.c
+++ b/sound/firewire/lib.c
@@ -22,7 +22,7 @@ 
  * @length: length of @buffer
  * @flags: use %FW_FIXED_GENERATION and add the generation value to attempt the
  *         request only in that generation; use %FW_QUIET to suppress error
- *         messages
+ *         messages: use %FW_RETURN_TIMEOUT to avoid retry at RCODE_CANCELLED
  *
  * Submits an asynchronous request to the target device, and waits for the
  * response.  The node ID and the current generation are derived from @unit.
@@ -53,6 +53,10 @@  int snd_fw_transaction(struct fw_unit *unit, int tcode,
 		if (rcode == RCODE_GENERATION && (flags & FW_FIXED_GENERATION))
 			return -EAGAIN;
 
+		/* timeout */
+		if ((rcode == RCODE_CANCELLED) && (flags & FW_RETURN_TIMEOUT))
+			return -ETIMEDOUT;
+
 		if (rcode_is_permanent_error(rcode) || ++tries >= 3) {
 			if (!(flags & FW_QUIET))
 				dev_err(&unit->device,
diff --git a/sound/firewire/lib.h b/sound/firewire/lib.h
index 02cfabc..b8ea90b 100644
--- a/sound/firewire/lib.h
+++ b/sound/firewire/lib.h
@@ -9,6 +9,7 @@  struct fw_unit;
 #define FW_GENERATION_MASK	0x00ff
 #define FW_FIXED_GENERATION	0x0100
 #define FW_QUIET		0x0200
+#define FW_RETURN_TIMEOUT	0x0400
 
 int snd_fw_transaction(struct fw_unit *unit, int tcode,
 		       u64 offset, void *buffer, size_t length,