On 25/07/24 09:38, Takashi Iwai wrote:
On Thu, 25 Jul 2024 17:11:31 +0200, Gustavo A. R. Silva wrote:
On 25/07/24 08:08, Gustavo A. R. Silva wrote:
Hi!
On 25/07/24 07:07, Takashi Iwai wrote:
On Thu, 25 Jul 2024 00:24:29 +0200, edmund.raile wrote:
Bisection revealed that the bitcrushing distortion with RME FireFace 800 was caused by 1d717123bb1a7555 ("ALSA: firewire-lib: Avoid -Wflex-array-member-not-at-end warning").
Reverting this commit yields restoration of clear audio output. I will send in a patch reverting this commit for now, soonTM.
#regzbot introduced: 1d717123bb1a7555
While it's OK to have a quick revert, it'd be worth to investigate further what broke there; the change is rather trivial, so it might be something in the macro expansion or a use of flex array stuff.
I wonder is there is any log that I can take a look at. That'd be really helpful.
OK, I found a discrepancy in how the `header_length` field in the flexible structure (a struct that contains a flexible-array member) below is used:
include/linux/firewire.h: 458 struct fw_iso_packet { ... 465 u32 header_length:8; /* Length of immediate header */ 466 /* tx: Top of 1394 isoch. data_block */ 467 u32 header[] __counted_by(header_length); 468 };
Take a look at the following piece of code:
sound/firewire/amdtp-stream.c: 1164 if (!(s->flags & CIP_NO_HEADER)) 1165 pkt_header_length = IT_PKT_HEADER_SIZE_CIP;
In the code above `pkt_header_length` is set to `IT_PKT_HEADER_SIZE_CIP`, which based on the following macros is 8 _bytes_:
sound/firewire/amdtp-stream.c:37:#define CIP_HEADER_QUADLETS 2 sound/firewire/amdtp-stream.c:58:#define CIP_HEADER_SIZE (sizeof(__be32) * CIP_HEADER_QUADLETS) sound/firewire/amdtp-stream.c:72:#define IT_PKT_HEADER_SIZE_CIP CIP_HEADER_SIZE
Then we use the DEFINE_FLEX() macro, which internally sets `template->header_length` to `CIP_HEADER_QUADLETS`, which based on the macros above, takes the value of 2 _elements_. We set `header_length` because such variable is the _counter_ used during the `__counted_by()` annotation in `struct fw_iso_packet`. The _counter_ is the variable that holds the number of _elements_ in the flex-array member at some point at run-time[1].
So, we set the counter to `CIP_HEADER_QUADLETS` because that's the total number of _elements_ allocated for the flexible-array member `header[]` by the DEFINE_FLEX() macro.
1183 DEFINE_FLEX(struct fw_iso_packet, template, header, 1184 header_length, CIP_HEADER_QUADLETS); 1185 bool sched_irq = false;
Then we call function `build_it_pkt_header()` and pass as arguments a pointer to `template`, and `pkt_header_length`, which at this point might hold the value of 8 _bytes_.
1187 build_it_pkt_header(s, desc->cycle, template, pkt_header_length, 1188 desc->data_blocks, desc->data_block_counter, 1189 desc->syt, i, curr_cycle_time);
Then inside function `build_it_pkt_header()`, the _counter_ is updated `params->header_length = header_length;`:
680 static void build_it_pkt_header(struct amdtp_stream *s, unsigned int cycle, 681 struct fw_iso_packet *params, unsigned int header_length, ... 692 if (header_length > 0) { 693 cip_header = (__be32 *)params->header; 694 generate_cip_header(s, cip_header, data_block_counter, syt); 695 params->header_length = header_length; 696 } else {
This causes `params->header_length == 8`; however, only enough space for 2 _elements_ was allocated for the flex array (via DEFINE_FLEX()).
So, regardless of how `pkt_header_length` is intended to be used in the rest of the code inside `build_it_pkt_header()`, this last update to `params->header_length` seems to be incorrect.
So, my question here is whether this `header_length` struct member was originally intended to be used as a counter for the elements in the flex array or as size variable to hold the total number of bytes in the array?
Based on the comment "Length of immediate header", I suppose `header_length` would hold _elements_ not _bytes_.
Thanks, now I took a look over the whole picture, and I guess there are two problems:
- The header_length should be in bytes, as far as I read the code in drivers/firwire/*. So the assumption in the commit d3155742db89 ("firewire: Annotate struct fw_iso_packet with __counted_by()") was already wrong, and it couldn't be annotated like that -- unless we fix up all users of header_length field.
I see. In this case, the code comment should also be changed:
s/Length/Size
to prevent any further confusion, as Size is clearly for bytes.
- By the use of DEFINE_FLEX() in amdtp-stream.c, process_rx_packets() sets the header_length field to CIP_HEADER_QUADLETS (= 2) as default. Meanwhile, build_it_pkt_header() doesn't touch header_length unless non-zero pkt_header_length is passed, supposing it being zero. So this may lead to a bogus header_length, which is processed by the firewire core code wrongly.
The actual effect we see is likely the latter. A simple fix would be to use DEFINE_RAW_FLEX() instead of DEFINE_FLEX() like below.
Yes, `DEFINE_RAW_FLEX()` is the way to go in this case, as long as the following patch is also applied:
--- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -462,9 +462,9 @@ struct fw_iso_packet { /* rx: Sync bit, wait for matching sy */ u32 tag:2; /* tx: Tag in packet header */ u32 sy:4; /* tx: Sy in packet header */ - u32 header_length:8; /* Length of immediate header */ + u32 header_length:8; /* Size of immediate header */ /* tx: Top of 1394 isoch. data_block */ - u32 header[] __counted_by(header_length); + u32 header[]; };
Thanks - Gustavo
thanks,
Takashi
-- 8< -- --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -1180,8 +1180,8 @@ static void process_rx_packets(struct fw_iso_context *context, u32 tstamp, size_ (void)fw_card_read_cycle_time(fw_parent_device(s->unit)->card, &curr_cycle_time);
for (i = 0; i < packets; ++i) {
DEFINE_FLEX(struct fw_iso_packet, template, header,header_length, CIP_HEADER_QUADLETS);
DEFINE_RAW_FLEX(struct fw_iso_packet, template, header,CIP_HEADER_QUADLETS);bool sched_irq = false;
build_it_pkt_header(s, desc->cycle, template, pkt_header_length,