[PATCH 0/4] ALSA: firewire-lib: code cleanup

[PATCH 0/4] ALSA: firewire-lib: code cleanup

[Takashi Sakamoto]

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(-)

-- 
2.1.4

[PATCH 1/4] ALSA: firewire-lib: rename local functions for code cleanup

[Takashi Sakamoto]

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)
-- 
2.1.4

[PATCH 2/4] ALSA: firewire-lib: macro arrangement for code cleanup

[Takashi Sakamoto]

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);
 
-- 
2.1.4

[PATCH 3/4] ALSA: firewire-lib: use dev_err() when detecting incoming streaming error

[Takashi Sakamoto]

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;
 	}
 
-- 
2.1.4

Re: [PATCH 3/4] ALSA: firewire-lib: use dev_err() when detecting incoming streaming error

[Takashi Iwai]

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);
>  		return -EIO;
>  	}
>  
> -- 
> 2.1.4
> 

Re: [PATCH 3/4] ALSA: firewire-lib: use dev_err() when detecting incoming streaming error

[Clemens Ladisch]

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

Re: [PATCH 3/4] ALSA: firewire-lib: use dev_err() when detecting incoming streaming error

[Takashi Sakamoto]

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

Re: [PATCH 3/4] ALSA: firewire-lib: use dev_err() when detecting incoming streaming error

[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

[PATCH 4/4] ALSA: firewire-lib: use protocol error when detecting wrong value in CIP header

[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;
-- 
2.1.4

Re: [PATCH 0/4] ALSA: firewire-lib: code cleanup

[Clemens Ladisch]

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(-)

Re: [PATCH 0/4] ALSA: firewire-lib: code cleanup

[Takashi Iwai]

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
> 

Re: [PATCH 0/4] ALSA: firewire-lib: code cleanup

[Takashi Sakamoto]

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