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

[Takashi Iwai <tiwai@suse.de>]

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