[PATCH 0/3] ALSA: BeBoB: Fine-tuning for six function implementations

[PATCH 0/3] ALSA: BeBoB: Fine-tuning for six function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 6 Sep 2017 12:16:54 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Use common error handling code in snd_bebob_stream_start_duplex()
  Adjust six checks for null pointers
  Improve a size determination in parse_stream_formation()

 sound/firewire/bebob/bebob_stream.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

-- 
2.14.1

[PATCH 1/3] ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 6 Sep 2017 11:40:53 +0200

Add jump targets so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/firewire/bebob/bebob_stream.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/sound/firewire/bebob/bebob_stream.c b/sound/firewire/bebob/bebob_stream.c
index 4d3034a68bdf..bc9e42b6368e 100644
--- a/sound/firewire/bebob/bebob_stream.c
+++ b/sound/firewire/bebob/bebob_stream.c
@@ -629,8 +629,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 		if (err < 0) {
 			dev_err(&bebob->unit->device,
 				"fail to run AMDTP master stream:%d\n", err);
-			break_both_connections(bebob);
-			goto end;
+			goto break_connections;
 		}
 
 		/*
@@ -644,19 +643,15 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 				dev_err(&bebob->unit->device,
 					"fail to ensure sampling rate: %d\n",
 					err);
-				amdtp_stream_stop(&bebob->rx_stream);
-				break_both_connections(bebob);
-				goto end;
+				goto stop_rx_stream;
 			}
 		}
 
 		/* wait first callback */
 		if (!amdtp_stream_wait_callback(&bebob->rx_stream,
 						CALLBACK_TIMEOUT)) {
-			amdtp_stream_stop(&bebob->rx_stream);
-			break_both_connections(bebob);
 			err = -ETIMEDOUT;
-			goto end;
+			goto stop_rx_stream;
 		}
 	}
 
@@ -666,9 +661,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 		if (err < 0) {
 			dev_err(&bebob->unit->device,
 				"fail to run AMDTP slave stream:%d\n", err);
-			amdtp_stream_stop(&bebob->rx_stream);
-			break_both_connections(bebob);
-			goto end;
+			goto stop_rx_stream;
 		}
 
 		/* wait first callback */
@@ -682,6 +675,12 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 	}
 end:
 	return err;
+
+stop_rx_stream:
+	amdtp_stream_stop(&bebob->rx_stream);
+break_connections:
+	break_both_connections(bebob);
+	return err;
 }
 
 void snd_bebob_stream_stop_duplex(struct snd_bebob *bebob)
-- 
2.14.1

[PATCH 2/3] ALSA: bebob: Adjust six checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 6 Sep 2017 11:48:44 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written …

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/firewire/bebob/bebob_stream.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/firewire/bebob/bebob_stream.c b/sound/firewire/bebob/bebob_stream.c
index bc9e42b6368e..211ff43207d6 100644
--- a/sound/firewire/bebob/bebob_stream.c
+++ b/sound/firewire/bebob/bebob_stream.c
@@ -268,7 +268,7 @@ map_data_channels(struct snd_bebob *bebob, struct amdtp_stream *s)
 	 * use the maximum length of FCP.
 	 */
 	buf = kzalloc(256, GFP_KERNEL);
-	if (buf == NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	if (s == &bebob->tx_stream)
@@ -472,7 +472,7 @@ break_both_connections(struct snd_bebob *bebob)
 	bebob->connected = false;
 
 	/* These models seems to be in transition state for a longer time. */
-	if (bebob->maudio_special_quirk != NULL)
+	if (bebob->maudio_special_quirk)
 		msleep(200);
 }
 
@@ -496,7 +496,7 @@ start_stream(struct snd_bebob *bebob, struct amdtp_stream *stream,
 		conn = &bebob->out_conn;
 
 	/* channel mapping */
-	if (bebob->maudio_special_quirk == NULL) {
+	if (!bebob->maudio_special_quirk) {
 		err = map_data_channels(bebob, stream);
 		if (err < 0)
 			goto end;
@@ -611,7 +611,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 		 *
 		 * For firmware customized by M-Audio, refer to next NOTE.
 		 */
-		if (bebob->maudio_special_quirk == NULL) {
+		if (!bebob->maudio_special_quirk) {
 			err = rate_spec->set(bebob, rate);
 			if (err < 0) {
 				dev_err(&bebob->unit->device,
@@ -637,7 +637,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
 		 * The firmware customized by M-Audio uses these commands to
 		 * start transmitting stream. This is not usual way.
 		 */
-		if (bebob->maudio_special_quirk != NULL) {
+		if (bebob->maudio_special_quirk) {
 			err = rate_spec->set(bebob, rate);
 			if (err < 0) {
 				dev_err(&bebob->unit->device,
@@ -794,7 +794,7 @@ fill_stream_formations(struct snd_bebob *bebob, enum avc_bridgeco_plug_dir dir,
 	int err;
 
 	buf = kmalloc(FORMAT_MAXIMUM_LENGTH, GFP_KERNEL);
-	if (buf == NULL)
+	if (!buf)
 		return -ENOMEM;
 
 	if (dir == AVC_BRIDGECO_PLUG_DIR_IN)
-- 
2.14.1

[PATCH 3/3] ALSA: bebob: Improve a size determination in parse_stream_formation()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 6 Sep 2017 11:55:45 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/firewire/bebob/bebob_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/bebob/bebob_stream.c b/sound/firewire/bebob/bebob_stream.c
index 211ff43207d6..a26863b82330 100644
--- a/sound/firewire/bebob/bebob_stream.c
+++ b/sound/firewire/bebob/bebob_stream.c
@@ -736,7 +736,7 @@ parse_stream_formation(u8 *buf, unsigned int len,
 		return -ENOSYS;
 
 	/* Avoid double count by different entries for the same rate. */
-	memset(&formation[i], 0, sizeof(struct snd_bebob_stream_formation));
+	memset(&formation[i], 0, sizeof(*formation));
 
 	for (e = 0; e < buf[4]; e++) {
 		channels = buf[5 + e * 2];
-- 
2.14.1

Re: [PATCH 1/3] ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[Takashi Sakamoto]

Hi,

On Sep 6 2017 19:22, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 6 Sep 2017 11:40:53 +0200
> 
> Add jump targets so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   sound/firewire/bebob/bebob_stream.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/firewire/bebob/bebob_stream.c b/sound/firewire/bebob/bebob_stream.c
> index 4d3034a68bdf..bc9e42b6368e 100644
> --- a/sound/firewire/bebob/bebob_stream.c
> +++ b/sound/firewire/bebob/bebob_stream.c
> ...
> @@ -666,9 +661,7 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
>   		if (err < 0) {
>   			dev_err(&bebob->unit->device,
>   				"fail to run AMDTP slave stream:%d\n", err);
> -			amdtp_stream_stop(&bebob->rx_stream);
> -			break_both_connections(bebob);
> -			goto end;
> +			goto stop_rx_stream;
>   		}
>   
>   		/* wait first callback */

After the above code block, we have below code block.

658 /* start slave if needed */
659 if (!amdtp_stream_running(&bebob->tx_stream)) {
660     err = start_stream(bebob, &bebob->tx_stream, rate);
661     if (err < 0) {
662         dev_err(&bebob->unit->device,
663                 "fail to run AMDTP slave stream:%d\n", err);
664         goto stop_rx_stream;
665     }
666
667     /* wait first callback */
668     if (!amdtp_stream_wait_callback(&bebob->tx_stream,
669                                     CALLBACK_TIMEOUT)) {
670         amdtp_stream_stop(&bebob->tx_stream);
671         amdtp_stream_stop(&bebob->rx_stream);
672         break_both_connections(bebob);
673         err = -ETIMEDOUT;
674     }
675 }

I think it better to apply your solution too in the above to keep code 
consistency.

> @@ -682,6 +675,12 @@ int snd_bebob_stream_start_duplex(struct snd_bebob *bebob, unsigned int rate)
>   	}
>   end:
>   	return err;
> +
> +stop_rx_stream:
> +	amdtp_stream_stop(&bebob->rx_stream);
> +break_connections:
> +	break_both_connections(bebob);
> +	return err;
>   }
>   
>   void snd_bebob_stream_stop_duplex(struct snd_bebob *bebob)

For the other patches, I can find no merit to apply except for reduction 
of the number of characters included in the file.


Thanks

Takashi Sakamoto

Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[SF Markus Elfring]

> 668     if (!amdtp_stream_wait_callback(&bebob->tx_stream,
> 669                                     CALLBACK_TIMEOUT)) {
> 670         amdtp_stream_stop(&bebob->tx_stream);
> 671         amdtp_stream_stop(&bebob->rx_stream);
> 672         break_both_connections(bebob);
> 673         err = -ETIMEDOUT;
> 674     }
> 675 }
> 
> I think it better to apply your solution too in the above to keep code consistency.

How do you think about to adjust this function implementation after the other two
update steps from the patch series would be integrated?


> For the other patches, I can find no merit to apply except for reduction
> of the number of characters included in the file.

Would you like to refer to any specific update suggestions for further clarification?

Regards,
Markus

Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[Takashi Sakamoto]

Hi,

On Sep 24 2017 16:06, SF Markus Elfring wrote:
>> 668     if (!amdtp_stream_wait_callback(&bebob->tx_stream,
>> 669                                     CALLBACK_TIMEOUT)) {
>> 670         amdtp_stream_stop(&bebob->tx_stream);
>> 671         amdtp_stream_stop(&bebob->rx_stream);
>> 672         break_both_connections(bebob);
>> 673         err = -ETIMEDOUT;
>> 674     }
>> 675 }
>>
>> I think it better to apply your solution too in the above to keep code consistency.
> 
> How do you think about to adjust this function implementation after the other two
> update steps from the patch series would be integrated?
> 
> 
>> For the other patches, I can find no merit to apply except for reduction
>> of the number of characters included in the file.
> 
> Would you like to refer to any specific update suggestions for further clarification?

I had already read your patch comments and understand your intention
somehow, because it's a part of task for daily reviewing. Then, I did
comment.

At development for Linux kernel, there're some important points for
developers to notice. In our case, we should care of _practicality_.
Roughly speaking, our work for kernel should add advantages for
kernel/application runtime or assists the other developers' work. In
this point, some patches are welcome and the others are not. I'll show
you two examples.

In this subsystem, I have reviewed patches from Julia Lawall. The most
of her or his patches attempt to add 'const' qualifier for read-only
symbols. As a result, the symbols place to '.rodata' section of ELF
binary. This section has a hint for loaders to put these symbols to
segments with read-only attributes. This has an advantage for
compilation time and runtime. Recent compilers can detect codes to
change content of the symbols with 'const' qualifier. Even if the codes
were exposed from developers understanding or compilers' check,
segmentation would occur in runtime at early stage of development or
early days after releasing kernel. This is very helpful for developers
to find unexpected changes for contents of the symbol.

I also subscribe a mailing list of Linux Driver Project[1], which is for
various drivers. Sometimes posted patches are rejected by maintainers.
Such patches typically include minor code change without actual
advantages for runtime or developers. For instance, patches for '80
characters per line' are often posted and rejected. If this kind of
patchset were added to change history of kernel, tons of unpractical
logs are accumulated for development.  This make it worse for developers
to maintain the kernel.

By the way, ALSA BeBoB driver is just for ASIC and firmwares which
ArchWave AG (formerly BridgeCo. AG.) had produced for devices with
IEC 61883-1/6 packet streaming on IEEE 1394 bus and Time Sensitive
Network (TSN). As long as I know, the last product with this solution
was shipped at 2010. Thus the driver is under maintenance. I have some
tasks for this driver as well as drivers in ALSA firewire stack,
however basically this driver is under maintenance and might not get
further integration. I think that code cleanup for the driver don't help
the other developers. It's better to apply such cleanup to more
long-live codes such as core functionality of ALSA. (But pay enough
attention to practicality of the changes when you start this kind of
work.)

In this point of view, whether your patchset is worth to be applied
or not, Please keep enough time to think about.


[1] http://driverdev.linuxdriverproject.org/mailman/listinfo

Thanks

Takashi Sakamoto

Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[SF Markus Elfring]

> As long as I know, the last product with this solution was shipped
> at 2010. Thus the driver is under maintenance.

Thanks for your information.


> I have some tasks for this driver as well as drivers in ALSA firewire stack,
> however basically this driver is under maintenance and might not get
> further integration.

Good to know in principle …


> I think that code cleanup for the driver don't help the other developers.

I thought in an other direction if other contributors would like to care
for longer term support.


> It's better to apply such cleanup to more long-live codes such as
> core functionality of ALSA.

I am trying another general transformation pattern out.


> In this point of view, whether your patchset is worth to be applied
> or not, Please keep enough time to think about.

Automatic source code analysis can find various update candidates.
I am just unsure if a bit more software development attention is still
appropriate at some places.

Do you find any of the affected software modules so outdated
so that they should not be touched any more?

Regards,
Markus

Re: ALSA: bebob: Use common error handling code in snd_bebob_stream_start_duplex()

[Takashi Sakamoto]

On Sep 27 2017 15:38, SF Markus Elfring wrote:
>> As long as I know, the last product with this solution was shipped
>> at 2010. Thus the driver is under maintenance.
> 
> Thanks for your information.
> 
> 
>> I have some tasks for this driver as well as drivers in ALSA firewire stack,
>> however basically this driver is under maintenance and might not get
>> further integration.
> 
> Good to know in principle …
> 
> 
>> I think that code cleanup for the driver don't help the other developers.
> 
> I thought in an other direction if other contributors would like to care
> for longer term support.
> 
> 
>> It's better to apply such cleanup to more long-live codes such as
>> core functionality of ALSA.
> 
> I am trying another general transformation pattern out.
> 
> 
>> In this point of view, whether your patchset is worth to be applied
>> or not, Please keep enough time to think about.
> 
> Automatic source code analysis can find various update candidates.
> I am just unsure if a bit more software development attention is still
> appropriate at some places.
> 
> Do you find any of the affected software modules so outdated
> so that they should not be touched any more?

Please don't ignore the practicality I've explained, and don't 
accumulate your questions without it.


Regards

Takashi Sakamoto