[alsa-devel] [PATCH 0/4] ALSA: firewire-lib: code cleanup
Current implementation includes several macros and functions with improper name, hard-coded numerical value and so on.
This patchset is for code cleanup.
Takashi Sakamoto (4): ALSA: firewire-lib: rename local functions for code cleanup ALSA: firewire-lib: macro arrangement for code cleanup ALSA: firewire-lib: use dev_err() when detecting incoming streaming error ALSA: firewire-lib: use protocol error when detecting wrong value in CIP header
sound/firewire/amdtp.c | 104 ++++++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 49 deletions(-)
The naming rule for local functions was inconsistent. This commit rename them with a consistent manner.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp.c | 60 +++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index 97afc88..9d72345 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -260,15 +260,15 @@ unsigned int amdtp_stream_get_max_payload(struct amdtp_stream *s) } EXPORT_SYMBOL(amdtp_stream_get_max_payload);
-static void amdtp_write_s16(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); -static void amdtp_write_s32(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); -static void amdtp_read_s32(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames); +static void write_pcm_s16(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames); +static void write_pcm_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames); +static void read_pcm_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames);
/** * amdtp_stream_set_pcm_format - set the PCM format @@ -291,16 +291,16 @@ void amdtp_stream_set_pcm_format(struct amdtp_stream *s, /* fall through */ case SNDRV_PCM_FORMAT_S16: if (s->direction == AMDTP_OUT_STREAM) { - s->transfer_samples = amdtp_write_s16; + s->transfer_samples = write_pcm_s16; break; } WARN_ON(1); /* fall through */ case SNDRV_PCM_FORMAT_S32: if (s->direction == AMDTP_OUT_STREAM) - s->transfer_samples = amdtp_write_s32; + s->transfer_samples = write_pcm_s32; else - s->transfer_samples = amdtp_read_s32; + s->transfer_samples = read_pcm_s32; break; } } @@ -408,9 +408,9 @@ static unsigned int calculate_syt(struct amdtp_stream *s, } }
-static void amdtp_write_s32(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames) +static void write_pcm_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames) { struct snd_pcm_runtime *runtime = pcm->runtime; unsigned int channels, remaining_frames, i, c; @@ -433,9 +433,9 @@ static void amdtp_write_s32(struct amdtp_stream *s, } }
-static void amdtp_write_s16(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames) +static void write_pcm_s16(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames) { struct snd_pcm_runtime *runtime = pcm->runtime; unsigned int channels, remaining_frames, i, c; @@ -458,9 +458,9 @@ static void amdtp_write_s16(struct amdtp_stream *s, } }
-static void amdtp_read_s32(struct amdtp_stream *s, - struct snd_pcm_substream *pcm, - __be32 *buffer, unsigned int frames) +static void read_pcm_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames) { struct snd_pcm_runtime *runtime = pcm->runtime; unsigned int channels, remaining_frames, i, c; @@ -482,8 +482,8 @@ static void amdtp_read_s32(struct amdtp_stream *s, } }
-static void amdtp_fill_pcm_silence(struct amdtp_stream *s, - __be32 *buffer, unsigned int frames) +static void write_pcm_silence(struct amdtp_stream *s, + __be32 *buffer, unsigned int frames) { unsigned int i, c;
@@ -524,8 +524,8 @@ static void midi_rate_use_one_byte(struct amdtp_stream *s, unsigned int port) s->midi_fifo_used[port] += amdtp_rate_table[s->sfc]; }
-static void amdtp_fill_midi(struct amdtp_stream *s, - __be32 *buffer, unsigned int frames) +static void write_midi_messages(struct amdtp_stream *s, + __be32 *buffer, unsigned int frames) { unsigned int f, port; u8 *b; @@ -551,8 +551,8 @@ static void amdtp_fill_midi(struct amdtp_stream *s, } }
-static void amdtp_pull_midi(struct amdtp_stream *s, - __be32 *buffer, unsigned int frames) +static void read_midi_messages(struct amdtp_stream *s, + __be32 *buffer, unsigned int frames) { unsigned int f, port; int len; @@ -666,9 +666,9 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks, if (pcm) s->transfer_samples(s, pcm, buffer, data_blocks); else - amdtp_fill_pcm_silence(s, buffer, data_blocks); + write_pcm_silence(s, buffer, data_blocks); if (s->midi_ports) - amdtp_fill_midi(s, buffer, data_blocks); + write_midi_messages(s, buffer, data_blocks);
s->data_block_counter = (s->data_block_counter + data_blocks) & 0xff;
@@ -766,7 +766,7 @@ static int handle_in_packet(struct amdtp_stream *s, s->transfer_samples(s, pcm, buffer, data_blocks);
if (s->midi_ports) - amdtp_pull_midi(s, buffer, data_blocks); + read_midi_messages(s, buffer, data_blocks); }
if (s->flags & CIP_DBC_IS_END_EVENT)
Some macros include my misunderstanding for IEC 61883-1 or -6. Additionally, some fixed values appear on codes.
This commit replaces these with macros with proper names.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index 9d72345..29efbda 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -40,24 +40,28 @@ #define TAG_CIP 1
/* common isochronous packet header parameters */ -#define CIP_EOH (1u << 31) +#define CIP_EOH_SHIFT 31 +#define CIP_EOH (1u << CIP_EOH_SHIFT) #define CIP_EOH_MASK 0x80000000 -#define CIP_FMT_AM (0x10 << 24) +#define CIP_SID_SHIFT 24 +#define CIP_SID_MASK 0x3f000000 +#define CIP_DBS_MASK 0x00ff0000 +#define CIP_DBS_SHIFT 16 +#define CIP_DBC_MASK 0x000000ff +#define CIP_FMT_SHIFT 24 #define CIP_FMT_MASK 0x3f000000 +#define CIP_FDF_MASK 0x00ff0000 +#define CIP_FDF_SHIFT 16 #define CIP_SYT_MASK 0x0000ffff #define CIP_SYT_NO_INFO 0xffff -#define CIP_FDF_MASK 0x00ff0000 -#define CIP_FDF_SFC_SHIFT 16
/* * Audio and Music transfer protocol specific parameters * only "Clock-based rate control mode" is supported */ -#define AMDTP_FDF_AM824 (0 << (CIP_FDF_SFC_SHIFT + 3)) +#define CIP_FMT_AM (0x10 << CIP_FMT_SHIFT) +#define AMDTP_FDF_AM824 (0 << (CIP_FDF_SHIFT + 3)) #define AMDTP_FDF_NO_DATA 0xff -#define AMDTP_DBS_MASK 0x00ff0000 -#define AMDTP_DBS_SHIFT 16 -#define AMDTP_DBC_MASK 0x000000ff
/* TODO: make these configurable */ #define INTERRUPT_INTERVAL 16 @@ -656,10 +660,10 @@ static int handle_out_packet(struct amdtp_stream *s, unsigned int data_blocks,
buffer = s->buffer.packets[s->packet_index].buffer; buffer[0] = cpu_to_be32(ACCESS_ONCE(s->source_node_id_field) | - (s->data_block_quadlets << AMDTP_DBS_SHIFT) | + (s->data_block_quadlets << CIP_DBS_SHIFT) | s->data_block_counter); buffer[1] = cpu_to_be32(CIP_EOH | CIP_FMT_AM | AMDTP_FDF_AM824 | - (s->sfc << CIP_FDF_SFC_SHIFT) | syt); + (s->sfc << CIP_FDF_SHIFT) | syt); buffer += 2;
pcm = ACCESS_ONCE(s->pcm); @@ -712,11 +716,11 @@ static int handle_in_packet(struct amdtp_stream *s, /* Calculate data blocks */ if (payload_quadlets < 3 || ((cip_header[1] & CIP_FDF_MASK) == - (AMDTP_FDF_NO_DATA << CIP_FDF_SFC_SHIFT))) { + (AMDTP_FDF_NO_DATA << CIP_FDF_SHIFT))) { data_blocks = 0; } else { data_block_quadlets = - (cip_header[0] & AMDTP_DBS_MASK) >> AMDTP_DBS_SHIFT; + (cip_header[0] & CIP_DBS_MASK) >> CIP_DBS_SHIFT; /* avoid division by zero */ if (data_block_quadlets == 0) { dev_info_ratelimited(&s->unit->device, @@ -731,7 +735,7 @@ static int handle_in_packet(struct amdtp_stream *s, }
/* Check data block counter continuity */ - data_block_counter = cip_header[0] & AMDTP_DBC_MASK; + data_block_counter = cip_header[0] & CIP_DBC_MASK; if (data_blocks == 0 && (s->flags & CIP_EMPTY_HAS_WRONG_DBC) && s->data_block_counter != UINT_MAX) data_block_counter = s->data_block_counter; @@ -1050,8 +1054,10 @@ EXPORT_SYMBOL(amdtp_stream_pcm_pointer); */ void amdtp_stream_update(struct amdtp_stream *s) { + /* Precomputing. */ ACCESS_ONCE(s->source_node_id_field) = - (fw_parent_device(s->unit)->card->node_id & 0x3f) << 24; + (fw_parent_device(s->unit)->card->node_id << CIP_SID_SHIFT) & + CIP_SID_MASK; } EXPORT_SYMBOL(amdtp_stream_update);
When detecting invalid value in 'dbs' field of CIP header or packet discontinuity, current implementation reports the status by err_info(). In most cases this state is caused by model-specific issue due to vendor's customization and should be reported to developers.
This commit use dev_err() instead of dev_info() for this purpose. In the cases, packet streaming is aborted, thus no message floading occurs.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index 29efbda..93cf93a 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -723,7 +723,7 @@ static int handle_in_packet(struct amdtp_stream *s, (cip_header[0] & CIP_DBS_MASK) >> CIP_DBS_SHIFT; /* avoid division by zero */ if (data_block_quadlets == 0) { - dev_info_ratelimited(&s->unit->device, + dev_err(&s->unit->device, "Detect invalid value in dbs field: %08X\n", cip_header[0]); return -EIO; @@ -756,9 +756,9 @@ static int handle_in_packet(struct amdtp_stream *s, }
if (lost) { - dev_info(&s->unit->device, - "Detect discontinuity of CIP: %02X %02X\n", - s->data_block_counter, data_block_counter); + dev_err(&s->unit->device, + "Detect discontinuity of CIP: %02X %02X\n", + s->data_block_counter, data_block_counter); return -EIO; }
At Fri, 22 May 2015 23:21:13 +0900, Takashi Sakamoto wrote:
When detecting invalid value in 'dbs' field of CIP header or packet discontinuity, current implementation reports the status by err_info().
dev_info()
In most cases this state is caused by model-specific issue due to vendor's customization and should be reported to developers.
This commit use dev_err() instead of dev_info() for this purpose. In the cases, packet streaming is aborted, thus no message floading occurs.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp
sound/firewire/amdtp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index 29efbda..93cf93a 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -723,7 +723,7 @@ static int handle_in_packet(struct amdtp_stream *s, (cip_header[0] & CIP_DBS_MASK) >> CIP_DBS_SHIFT; /* avoid division by zero */ if (data_block_quadlets == 0) {
dev_info_ratelimited(&s->unit->device,
dev_err(&s->unit->device,
Here you dropped _ratelimited(). Are you sure that it won't give any problem?
Takashi
"Detect invalid value in dbs field: %08X\n", cip_header[0]); return -EIO;@@ -756,9 +756,9 @@ static int handle_in_packet(struct amdtp_stream *s, }
if (lost) {
dev_info(&s->unit->device,"Detect discontinuity of CIP: %02X %02X\n",s->data_block_counter, data_block_counter);
dev_err(&s->unit->device,"Detect discontinuity of CIP: %02X %02X\n",s->data_block_counter, data_block_counter);-- 2.1.4
Takashi Sakamoto wrote:
When detecting invalid value in 'dbs' field of CIP header or packet discontinuity, current implementation reports the status by err_info(). [...] This commit use dev_err() instead of dev_info() for this purpose. In the cases, packet streaming is aborted, thus no message floading occurs.
An aborted stream ends up in the XRUN state; the application is likely to prepare and start the stream again. So it is likely that there will be message flooding.
Regards, Clemens
Hi,
On May 23 2015 00:08, Takashi Iwai wrote:
Here you dropped _ratelimited(). Are you sure that it won't give any problem?
Similar situation as we discussed before: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-May/092192.html
On May 23 2015 03:05, Clemens Ladisch wrote:
An aborted stream ends up in the XRUN state; the application is likely to prepare and start the stream again. So it is likely that there will be message flooding.
Considering about how frequently this message can be generated at receiving such problematic packets, Not so flooding.
In the situation, all of committed drivers (fireworks/bebob/oxfw/dice) stops isochronous contexts and actual streams once, then start new streams and isochronous contexts. This is because our old packet stream falls to error state. Therefore, when devices transfers such problematic packets, different isochronous contexts (old one and new one) generate the error messages. This means that there're a gap between the generated error messages, approximately several hundred seconds because it cost expensive for devices to restart isochronous streams. In my opinion, this is not such flooding.
Regards
Takashi Sakamoto
On May 23 2015 13:16, Takashi Sakamoto wrote:
This means that there're a gap between the generated error messages, approximately several hundred seconds because it cost expensive for devices to restart isochronous streams. In my opinion, this is not such flooding.
Oops, "several hundred mili-seconds"...
Regards
Takashi Sakamoto
When detecting zero in 'dbs' field of CIP header, this packet streaming should be aborted because of avoiding division-by-zero. This is an error in an aspect of IEC 61883-1, thus protocol error.
This commit use EPROTO instead of EIO. Actually, the returned value is not used for userspace and this commit has no effect.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/amdtp.c b/sound/firewire/amdtp.c index 93cf93a..2b3e8b1 100644 --- a/sound/firewire/amdtp.c +++ b/sound/firewire/amdtp.c @@ -726,7 +726,7 @@ static int handle_in_packet(struct amdtp_stream *s, dev_err(&s->unit->device, "Detect invalid value in dbs field: %08X\n", cip_header[0]); - return -EIO; + return -EPROTO; } if (s->flags & CIP_WRONG_DBS) data_block_quadlets = s->data_block_quadlets;
Takashi Sakamoto wrote:
Current implementation includes several macros and functions with improper name, hard-coded numerical value and so on.
This patchset is for code cleanup.
Takashi Sakamoto (4): ALSA: firewire-lib: rename local functions for code cleanup ALSA: firewire-lib: macro arrangement for code cleanup ALSA: firewire-lib: use dev_err() when detecting incoming streaming error ALSA: firewire-lib: use protocol error when detecting wrong value in CIP header
All four: Acked-by: Clemens Ladisch clemens@ladisch.de
sound/firewire/amdtp.c | 104 ++++++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 49 deletions(-)
At Fri, 22 May 2015 23:21:10 +0900, Takashi Sakamoto wrote:
Current implementation includes several macros and functions with improper name, hard-coded numerical value and so on.
This patchset is for code cleanup.
Takashi Sakamoto (4): ALSA: firewire-lib: rename local functions for code cleanup ALSA: firewire-lib: macro arrangement for code cleanup ALSA: firewire-lib: use dev_err() when detecting incoming streaming error ALSA: firewire-lib: use protocol error when detecting wrong value in CIP header
Applied all four patches. Thanks.
Takashi
sound/firewire/amdtp.c | 104 ++++++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 49 deletions(-)
-- 2.1.4
On May 24 2015 15:35, Takashi Iwai wrote:
At Fri, 22 May 2015 23:21:10 +0900, Takashi Sakamoto wrote:
Current implementation includes several macros and functions with improper name, hard-coded numerical value and so on.
This patchset is for code cleanup.
Takashi Sakamoto (4): ALSA: firewire-lib: rename local functions for code cleanup ALSA: firewire-lib: macro arrangement for code cleanup ALSA: firewire-lib: use dev_err() when detecting incoming streaming error ALSA: firewire-lib: use protocol error when detecting wrong value in CIP header
Applied all four patches. Thanks.
Thanks for your reviewing.
Regards
Takashi Sakamoto
participants (3)
-
Clemens Ladisch -
Takashi Iwai -
Takashi Sakamoto