[PATCH] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

newer
[PATCH 0/3] ALSA: firewire: media...

Nathan Chancellor

24 Jun 2021 24 Jun '21

11:20 p.m.

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

Replace the post-increment shorthand with the full expression so the cast can be added in the right place and the look works as expected.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Signed-off-by: Nathan Chancellor nathan@kernel.org --- sound/usb/mixer_scarlett_gen2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..c20c7f1ddc50 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1186,7 +1186,7 @@ static int scarlett2_usb_get_config( if (err < 0) return err; if (size == 2) - for (i = 0; i < count; i++, (u16 *)buf++) + for (i = 0; i < count; i++, buf = (u16 *)buf + 1) *(u16 *)buf = le16_to_cpu(*(__le16 *)buf); return 0; }

base-commit: 5c89c2c7fbfa9124dd521c375b9c82b9ed75bc28

-- 2.32.0.93.g670b81a890

Show replies by date

Geoffrey D. Bennett

25 Jun 25 Jun

4:07 a.m.

On Thu, Jun 24, 2021 at 02:20:48PM -0700, Nathan Chancellor wrote:

...

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected.

Replace the post-increment shorthand with the full expression so the cast can be added in the right place and the look works as expected.

look -> loop

...

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Signed-off-by: Nathan Chancellor nathan@kernel.org

sound/usb/mixer_scarlett_gen2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..c20c7f1ddc50 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1186,7 +1186,7 @@ static int scarlett2_usb_get_config( if (err < 0) return err; if (size == 2)
	for (i = 0; i < count; i++, (u16 *)buf++)
	for (i = 0; i < count; i++, buf = (u16 *)buf + 1)
	*(u16 *)buf = le16_to_cpu(*(__le16 *)buf);
return 0; }

Thanks Nathan!

FYI: although incorrect, this caused no bug as there is not yet any case where count > 1.

Acked-by: Geoffrey D. Bennett g@b4.vu

Takashi Iwai

9:45 a.m.

On Thu, 24 Jun 2021 23:20:48 +0200, Nathan Chancellor wrote:

...

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected.

Replace the post-increment shorthand with the full expression so the cast can be added in the right place and the look works as expected.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Signed-off-by: Nathan Chancellor nathan@kernel.org

sound/usb/mixer_scarlett_gen2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..c20c7f1ddc50 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1186,7 +1186,7 @@ static int scarlett2_usb_get_config( if (err < 0) return err; if (size == 2)
	for (i = 0; i < count; i++, (u16 *)buf++)
	for (i = 0; i < count; i++, buf = (u16 *)buf + 1)
	*(u16 *)buf = le16_to_cpu(*(__le16 *)buf);

That's still too error-prone.

Could you rather introduce another variable of u16 * type, and use it instead? Ditto for u8 access for the code after that, too.

thanks,

Takashi

Nathan Chancellor

7:54 p.m.

New subject: [PATCH v2] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected. This is not a bug in practice because count is not greater than one at the moment but this could change in the future so this should be fixed.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

v1 -> v2:

* Use temporary variables of proper type rather than casting, as requested by Takashi. I did not include Geoffrey's ack for this reason.

* Mention that there is not a bug at the moment per Geoffrey's comment.

sound/usb/mixer_scarlett_gen2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..b13903bed330 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,17 +1177,22 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i; + u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */ if (config_item->size >= 8) { + u16 *buf_16; + size = config_item->size / 8 * count; err = scarlett2_usb_get(mixer, config_item->offset, buf, size); if (err < 0) return err; - if (size == 2) - for (i = 0; i < count; i++, (u16 *)buf++) - *(u16 *)buf = le16_to_cpu(*(__le16 *)buf); + if (size == 2) { + buf_16 = buf; + for (i = 0; i < count; i++, buf_16++) + *buf_16 = le16_to_cpu(*(__le16 *)buf_16); + } return 0; }

@@ -1197,8 +1202,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */ + buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1) - *(u8 *)buf++ = value & 1; + *buf_8++ = value & 1;

return 0; }

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

-- 2.32.0.93.g670b81a890

Geoffrey D. Bennett

8:43 p.m.

New subject: [PATCH v2] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

On Fri, Jun 25, 2021 at 10:54:19AM -0700, Nathan Chancellor wrote:

...

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected. This is not a bug in practice because count is not greater than one at the moment but this could change in the future so this should be fixed.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Signed-off-by: Nathan Chancellor nathan@kernel.org

v1 -> v2:

Use temporary variables of proper type rather than casting, as requested by Takashi. I did not include Geoffrey's ack for this reason.

Mention that there is not a bug at the moment per Geoffrey's comment.

sound/usb/mixer_scarlett_gen2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..b13903bed330 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,17 +1177,22 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i;
u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */ if (config_item->size >= 8) {
u16 *buf_16;

I would prefer that the u16 *buf_16 declaration above be removed from there...

...

size = config_item->size / 8 * count;
err = scarlett2_usb_get(mixer, config_item->offset, buf, size);
if (err < 0)
	return err;

```
if (size == 2)
```

	for (i = 0; i < count; i++, (u16 *)buf++)

		*(u16 *)buf = le16_to_cpu(*(__le16 *)buf);

```
if (size == 2) {
```
```
	buf_16 = buf;
```

...and combined with the assignment here, like: u16 *buf_16 = buf;

Regardless:

Acked-by: Geoffrey D. Bennett g@b4.vu

And, thanks again!

...

	for (i = 0; i < count; i++, buf_16++)
		*buf_16 = le16_to_cpu(*(__le16 *)buf_16);
}
return 0; }
@@ -1197,8 +1202,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */

buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1)
*(u8 *)buf++ = value & 1;
*buf_8++ = value & 1;
return 0;
}

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

2.32.0.93.g670b81a890

Nathan Chancellor

10:05 p.m.

New subject: [PATCH v3] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Acked-by: Geoffrey D. Bennett g@b4.vu Signed-off-by: Nathan Chancellor nathan@kernel.org ---

v1 -> v2:

* Use temporary variables of proper type rather than casting, as requested by Takashi.

* Mention that there is not a bug at the moment per Geoffrey's comment.

v2 -> v3:

* Restrict scope of buf_16 more, as requested by Geoffrey.

* Add Geoffrey's ack.

sound/usb/mixer_scarlett_gen2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..07ba0ef884a2 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,17 +1177,22 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i; + u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */ if (config_item->size >= 8) { + size = config_item->size / 8 * count; err = scarlett2_usb_get(mixer, config_item->offset, buf, size); if (err < 0) return err; - if (size == 2) - for (i = 0; i < count; i++, (u16 *)buf++) - *(u16 *)buf = le16_to_cpu(*(__le16 *)buf); + if (size == 2) { + u16 *buf_16 = buf; + + for (i = 0; i < count; i++, buf_16++) + *buf_16 = le16_to_cpu(*(__le16 *)buf_16); + } return 0; }

@@ -1197,8 +1202,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */ + buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1) - *(u8 *)buf++ = value & 1; + *buf_8++ = value & 1;

return 0; }

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

-- 2.32.0.93.g670b81a890

Nathan Chancellor

10:11 p.m.

New subject: [PATCH v4] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

v1 -> v2:

* Use temporary variables of proper type rather than casting, as requested by Takashi.

* Mention that there is not a bug at the moment per Geoffrey's comment.

v2 -> v3:

* Restrict scope of buf_16 more, as requested by Geoffrey.

* Add Geoffrey's ack.

v3 -> v4:

* Fix stray newline added below

if (config_item->size >= 8) {

leftover from buf_16's declaration.

sound/usb/mixer_scarlett_gen2.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..161d832cafef 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,6 +1177,7 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i; + u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */ @@ -1185,9 +1186,12 @@ static int scarlett2_usb_get_config( err = scarlett2_usb_get(mixer, config_item->offset, buf, size); if (err < 0) return err; - if (size == 2) - for (i = 0; i < count; i++, (u16 *)buf++) - *(u16 *)buf = le16_to_cpu(*(__le16 *)buf); + if (size == 2) { + u16 *buf_16 = buf; + + for (i = 0; i < count; i++, buf_16++) + *buf_16 = le16_to_cpu(*(__le16 *)buf_16); + } return 0; }

@@ -1197,8 +1201,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */ + buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1) - *(u8 *)buf++ = value & 1; + *buf_8++ = value & 1;

return 0; }

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

-- 2.32.0.93.g670b81a890

Geoffrey D. Bennett

10:26 p.m.

New subject: [PATCH v4] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

On Fri, Jun 25, 2021 at 01:11:51PM -0700, Nathan Chancellor wrote:

...

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected.

Your note about no bug which was added in v2 went missing in v3:

...

...
the loop does not iterate as expected. This is not a bug in practice because count is not greater than one at the moment but this could change in the future so this should be fixed.

...

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Acked-by: Geoffrey D. Bennett g@b4.vu Signed-off-by: Nathan Chancellor nathan@kernel.org

v1 -> v2:

Use temporary variables of proper type rather than casting, as requested by Takashi.

Mention that there is not a bug at the moment per Geoffrey's comment.

v2 -> v3:

Restrict scope of buf_16 more, as requested by Geoffrey.

Add Geoffrey's ack.

v3 -> v4:

Fix stray newline added below

if (config_item->size >= 8) {

leftover from buf_16's declaration.

sound/usb/mixer_scarlett_gen2.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..161d832cafef 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,6 +1177,7 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i;

u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */

@@ -1185,9 +1186,12 @@ static int scarlett2_usb_get_config( err = scarlett2_usb_get(mixer, config_item->offset, buf, size); if (err < 0) return err;
if (size == 2)
	for (i = 0; i < count; i++, (u16 *)buf++)
		*(u16 *)buf = le16_to_cpu(*(__le16 *)buf);
if (size == 2) {
	u16 *buf_16 = buf;
	for (i = 0; i < count; i++, buf_16++)
		*buf_16 = le16_to_cpu(*(__le16 *)buf_16);
}
return 0; }
@@ -1197,8 +1201,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */

buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1)
*(u8 *)buf++ = value & 1;
*buf_8++ = value & 1;
return 0;
}

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

2.32.0.93.g670b81a890

Nathan Chancellor

27 Jun 27 Jun

7:12 a.m.

New subject: [PATCH v5] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

v1 -> v2:

* Use temporary variables of proper type rather than casting, as requested by Takashi.

* Mention that there is not a bug at the moment per Geoffrey's comment.

v2 -> v3:

* Restrict scope of buf_16 more, as requested by Geoffrey.

* Add Geoffrey's ack.

v3 -> v4:

* Fix stray newline added below

if (config_item->size >= 8) {

leftover from buf_16's declaration.

v4 -> v5 (or how many times does it take Nathan to get a patch right):

* Re-add note about no bug that was dropped in v3 by accident, as noticed by Geoffrey. My apologies for the multiple revisions.

sound/usb/mixer_scarlett_gen2.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

@@ -1197,8 +1201,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */ + buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1) - *(u8 *)buf++ = value & 1; + *buf_8++ = value & 1;

return 0; }

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

-- 2.32.0.93.g670b81a890

Takashi Iwai

1 Jul 1 Jul

7:12 p.m.

New subject: [PATCH v5] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

On Sun, 27 Jun 2021 07:12:03 +0200, Nathan Chancellor wrote:

...

Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected. This is not a bug in practice because count is not greater than one at the moment but this could change in the future so this should be fixed.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Acked-by: Geoffrey D. Bennett g@b4.vu Signed-off-by: Nathan Chancellor nathan@kernel.org

v1 -> v2:

Use temporary variables of proper type rather than casting, as requested by Takashi.

Mention that there is not a bug at the moment per Geoffrey's comment.

v2 -> v3:

Restrict scope of buf_16 more, as requested by Geoffrey.

Add Geoffrey's ack.

v3 -> v4:

Fix stray newline added below

if (config_item->size >= 8) {

leftover from buf_16's declaration.

v4 -> v5 (or how many times does it take Nathan to get a patch right):

Re-add note about no bug that was dropped in v3 by accident, as noticed by Geoffrey. My apologies for the multiple revisions.

Thanks, applied now.

Takashi

Nathan Chancellor

25 Jun 25 Jun

10:08 p.m.

New subject: [PATCH v2] ALSA: usb-audio: scarlett2: Fix for loop increment in scarlett2_usb_get_config

On Sat, Jun 26, 2021 at 04:13:42AM +0930, Geoffrey D. Bennett wrote:

...

On Fri, Jun 25, 2021 at 10:54:19AM -0700, Nathan Chancellor wrote:

...
Clang warns:

sound/usb/mixer_scarlett_gen2.c:1189:32: warning: expression result unused [-Wunused-value] for (i = 0; i < count; i++, (u16 *)buf++) ^ ~~~~~ 1 warning generated.

It appears the intention was to cast the void pointer to a u16 pointer so that the data could be iterated through like an array of u16 values. However, the cast happens after the increment because a cast is an rvalue, whereas the post-increment operator only works on lvalues, so the loop does not iterate as expected. This is not a bug in practice because count is not greater than one at the moment but this could change in the future so this should be fixed.

Replace the cast with a temporary variable of the proper type, which is less error prone and fixes the iteration. Do the same thing for the 'u8 *' below this if block.

Fixes: ac34df733d2d ("ALSA: usb-audio: scarlett2: Update get_config to do endian conversion") Link: https://github.com/ClangBuiltLinux/linux/issues/1408 Signed-off-by: Nathan Chancellor nathan@kernel.org

v1 -> v2:

Use temporary variables of proper type rather than casting, as requested by Takashi. I did not include Geoffrey's ack for this reason.

Mention that there is not a bug at the moment per Geoffrey's comment.

sound/usb/mixer_scarlett_gen2.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index fcba682cd422..b13903bed330 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1177,17 +1177,22 @@ static int scarlett2_usb_get_config( const struct scarlett2_config *config_item = &scarlett2_config_items[info->has_mixer][config_item_num]; int size, err, i;
u8 *buf_8; u8 value;

/* For byte-sized parameters, retrieve directly into buf */ if (config_item->size >= 8) {
u16 *buf_16;
I would prefer that the u16 *buf_16 declaration above be removed from there...

...
size = config_item->size / 8 * count;
err = scarlett2_usb_get(mixer, config_item->offset, buf, size);
if (err < 0)
	return err;
if (size == 2)
	for (i = 0; i < count; i++, (u16 *)buf++)
		*(u16 *)buf = le16_to_cpu(*(__le16 *)buf);
if (size == 2) {
	buf_16 = buf;
...and combined with the assignment here, like: u16 *buf_16 = buf;

Thanks for pointing it out, I was not paying enough attention to realize that the scope could be reduced. v3 sent with your Ack added, thank you for taking a look in a quick manner!

Cheers, Nathan

...

Regardless:

Acked-by: Geoffrey D. Bennett g@b4.vu

And, thanks again!

...
	for (i = 0; i < count; i++, buf_16++)
		*buf_16 = le16_to_cpu(*(__le16 *)buf_16);
}
return 0; }
@@ -1197,8 +1202,9 @@ static int scarlett2_usb_get_config( return err;

/* then unpack from value into buf[] */

buf_8 = buf; for (i = 0; i < 8 && i < count; i++, value >>= 1)
*(u8 *)buf++ = value & 1;
*buf_8++ = value & 1;
return 0;
}

base-commit: 0cbbeaf370221fc469c95945dd3c1198865c5fe4

2.32.0.93.g670b81a890

1270

Age (days ago)

1277

Last active (days ago)

List overview

Download

10 comments

3 participants

tags (0)

participants (3)

Geoffrey D. Bennett
Nathan Chancellor
Takashi Iwai