| Message ID | 20181211212318.28644-10-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sound Open Firmware (SOF) core | expand |
On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: > 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. > +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; > + > + int ret = 0; I don't see how it's used. Perhaps you need to check code with `make W=1`. > + 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 ret; > +} > + 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); Hmm... Why do we need a kernel level duplication in words? > +int snd_sof_load_firmware(struct snd_sof_dev *sdev) > +{ > + dev_dbg(sdev->dev, "loading firmware\n"); Noise. Better to introduce a trace points and drop all these kind of messages. > + > + if (sdev->ops->load_firmware) > + return sdev->ops->load_firmware(sdev); > + return 0; > +} > +EXPORT_SYMBOL(snd_sof_load_firmware);
On 12/11/18 4:38 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: >> 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. >> +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; >> + >> + int ret = 0; > I don't see how it's used. Perhaps you need to check code with `make W=1`. Darn, we missed this one. I thought we were using W=1 on github but we aren't, this will be fixed. Though W=1 doesn't report this one, so need to re-inspect this. Thanks for the sighting. > >> + 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 ret; >> +} >> + 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); > Hmm... Why do we need a kernel level duplication in words? Not sure I get this one, it's just a warning message where you don't copy a block of size zero into the target memory. > >> +int snd_sof_load_firmware(struct snd_sof_dev *sdev) >> +{ >> + dev_dbg(sdev->dev, "loading firmware\n"); > Noise. > Better to introduce a trace points and drop all these kind of messages. it's not that bad, most people understand what dmesg is, it happens once and only if you have dynamic debug. At some point we also thought about something like drm_debug where you can specify which parts you are interested in, in addition to the verbosity level. > >> + >> + if (sdev->ops->load_firmware) >> + return sdev->ops->load_firmware(sdev); >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_load_firmware);
On Tue, 11 Dec 2018 22:23:13 +0100, 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; > + > + dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n", > + module->size, module->num_blocks, module->type); > + > + block = (void *)module + sizeof(*module); > + > + for (count = 0; count < module->num_blocks; count++) { Need a sanity check that it won't go beyond the actual firmware size. User may pass a malicious module data, e.g. with extra large num_blocks. > + 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_BLK_IMAGE: > + case SOF_BLK_CACHE: > + case SOF_BLK_REGS: > + case SOF_BLK_SIG: > + case SOF_BLK_ROM: > + continue; /* not handled atm */ > + case SOF_BLK_TEXT: > + case SOF_BLK_DATA: > + 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); > + > + snd_sof_dsp_block_write(sdev, offset, > + (void *)block + sizeof(*block), > + block->size); > + > + /* next block */ > + block = (void *)block + sizeof(*block) + block->size; This may lead to an unaligned access. Also how is the endianess guaranteed? thanks, Takashi
On 12/12/18 5:23 AM, Takashi Iwai wrote: > On Tue, 11 Dec 2018 22:23:13 +0100, > 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; >> + >> + dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n", >> + module->size, module->num_blocks, module->type); >> + >> + block = (void *)module + sizeof(*module); >> + >> + for (count = 0; count < module->num_blocks; count++) { > Need a sanity check that it won't go beyond the actual firmware size. > User may pass a malicious module data, e.g. with extra large > num_blocks. Good point, will check. > >> + 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_BLK_IMAGE: >> + case SOF_BLK_CACHE: >> + case SOF_BLK_REGS: >> + case SOF_BLK_SIG: >> + case SOF_BLK_ROM: >> + continue; /* not handled atm */ >> + case SOF_BLK_TEXT: >> + case SOF_BLK_DATA: >> + 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); >> + >> + snd_sof_dsp_block_write(sdev, offset, >> + (void *)block + sizeof(*block), >> + block->size); >> + >> + /* next block */ >> + block = (void *)block + sizeof(*block) + block->size; > This may lead to an unaligned access. > Also how is the endianess guaranteed? Will check, valid points.
On Tue, Dec 11, 2018 at 05:54:02PM -0600, Pierre-Louis Bossart wrote: > On 12/11/18 4:38 PM, Andy Shevchenko wrote: > > On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: > > > + struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr; > > > + > > > + int ret = 0; > > I don't see how it's used. Perhaps you need to check code with `make W=1`. > Darn, we missed this one. I thought we were using W=1 on github but we > aren't, this will be fixed. > Though W=1 doesn't report this one, so need to re-inspect this. Thanks for > the sighting. This is one reason not to unconditionally init stuff on declaration - it masks some warnings. > > > +int snd_sof_load_firmware(struct snd_sof_dev *sdev) > > > +{ > > > + dev_dbg(sdev->dev, "loading firmware\n"); > > Noise. > > Better to introduce a trace points and drop all these kind of messages. > it's not that bad, most people understand what dmesg is, it happens once and > only if you have dynamic debug. Then everyone adds their print on boot and the log gets huge (and slow if you push debug through serial during development). As a rule of thumb I tend to suggest that if you're not reporting something you discovered at runtime there's probably already other ways of getting equivalent trace.
On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: > +int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev) > +{ > + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev); > + const char *fw_filename; > + int ret; This never actually calls the load_firmware() operation for the DSP AFAICT? > + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev); > + > + if (ret < 0) { > + dev_err(sdev->dev, "error: request firmware failed err: %d\n", > + ret); > + return ret; > + } I'd suggest logging the name of the firmware we tried to load, users will thank you. > + /* create fw_version debugfs to store boot version info */ > + if (sdev->first_boot) { > + ret = snd_sof_debugfs_buf_create_item(sdev, &sdev->fw_version, > + sizeof(sdev->fw_version), > + "fw_version"); > + > + if (ret < 0) { > + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\n"); > + return ret; > + } > + } As Andy said elsewhere debugfs stuff like that probably shouldn't be fatal.
Thanks for the reviews Mark, much appreciated. +int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev) >> +{ >> + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev); >> + const char *fw_filename; >> + int ret; > This never actually calls the load_firmware() operation for the DSP > AFAICT? it's actually the implementation of the load_firmware callback, see e.g. for Baytrail/Broadwell /*Firmware loading */ .load_firmware = snd_sof_load_firmware_memcpy, > >> + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: request firmware failed err: %d\n", >> + ret); >> + return ret; >> + } > I'd suggest logging the name of the firmware we tried to load, users > will thank you. yes indeed, thanks for the suggestion, will add this right away. > >> + /* create fw_version debugfs to store boot version info */ >> + if (sdev->first_boot) { >> + ret = snd_sof_debugfs_buf_create_item(sdev, &sdev->fw_version, >> + sizeof(sdev->fw_version), >> + "fw_version"); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\n"); >> + return ret; >> + } >> + } > As Andy said elsewhere debugfs stuff like that probably shouldn't be > fatal. yes, we've fixed this already.
diff --git a/sound/soc/sof/loader.c b/sound/soc/sof/loader.c new file mode 100644 index 000000000000..dbadd8c521e6 --- /dev/null +++ b/sound/soc/sof/loader.c @@ -0,0 +1,322 @@ +// 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; + + int ret = 0; + 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 ret; +} + +/* parse the extended FW boot data structures from FW boot message */ +int snd_sof_fw_parse_ext_data(struct snd_sof_dev *sdev, 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, 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, offset, + 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); + goto out; + } + + /* move to next header */ + offset += ext_hdr->hdr.size; + snd_sof_dsp_block_read(sdev, offset, ext_data, + sizeof(*ext_hdr)); + ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data; + } +out: + 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; + + dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n", + module->size, module->num_blocks, module->type); + + block = (void *)module + sizeof(*module); + + for (count = 0; count < module->num_blocks; count++) { + 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_BLK_IMAGE: + case SOF_BLK_CACHE: + case SOF_BLK_REGS: + case SOF_BLK_SIG: + case SOF_BLK_ROM: + continue; /* not handled atm */ + case SOF_BLK_TEXT: + case SOF_BLK_DATA: + 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); + + snd_sof_dsp_block_write(sdev, offset, + (void *)block + sizeof(*block), + block->size); + + /* next block */ + block = (void *)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; + + header = (struct snd_sof_fw_header *)fw->data; + load_module = sdev->ops->load_module; + if (!load_module) + return -EINVAL; + + /* parse each module */ + module = (void *)fw->data + sizeof(*header); + for (count = 0; count < header->num_modules; count++) { + /* module */ + ret = load_module(sdev, module); + if (ret < 0) { + dev_err(sdev->dev, "error: invalid module %d\n", count); + return ret; + } + module = (void *)module + sizeof(*module) + module->size; + } + + return 0; +} + +int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev) +{ + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev); + const char *fw_filename; + int ret; + + /* set code loading condition to true */ + sdev->code_loading = 1; + fw_filename = plat_data->machine->sof_fw_filename; + + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev); + + if (ret < 0) { + dev_err(sdev->dev, "error: request firmware failed err: %d\n", + ret); + 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); + 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 (sdev->ops->load_firmware) + return sdev->ops->load_firmware(sdev); + return 0; +} +EXPORT_SYMBOL(snd_sof_load_firmware); + +int snd_sof_run_firmware(struct snd_sof_dev *sdev) +{ + int ret; + + 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_create_item(sdev, &sdev->fw_version, + sizeof(sdev->fw_version), + "fw_version"); + + if (ret < 0) { + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\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; + } + + /* 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 timeout\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; + } + + 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);