[PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[René Herman]

Hi Takashi.

I sent these out a week ago to alsa-devel (only) but am not sure they
got anywhere, perhaps due to at least the alsa-devel archive scrubbing
the text/x-patch attachments as "non-text".

The snd-usb-6fire driver for the TerraTec DMX 6Fire USB soundcard has
been failing its firmware upload due to a non DMA-capable buffer on the
stack. First of the patches kmallocs said bufffer instead and fixes the
firmware upload.

After that first patch the driver nominally works again but still has
Pulseaudio crap out due to struct snd_pcm_hardware.channels_min=1
causing it to recognize it as a mono device only. Comparing with e.g.
the TerraTec Aureon 7.1 Universe driver it seems that the solution is to
simply set channels_min=2 as per the second patch.

Third patch unmarks the snd_pcm_hardware struct const as it it is in
fact changed in usb6fire_pcm_open(), even though through a pointer, and
is supposedly trivial.

With these changes the card works again. Driver author Torsten Schenk
has seen these and is fine with them: maintains an external driver with
more options. I or he might time permitting start integrating more into
the kernel driver over time.

Regards,
René

René Herman (3):
  snd-usb-6fire: Move DMA-buffer off of the stack
  snd-usb-6fire: Pulseaudio needs snd_pcm_hardware.channels_min > 1
  snd-usb-6fire: Unmark struct snd_pcm_hardware const

 firmware.c | 95 ++++++++++++++++++++++++++----------------------------
 pcm.c      |  4 +--
 2 files changed, 48 insertions(+), 51 deletions(-)

-- 
2.17.1

[PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack

[René Herman]

snd-usb-fire currently fails its firmware load with "transfer buffer not dma
capable". Move said buffer off of the stack.

Signed-off-by: René Herman <rene.herman@gmail.com>
---
 firmware.c | 95 ++++++++++++++++++++++++++----------------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

diff --git a/firmware.c b/firmware.c
index 69137c1..502653a 100644
--- a/firmware.c
+++ b/firmware.c
@@ -355,63 +355,60 @@ static int usb6fire_fw_check(struct usb_interface *intf, const u8 *version)
 
 int usb6fire_fw_init(struct usb_interface *intf)
 {
-	int i;
-	int ret;
 	struct usb_device *device = interface_to_usbdev(intf);
+	int ret, i;
+
 	/* buffer: 8 receiving bytes from device and
 	 * sizeof(EP_W_MAX_PACKET_SIZE) bytes for non-const copy */
-	u8 buffer[12];
+	u8 *buffer = kmalloc(12, GFP_KERNEL);
+
+	if (!buffer)
+		return -ENOMEM;
 
 	ret = usb6fire_fw_ezusb_read(device, 1, 0, buffer, 8);
 	if (ret < 0) {
 		dev_err(&intf->dev,
 			"unable to receive device firmware state.\n");
-		return ret;
-	}
-	if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55) {
-		dev_err(&intf->dev,
-			"unknown device firmware state received from device:");
-		for (i = 0; i < 8; i++)
-			printk(KERN_CONT "%02x ", buffer[i]);
-		printk(KERN_CONT "\n");
-		return -EIO;
-	}
-	/* do we need fpga loader ezusb firmware? */
-	if (buffer[3] == 0x01) {
-		ret = usb6fire_fw_ezusb_upload(intf,
-				"6fire/dmx6firel2.ihx", 0, NULL, 0);
-		if (ret < 0)
-			return ret;
-		return FW_NOT_READY;
+		goto out;
 	}
-	/* do we need fpga firmware and application ezusb firmware? */
-	else if (buffer[3] == 0x02) {
-		ret = usb6fire_fw_check(intf, buffer + 4);
-		if (ret < 0)
-			return ret;
-		ret = usb6fire_fw_fpga_upload(intf, "6fire/dmx6firecf.bin");
-		if (ret < 0)
-			return ret;
-		memcpy(buffer, ep_w_max_packet_size,
-				sizeof(ep_w_max_packet_size));
-		ret = usb6fire_fw_ezusb_upload(intf, "6fire/dmx6fireap.ihx",
-				0x0003,	buffer, sizeof(ep_w_max_packet_size));
-		if (ret < 0)
-			return ret;
-		return FW_NOT_READY;
-	}
-	/* all fw loaded? */
-	else if (buffer[3] == 0x03)
-		return usb6fire_fw_check(intf, buffer + 4);
-	/* unknown data? */
-	else {
-		dev_err(&intf->dev,
-			"unknown device firmware state received from device: ");
-		for (i = 0; i < 8; i++)
-			printk(KERN_CONT "%02x ", buffer[i]);
-		printk(KERN_CONT "\n");
-		return -EIO;
+	if (buffer[0] == 0xeb && buffer[1] == 0xaa && buffer[2] == 0x55) {
+		/* do we need fpga loader ezusb firmware? */
+		if (buffer[3] == 1) {
+			ret = usb6fire_fw_ezusb_upload(intf,
+					"6fire/dmx6firel2.ihx", 0, NULL, 0);
+			if (ret >= 0)
+				ret = FW_NOT_READY;
+			goto out;
+		}
+		/* do we need fpga firmware and application ezusb firmware? */
+		if (buffer[3] == 2) {
+			ret = usb6fire_fw_check(intf, buffer + 4);
+			if (ret < 0)
+				goto out;
+			ret = usb6fire_fw_fpga_upload(intf, "6fire/dmx6firecf.bin");
+			if (ret < 0)
+				goto out;
+			memcpy(buffer, ep_w_max_packet_size,
+					sizeof(ep_w_max_packet_size));
+			ret = usb6fire_fw_ezusb_upload(intf, "6fire/dmx6fireap.ihx",
+					0x0003,	buffer, sizeof(ep_w_max_packet_size));
+			if (ret >= 0)
+				ret = FW_NOT_READY;
+			goto out;
+		}
+		/* all fw loaded? */
+		if (buffer[3] == 3) {
+			ret = usb6fire_fw_check(intf, buffer + 4);
+			goto out;
+		}
 	}
-	return 0;
+	dev_err(&intf->dev,
+		"unknown device firmware state received from device: ");
+	for (i = 0; i < 8; i++)
+		printk(KERN_CONT "%02x ", buffer[i]);
+	printk(KERN_CONT "\n");
+	ret = -EIO;
+
+out:	kfree(buffer);
+	return ret;
 }
-
-- 
2.17.1

[PATCH 2/3] snd-usb-6fire: Pulseaudio needs snd_pcm_hardware.channels_min > 1

[René Herman]

Pulseaudio with snd_pcm_hardware.channels_min=1 creates a mono device
only.

Signed-off-by: René Herman <rene.herman@gmail.com>
---
 pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pcm.c b/pcm.c
index 88ac1c4..cce1312 100644
--- a/pcm.c
+++ b/pcm.c
@@ -58,7 +58,7 @@ static const struct snd_pcm_hardware pcm_hw = {
 
 	.rate_min = 44100,
 	.rate_max = 192000,
-	.channels_min = 1,
+	.channels_min = 2,
 	.channels_max = 0, /* set in pcm_open, depending on capture/playback */
 	.buffer_bytes_max = MAX_BUFSIZE,
 	.period_bytes_min = PCM_N_PACKETS_PER_URB * (PCM_MAX_PACKET_SIZE - 4),
-- 
2.17.1

[PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const

[René Herman]

struct snd_pcm_hardware pcm_hw is in fact changed in usb6fire_pcm_open()

Signed-off-by: René Herman <rene.herman@gmail.com>
---
 pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pcm.c b/pcm.c
index cce1312..8ccf638 100644
--- a/pcm.c
+++ b/pcm.c
@@ -40,7 +40,7 @@ enum { /* pcm streaming states */
 	STREAM_STOPPING
 };
 
-static const struct snd_pcm_hardware pcm_hw = {
+static struct snd_pcm_hardware pcm_hw = {
 	.info = SNDRV_PCM_INFO_MMAP |
 		SNDRV_PCM_INFO_INTERLEAVED |
 		SNDRV_PCM_INFO_BLOCK_TRANSFER |
-- 
2.17.1

Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const

[Takashi Iwai]

On Tue, 21 Jul 2020 08:48:53 +0200,
René Herman wrote:
> 
> struct snd_pcm_hardware pcm_hw is in fact changed in usb6fire_pcm_open()

This must be superfluous.  usb6fire_pcm_open() changes the field of
the copied pcm_hw, not the original pcm_hw itself.  Otherwise we must
have got already a compile warning / error.


thanks,

Takashi


> 
> Signed-off-by: René Herman <rene.herman@gmail.com>
> ---
>  pcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pcm.c b/pcm.c
> index cce1312..8ccf638 100644
> --- a/pcm.c
> +++ b/pcm.c
> @@ -40,7 +40,7 @@ enum { /* pcm streaming states */
>  	STREAM_STOPPING
>  };
>  
> -static const struct snd_pcm_hardware pcm_hw = {
> +static struct snd_pcm_hardware pcm_hw = {
>  	.info = SNDRV_PCM_INFO_MMAP |
>  		SNDRV_PCM_INFO_INTERLEAVED |
>  		SNDRV_PCM_INFO_BLOCK_TRANSFER |
> -- 
> 2.17.1
> 

Re: [PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack

[Takashi Iwai]

On Tue, 21 Jul 2020 08:48:51 +0200,
René Herman wrote:
> 
> snd-usb-fire currently fails its firmware load with "transfer buffer not dma
> capable". Move said buffer off of the stack.
> 
> Signed-off-by: René Herman <rene.herman@gmail.com>
> ---
>  firmware.c | 95 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/firmware.c b/firmware.c
> index 69137c1..502653a 100644
> --- a/firmware.c
> +++ b/firmware.c
> @@ -355,63 +355,60 @@ static int usb6fire_fw_check(struct usb_interface *intf, const u8 *version)
>  
>  int usb6fire_fw_init(struct usb_interface *intf)
>  {
> -	int i;
> -	int ret;
>  	struct usb_device *device = interface_to_usbdev(intf);
> +	int ret, i;
> +
>  	/* buffer: 8 receiving bytes from device and
>  	 * sizeof(EP_W_MAX_PACKET_SIZE) bytes for non-const copy */
> -	u8 buffer[12];
> +	u8 *buffer = kmalloc(12, GFP_KERNEL);
> +
> +	if (!buffer)
> +		return -ENOMEM;
>  
>  	ret = usb6fire_fw_ezusb_read(device, 1, 0, buffer, 8);
>  	if (ret < 0) {
>  		dev_err(&intf->dev,
>  			"unable to receive device firmware state.\n");
> -		return ret;
> -	}
> -	if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55) {
> -		dev_err(&intf->dev,
> -			"unknown device firmware state received from device:");
> -		for (i = 0; i < 8; i++)
> -			printk(KERN_CONT "%02x ", buffer[i]);
> -		printk(KERN_CONT "\n");
> -		return -EIO;
> -	}

Could you rather change return with goto out (with ret variable set)?
In that way we can see what actually you changed more clearly.


thanks,

Takashi

Re: [PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[Takashi Iwai]

On Tue, 21 Jul 2020 08:48:50 +0200,
René Herman wrote:
> 
> Hi Takashi.
> 
> I sent these out a week ago to alsa-devel (only) but am not sure they
> got anywhere, perhaps due to at least the alsa-devel archive scrubbing
> the text/x-patch attachments as "non-text".
> 
> The snd-usb-6fire driver for the TerraTec DMX 6Fire USB soundcard has
> been failing its firmware upload due to a non DMA-capable buffer on the
> stack. First of the patches kmallocs said bufffer instead and fixes the
> firmware upload.
> 
> After that first patch the driver nominally works again but still has
> Pulseaudio crap out due to struct snd_pcm_hardware.channels_min=1
> causing it to recognize it as a mono device only. Comparing with e.g.
> the TerraTec Aureon 7.1 Universe driver it seems that the solution is to
> simply set channels_min=2 as per the second patch.
> 
> Third patch unmarks the snd_pcm_hardware struct const as it it is in
> fact changed in usb6fire_pcm_open(), even though through a pointer, and
> is supposedly trivial.
> 
> With these changes the card works again. Driver author Torsten Schenk
> has seen these and is fine with them: maintains an external driver with
> more options. I or he might time permitting start integrating more into
> the kernel driver over time.

The patch needs to point to the right path that is applicable with
patch -p1 option, i.e. it should be like
  diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c

At best use git for creating / submitting a patch.


thanks,

Takashi

Re: [PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[René Herman]

On 21-07-2020 10:02, Takashi Iwai wrote:

> The patch needs to point to the right path that is applicable with
> patch -p1 option, i.e. it should be like
>   diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c
> 
> At best use git for creating / submitting a patch.

Blast. I did, but worked against an isolated (DKMS-ed) version of the
kernel driver. Pardon. Shall re-setup against the kernel as such and
resend after dealing with your other two comments. I.e.,

Re: [PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack

> Could you rather change return with goto out (with ret variable
> set)? In that way we can see what actually you changed more clearly.

I already did exactly that though and in fact, the original not doing so
is what makes the patch seem involved. With the added kmalloc() I change
it so that all returns goto out, which kfree()s again. The only one that
does not is when kmalloc() fails, i.e., when there's nothing to kfree().
I suppose you just misread and do not need to have that single one go
through a goto as well?

Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const

> This must be superfluous.  usb6fire_pcm_open() changes the field of 
> the copied pcm_hw, not the original pcm_hw itself.  Otherwise we
> must have got already a compile warning / error.

Unfortunately no; it's as mentioned in the cover letter accessed via
pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then
changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;".
I.e., not a copy.

And yes, it had me wonder if this was "allowed" in the first place as
well (the same pcm_hw structure is also shared between playback and
capture) but it does seem to work fine, so just unmarking it const seems
a minimal fix or, better, "cleanup", since nothing complained, but the
const qualifier no doubt at least /conceptually/ means it could be an issue.

I'll re-setup against the kernel as such and wait for reply on the above
two comments to know what to re-submit.

Regards,
Rene

Re: [PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[Takashi Iwai]

On Tue, 21 Jul 2020 10:29:29 +0200,
Ren9 Herman wrote:
> 
> On 21-07-2020 10:02, Takashi Iwai wrote:
> 
> > The patch needs to point to the right path that is applicable with
> > patch -p1 option, i.e. it should be like
> >   diff -up a/sound/usb/6fire/xxx.c b/sound/usb/6fire/xxx.c
> > 
> > At best use git for creating / submitting a patch.
> 
> Blast. I did, but worked against an isolated (DKMS-ed) version of the
> kernel driver. Pardon. Shall re-setup against the kernel as such and
> resend after dealing with your other two comments. I.e.,
> 
> Re: [PATCH 1/3] snd-usb-6fire: Move DMA-buffer off of the stack

Submit a complete set freshly as a v2 patch set.

  [PATCH v2 1/3] ...

> > Could you rather change return with goto out (with ret variable
> > set)? In that way we can see what actually you changed more clearly.
> 
> I already did exactly that though and in fact, the original not doing so
> is what makes the patch seem involved. With the added kmalloc() I change
> it so that all returns goto out, which kfree()s again. The only one that
> does not is when kmalloc() fails, i.e., when there's nothing to kfree().
> I suppose you just misread and do not need to have that single one go
> through a goto as well?

The first return for kmalloc error should remain so, but the rest can
be replaced with goto out.  That I meant.

> 
> Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const
> 
> > This must be superfluous.  usb6fire_pcm_open() changes the field of 
> > the copied pcm_hw, not the original pcm_hw itself.  Otherwise we
> > must have got already a compile warning / error.
> 
> Unfortunately no; it's as mentioned in the cover letter accessed via
> pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then
> changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;".
> I.e., not a copy.

Note that it copies the whole instance, not the pointer.  So the
current code is correct.


thanks,

Takashi

Re: [PATCH 0/3] snd-usb-6fire: firmware load and pulseaudio assumption

[René Herman]

On 21-07-2020 10:37, Takashi Iwai wrote:

> Submit a complete set freshly as a v2 patch set.
> 
>   [PATCH v2 1/3] ...

Done, although now only the first two are left since you were of course
quite right about the third.

> The first return for kmalloc error should remain so, but the rest can
> be replaced with goto out.  That I meant.

I know, but that's exactly what it already did :) I guess it looks a bit
non-generic as a patch due to me also unifying the error path due to
that same change in structure. But any case, v2 is the same as v1
therefore, minus the fixed paths of course.

>> Re: [PATCH 3/3] snd-usb-6fire: Unmark struct snd_pcm_hardware const
>>
>>> This must be superfluous.  usb6fire_pcm_open() changes the field of 
>>> the copied pcm_hw, not the original pcm_hw itself.  Otherwise we
>>> must have got already a compile warning / error.
>>
>> Unfortunately no; it's as mentioned in the cover letter accessed via
>> pointer: usb6fire_pcm_open() sets "alsa_rt->hw = pcm_hw" and then
>> changes pcm_hw as e.g. "alsa_rt->hw.channels_max = OUT_N_CHANNELS;".
>> I.e., not a copy.
> 
> Note that it copies the whole instance, not the pointer.  So the
> current code is correct.

Yap. I blindly assumed that snd_pcm_runtime.hw would be a pointer and
synchronized that with the code without even really thinking about it by
convincing myself that, as in the case of an array, that "pcm_hw" name
would deteriorate to a pointer automatically. As such now keenly aware
that my grasp of C semantics has also taken a bit of a nosedive over the
last 10 years or so, I'll be more careful. Patch dropped.

Thanks.

Rene.