diff mbox series

[v5,09/14] ASoC: SOF: Add firmware loader support

Message ID 20190321161016.26515-10-pierre-louis.bossart@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: Sound Open Firmware (SOF) core | expand

Commit Message

Pierre-Louis Bossart March 21, 2019, 4:10 p.m. UTC
From: Liam Girdwood <liam.r.girdwood@linux.intel.com>

The firmware loader exports APIs that can be called by core to load and
process multiple different file formats.

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/loader.c | 376 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 376 insertions(+)
 create mode 100644 sound/soc/sof/loader.c

Comments

Takashi Iwai April 4, 2019, 1:25 p.m. UTC | #1
On Thu, 21 Mar 2019 17:10:11 +0100,
Pierre-Louis Bossart wrote:
> 
> +static int get_ext_windows(struct snd_sof_dev *sdev,
> +			   struct sof_ipc_ext_data_hdr *ext_hdr)
> +{
> +	struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr;

In general it's safer to use container_of() than the forced cast, and...

> +/* parse the extended FW boot data structures from FW boot message */
> +int snd_sof_fw_parse_ext_data(struct snd_sof_dev *sdev, u32 bar, u32 offset)
> +{
> +	struct sof_ipc_ext_data_hdr *ext_hdr;
> +	void *ext_data;
> +	int ret = 0;
> +
> +	ext_data = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!ext_data)
> +		return -ENOMEM;
> +
> +	/* get first header */
> +	snd_sof_dsp_block_read(sdev, bar, offset, ext_data,
> +			       sizeof(*ext_hdr));
> +	ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data;

The cast isn't needed from a void pointer.  The whole SOF codes have
way too many unneeded cast.  Let's reduce.

> +/* generic module parser for mmaped DSPs */
> +int snd_sof_parse_module_memcpy(struct snd_sof_dev *sdev,
> +				struct snd_sof_mod_hdr *module)
> +{
> +	struct snd_sof_blk_hdr *block;
> +	int count;
> +	u32 offset;
> +	size_t remaining;
> +
> +	dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n",
> +		module->size, module->num_blocks, module->type);
> +
> +	block = (struct snd_sof_blk_hdr *)((u8 *)module + sizeof(*module));
> +
> +	/* module->size doesn't include header size */
> +	remaining = module->size;
> +	for (count = 0; count < module->num_blocks; count++) {
> +		/* minus header size of block */
> +		remaining -= sizeof(*block);
> +		if (remaining < block->size) {
> +			dev_err(sdev->dev, "error: not enough data remaining\n");
> +			return -EINVAL;
> +		}

remaining is unsigned, so a negative check doesn't work here.
Hence you need the explicit underflow check.

> +static int load_modules(struct snd_sof_dev *sdev, const struct firmware *fw)
> +{
> +	struct snd_sof_fw_header *header;
> +	struct snd_sof_mod_hdr *module;
> +	int (*load_module)(struct snd_sof_dev *sof_dev,
> +			   struct snd_sof_mod_hdr *hdr);
> +	int ret, count;
> +	size_t remaining;
> +
> +	header = (struct snd_sof_fw_header *)fw->data;
> +	load_module = sof_ops(sdev)->load_module;
> +	if (!load_module)
> +		return -EINVAL;
> +
> +	/* parse each module */
> +	module = (struct snd_sof_mod_hdr *)((u8 *)(fw->data) + sizeof(*header));
> +	remaining = fw->size - sizeof(*header);

The size check should be more strict (e.g. here remaining might become
negative).

> +int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
> +{
> +	struct snd_sof_pdata *plat_data = sdev->pdata;
> +	const char *fw_filename;
> +	int ret;
> +
> +	/* set code loading condition to true */
> +	sdev->code_loading = 1;
> +
> +	/* Don't request firmware again if firmware is already requested */
> +	if (plat_data->fw)
> +		return 0;
> +
> +	fw_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
> +				     "%s/%s",
> +				     plat_data->fw_filename_prefix,
> +				     plat_data->fw_filename);
> +	if (!fw_filename)
> +		return -ENOMEM;
> +
> +	ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev);
> +
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: request firmware %s failed err: %d\n",
> +			fw_filename, ret);
> +	}
> +	return ret;

Hrm, any need of devm_kasprintf() here?  That is, do we need to keep
this string after the call of request_firmware()?  It doesn't make
sense if we need it only temporarily.


thanks,

Takashi
Pierre-Louis Bossart April 4, 2019, 1:59 p.m. UTC | #2
>> +static int get_ext_windows(struct snd_sof_dev *sdev,
>> +			   struct sof_ipc_ext_data_hdr *ext_hdr)
>> +{
>> +	struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr;
> 
> In general it's safer to use container_of() than the forced cast, and...

yes

> 
>> +/* parse the extended FW boot data structures from FW boot message */
>> +int snd_sof_fw_parse_ext_data(struct snd_sof_dev *sdev, u32 bar, u32 offset)
>> +{
>> +	struct sof_ipc_ext_data_hdr *ext_hdr;
>> +	void *ext_data;
>> +	int ret = 0;
>> +
>> +	ext_data = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +	if (!ext_data)
>> +		return -ENOMEM;
>> +
>> +	/* get first header */
>> +	snd_sof_dsp_block_read(sdev, bar, offset, ext_data,
>> +			       sizeof(*ext_hdr));
>> +	ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data;
> 
> The cast isn't needed from a void pointer.  The whole SOF codes have
> way too many unneeded cast.  Let's reduce.

ok. we've removed quite a few casts but that's obviously not finished.

> 
>> +/* generic module parser for mmaped DSPs */
>> +int snd_sof_parse_module_memcpy(struct snd_sof_dev *sdev,
>> +				struct snd_sof_mod_hdr *module)
>> +{
>> +	struct snd_sof_blk_hdr *block;
>> +	int count;
>> +	u32 offset;
>> +	size_t remaining;
>> +
>> +	dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n",
>> +		module->size, module->num_blocks, module->type);
>> +
>> +	block = (struct snd_sof_blk_hdr *)((u8 *)module + sizeof(*module));
>> +
>> +	/* module->size doesn't include header size */
>> +	remaining = module->size;
>> +	for (count = 0; count < module->num_blocks; count++) {
>> +		/* minus header size of block */
>> +		remaining -= sizeof(*block);
>> +		if (remaining < block->size) {
>> +			dev_err(sdev->dev, "error: not enough data remaining\n");
>> +			return -EINVAL;
>> +		}
> 
> remaining is unsigned, so a negative check doesn't work here.
> Hence you need the explicit underflow check.

yes, probably need ssize_t here.

> 
>> +static int load_modules(struct snd_sof_dev *sdev, const struct firmware *fw)
>> +{
>> +	struct snd_sof_fw_header *header;
>> +	struct snd_sof_mod_hdr *module;
>> +	int (*load_module)(struct snd_sof_dev *sof_dev,
>> +			   struct snd_sof_mod_hdr *hdr);
>> +	int ret, count;
>> +	size_t remaining;
>> +
>> +	header = (struct snd_sof_fw_header *)fw->data;
>> +	load_module = sof_ops(sdev)->load_module;
>> +	if (!load_module)
>> +		return -EINVAL;
>> +
>> +	/* parse each module */
>> +	module = (struct snd_sof_mod_hdr *)((u8 *)(fw->data) + sizeof(*header));
>> +	remaining = fw->size - sizeof(*header);
> 
> The size check should be more strict (e.g. here remaining might become
> negative).

yes.

> 
>> +int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
>> +{
>> +	struct snd_sof_pdata *plat_data = sdev->pdata;
>> +	const char *fw_filename;
>> +	int ret;
>> +
>> +	/* set code loading condition to true */
>> +	sdev->code_loading = 1;
>> +
>> +	/* Don't request firmware again if firmware is already requested */
>> +	if (plat_data->fw)
>> +		return 0;
>> +
>> +	fw_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> +				     "%s/%s",
>> +				     plat_data->fw_filename_prefix,
>> +				     plat_data->fw_filename);
>> +	if (!fw_filename)
>> +		return -ENOMEM;
>> +
>> +	ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev);
>> +
>> +	if (ret < 0) {
>> +		dev_err(sdev->dev, "error: request firmware %s failed err: %d\n",
>> +			fw_filename, ret);
>> +	}
>> +	return ret;
> 
> Hrm, any need of devm_kasprintf() here?  That is, do we need to keep
> this string after the call of request_firmware()?  It doesn't make
> sense if we need it only temporarily.

Good point, this can be simplified.
Takashi Iwai April 4, 2019, 2:05 p.m. UTC | #3
On Thu, 04 Apr 2019 15:59:46 +0200,
Pierre-Louis Bossart wrote:
> 
> >> +/* generic module parser for mmaped DSPs */
> >> +int snd_sof_parse_module_memcpy(struct snd_sof_dev *sdev,
> >> +				struct snd_sof_mod_hdr *module)
> >> +{
> >> +	struct snd_sof_blk_hdr *block;
> >> +	int count;
> >> +	u32 offset;
> >> +	size_t remaining;
> >> +
> >> +	dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n",
> >> +		module->size, module->num_blocks, module->type);
> >> +
> >> +	block = (struct snd_sof_blk_hdr *)((u8 *)module + sizeof(*module));
> >> +
> >> +	/* module->size doesn't include header size */
> >> +	remaining = module->size;
> >> +	for (count = 0; count < module->num_blocks; count++) {
> >> +		/* minus header size of block */
> >> +		remaining -= sizeof(*block);
> >> +		if (remaining < block->size) {
> >> +			dev_err(sdev->dev, "error: not enough data remaining\n");
> >> +			return -EINVAL;
> >> +		}
> >
> > remaining is unsigned, so a negative check doesn't work here.
> > Hence you need the explicit underflow check.
> 
> yes, probably need ssize_t here.

Be careful.  If block->size is unsigned, the comparison is also done
as unsigned in the code above.


Takashi
diff mbox series

Patch

diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c
new file mode 100644
index 000000000000..657c0dd5c013
--- /dev/null
+++ b/sound/soc/sof/loader.c
@@ -0,0 +1,376 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+//
+// This file is provided under a dual BSD/GPLv2 license.  When using or
+// redistributing this file, you may do so under either license.
+//
+// Copyright(c) 2018 Intel Corporation. All rights reserved.
+//
+// Author: Liam Girdwood <liam.r.girdwood@linux.intel.com>
+//
+// Generic firmware loader.
+//
+
+#include <linux/firmware.h>
+#include <sound/sof.h>
+#include "ops.h"
+
+static int get_ext_windows(struct snd_sof_dev *sdev,
+			   struct sof_ipc_ext_data_hdr *ext_hdr)
+{
+	struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr;
+	size_t size;
+
+	if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS)
+		return -EINVAL;
+
+	size = sizeof(*w) + sizeof(struct sof_ipc_window_elem) * w->num_windows;
+
+	/* keep a local copy of the data */
+	sdev->info_window = kmemdup(w, size, GFP_KERNEL);
+	if (!sdev->info_window)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/* parse the extended FW boot data structures from FW boot message */
+int snd_sof_fw_parse_ext_data(struct snd_sof_dev *sdev, u32 bar, u32 offset)
+{
+	struct sof_ipc_ext_data_hdr *ext_hdr;
+	void *ext_data;
+	int ret = 0;
+
+	ext_data = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!ext_data)
+		return -ENOMEM;
+
+	/* get first header */
+	snd_sof_dsp_block_read(sdev, bar, offset, ext_data,
+			       sizeof(*ext_hdr));
+	ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data;
+
+	while (ext_hdr->hdr.cmd == SOF_IPC_FW_READY) {
+		/* read in ext structure */
+		offset += sizeof(*ext_hdr);
+		snd_sof_dsp_block_read(sdev, bar, offset,
+				   (void *)((u8 *)ext_data + sizeof(*ext_hdr)),
+				   ext_hdr->hdr.size - sizeof(*ext_hdr));
+
+		dev_dbg(sdev->dev, "found ext header type %d size 0x%x\n",
+			ext_hdr->type, ext_hdr->hdr.size);
+
+		/* process structure data */
+		switch (ext_hdr->type) {
+		case SOF_IPC_EXT_DMA_BUFFER:
+			break;
+		case SOF_IPC_EXT_WINDOW:
+			ret = get_ext_windows(sdev, ext_hdr);
+			break;
+		default:
+			break;
+		}
+
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: failed to parse ext data type %d\n",
+				ext_hdr->type);
+			break;
+		}
+
+		/* move to next header */
+		offset += ext_hdr->hdr.size;
+		snd_sof_dsp_block_read(sdev, bar, offset, ext_data,
+				       sizeof(*ext_hdr));
+		ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data;
+	}
+
+	kfree(ext_data);
+	return ret;
+}
+EXPORT_SYMBOL(snd_sof_fw_parse_ext_data);
+
+/* generic module parser for mmaped DSPs */
+int snd_sof_parse_module_memcpy(struct snd_sof_dev *sdev,
+				struct snd_sof_mod_hdr *module)
+{
+	struct snd_sof_blk_hdr *block;
+	int count;
+	u32 offset;
+	size_t remaining;
+
+	dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n",
+		module->size, module->num_blocks, module->type);
+
+	block = (struct snd_sof_blk_hdr *)((u8 *)module + sizeof(*module));
+
+	/* module->size doesn't include header size */
+	remaining = module->size;
+	for (count = 0; count < module->num_blocks; count++) {
+		/* minus header size of block */
+		remaining -= sizeof(*block);
+		if (remaining < block->size) {
+			dev_err(sdev->dev, "error: not enough data remaining\n");
+			return -EINVAL;
+		}
+
+		if (block->size == 0) {
+			dev_warn(sdev->dev,
+				 "warning: block %d size zero\n", count);
+			dev_warn(sdev->dev, " type 0x%x offset 0x%x\n",
+				 block->type, block->offset);
+			continue;
+		}
+
+		switch (block->type) {
+		case SOF_FW_BLK_TYPE_RSRVD0:
+		case SOF_FW_BLK_TYPE_SRAM...SOF_FW_BLK_TYPE_RSRVD14:
+			continue;	/* not handled atm */
+		case SOF_FW_BLK_TYPE_IRAM:
+		case SOF_FW_BLK_TYPE_DRAM:
+			offset = block->offset;
+			break;
+		default:
+			dev_err(sdev->dev, "error: bad type 0x%x for block 0x%x\n",
+				block->type, count);
+			return -EINVAL;
+		}
+
+		dev_dbg(sdev->dev,
+			"block %d type 0x%x size 0x%x ==>  offset 0x%x\n",
+			count, block->type, block->size, offset);
+
+		/* checking block->size to avoid unaligned access */
+		if (block->size % sizeof(u32)) {
+			dev_err(sdev->dev, "error: invalid block size 0x%x\n",
+				block->size);
+			return -EINVAL;
+		}
+		snd_sof_dsp_block_write(sdev, sdev->mmio_bar, offset,
+					(void *)((u8 *)block + sizeof(*block)),
+					block->size);
+
+		/* minus body size of block */
+		remaining -= block->size;
+		/* next block */
+		block = (struct snd_sof_blk_hdr *)((u8 *)block + sizeof(*block)
+			+ block->size);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(snd_sof_parse_module_memcpy);
+
+static int check_header(struct snd_sof_dev *sdev, const struct firmware *fw)
+{
+	struct snd_sof_fw_header *header;
+
+	/* Read the header information from the data pointer */
+	header = (struct snd_sof_fw_header *)fw->data;
+
+	/* verify FW sig */
+	if (strncmp(header->sig, SND_SOF_FW_SIG, SND_SOF_FW_SIG_SIZE) != 0) {
+		dev_err(sdev->dev, "error: invalid firmware signature\n");
+		return -EINVAL;
+	}
+
+	/* check size is valid */
+	if (fw->size != header->file_size + sizeof(*header)) {
+		dev_err(sdev->dev, "error: invalid filesize mismatch got 0x%zx expected 0x%zx\n",
+			fw->size, header->file_size + sizeof(*header));
+		return -EINVAL;
+	}
+
+	dev_dbg(sdev->dev, "header size=0x%x modules=0x%x abi=0x%x size=%zu\n",
+		header->file_size, header->num_modules,
+		header->abi, sizeof(*header));
+
+	return 0;
+}
+
+static int load_modules(struct snd_sof_dev *sdev, const struct firmware *fw)
+{
+	struct snd_sof_fw_header *header;
+	struct snd_sof_mod_hdr *module;
+	int (*load_module)(struct snd_sof_dev *sof_dev,
+			   struct snd_sof_mod_hdr *hdr);
+	int ret, count;
+	size_t remaining;
+
+	header = (struct snd_sof_fw_header *)fw->data;
+	load_module = sof_ops(sdev)->load_module;
+	if (!load_module)
+		return -EINVAL;
+
+	/* parse each module */
+	module = (struct snd_sof_mod_hdr *)((u8 *)(fw->data) + sizeof(*header));
+	remaining = fw->size - sizeof(*header);
+	for (count = 0; count < header->num_modules; count++) {
+		/* minus header size of module */
+		remaining -= sizeof(*module);
+		if (remaining < module->size) {
+			dev_err(sdev->dev, "error: not enough data remaining\n");
+			return -EINVAL;
+		}
+		/* module */
+		ret = load_module(sdev, module);
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: invalid module %d\n", count);
+			return ret;
+		}
+		/* minus body size of module */
+		remaining -=  module->size;
+		module = (struct snd_sof_mod_hdr *)((u8 *)module
+			+ sizeof(*module) + module->size);
+	}
+
+	return 0;
+}
+
+int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
+{
+	struct snd_sof_pdata *plat_data = sdev->pdata;
+	const char *fw_filename;
+	int ret;
+
+	/* set code loading condition to true */
+	sdev->code_loading = 1;
+
+	/* Don't request firmware again if firmware is already requested */
+	if (plat_data->fw)
+		return 0;
+
+	fw_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
+				     "%s/%s",
+				     plat_data->fw_filename_prefix,
+				     plat_data->fw_filename);
+	if (!fw_filename)
+		return -ENOMEM;
+
+	ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev);
+
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: request firmware %s failed err: %d\n",
+			fw_filename, ret);
+	}
+	return ret;
+}
+EXPORT_SYMBOL(snd_sof_load_firmware_raw);
+
+int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev)
+{
+	struct snd_sof_pdata *plat_data = sdev->pdata;
+	int ret;
+
+	ret = snd_sof_load_firmware_raw(sdev);
+	if (ret < 0)
+		return ret;
+
+	/* make sure the FW header and file is valid */
+	ret = check_header(sdev, plat_data->fw);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: invalid FW header\n");
+		goto error;
+	}
+
+	/* prepare the DSP for FW loading */
+	ret = snd_sof_dsp_reset(sdev);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: failed to reset DSP\n");
+		goto error;
+	}
+
+	/* parse and load firmware modules to DSP */
+	ret = load_modules(sdev, plat_data->fw);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: invalid FW modules\n");
+		goto error;
+	}
+
+	return 0;
+
+error:
+	release_firmware(plat_data->fw);
+	plat_data->fw = NULL;
+	return ret;
+
+}
+EXPORT_SYMBOL(snd_sof_load_firmware_memcpy);
+
+int snd_sof_load_firmware(struct snd_sof_dev *sdev)
+{
+	dev_dbg(sdev->dev, "loading firmware\n");
+
+	if (sof_ops(sdev)->load_firmware)
+		return sof_ops(sdev)->load_firmware(sdev);
+	return 0;
+}
+EXPORT_SYMBOL(snd_sof_load_firmware);
+
+int snd_sof_run_firmware(struct snd_sof_dev *sdev)
+{
+	int ret;
+	int init_core_mask;
+
+	init_waitqueue_head(&sdev->boot_wait);
+	sdev->boot_complete = false;
+
+	/* create fw_version debugfs to store boot version info */
+	if (sdev->first_boot) {
+		ret = snd_sof_debugfs_buf_item(sdev, &sdev->fw_version,
+					       sizeof(sdev->fw_version),
+					       "fw_version");
+		/* errors are only due to memory allocation, not debugfs */
+		if (ret < 0) {
+			dev_err(sdev->dev, "error: snd_sof_debugfs_buf_item failed\n");
+			return ret;
+		}
+	}
+
+	/* perform pre fw run operations */
+	ret = snd_sof_dsp_pre_fw_run(sdev);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: failed pre fw run op\n");
+		return ret;
+	}
+
+	dev_dbg(sdev->dev, "booting DSP firmware\n");
+
+	/* boot the firmware on the DSP */
+	ret = snd_sof_dsp_run(sdev);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: failed to reset DSP\n");
+		return ret;
+	}
+
+	init_core_mask = ret;
+
+	/* now wait for the DSP to boot */
+	ret = wait_event_timeout(sdev->boot_wait, sdev->boot_complete,
+				 msecs_to_jiffies(sdev->boot_timeout));
+	if (ret == 0) {
+		dev_err(sdev->dev, "error: firmware boot failure\n");
+		snd_sof_dsp_dbg_dump(sdev, SOF_DBG_REGS | SOF_DBG_MBOX |
+			SOF_DBG_TEXT | SOF_DBG_PCI);
+		return -EIO;
+	}
+
+	dev_info(sdev->dev, "firmware boot complete\n");
+
+	/* perform post fw run operations */
+	ret = snd_sof_dsp_post_fw_run(sdev);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: failed post fw run op\n");
+		return ret;
+	}
+
+	/* fw boot is complete. Update the active cores mask */
+	sdev->enabled_cores_mask = init_core_mask;
+
+	return 0;
+}
+EXPORT_SYMBOL(snd_sof_run_firmware);
+
+void snd_sof_fw_unload(struct snd_sof_dev *sdev)
+{
+	/* TODO: support module unloading at runtime */
+}
+EXPORT_SYMBOL(snd_sof_fw_unload);