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

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

[René Herman]

Hi Takashi.

v2 of the earlier today sent out "set" for snd-usb-6fire, although now
only two patches left, incorportaing the requested changes.

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; also makes it more alsa-generic by using the "goto out"
structure. Note that it only looks a bit ungeneric as a patch due to
also needing to at the same time unify its failure path: it's obvious
when looked at post-apply.

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.

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 (2):
  Move DMA-buffer off the stack
  Pulseaudio needs snd_pcm_hardware.channels_min > 1

 sound/usb/6fire/firmware.c | 95 ++++++++++++++++++--------------------
 sound/usb/6fire/pcm.c      |  2 +-
 2 files changed, 47 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] Move DMA-buffer off 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>
---
 sound/usb/6fire/firmware.c | 95 ++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

diff --git a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c
index 69137c14d0dc..502653a89f01 100644
--- a/sound/usb/6fire/firmware.c
+++ b/sound/usb/6fire/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

Re: [PATCH v2 1/2] Move DMA-buffer off the stack

[Takashi Iwai]

On Tue, 21 Jul 2020 11:57:47 +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>
> ---
>  sound/usb/6fire/firmware.c | 95 ++++++++++++++++++--------------------
>  1 file changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c
> index 69137c14d0dc..502653a89f01 100644
> --- a/sound/usb/6fire/firmware.c
> +++ b/sound/usb/6fire/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;
> +

Don't put a blank line here.

>  	/* 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;

Erm, please don't change the code so much.  You need to replace *only*
the buffer allocation / free and the error path using goto instead of
the direct return.  If you need to modify other code, do it in another
patch.  In that way, it'll be clearer what each change means.


thanks,

Takashi

Re: [PATCH v2 1/2] Move DMA-buffer off the stack

[René Herman]

I'm afraid I'll now just go do other things again then; will keep things
local.

[PATCH v2 2/2] 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>
---
 sound/usb/6fire/pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
index 88ac1c4ee163..cce1312db93a 100644
--- a/sound/usb/6fire/pcm.c
+++ b/sound/usb/6fire/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