* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support [not found] <mailman.3934.1544619824.753.alsa-devel@alsa-project.org> @ 2018-12-20 2:11 ` Bard liao 2018-12-20 8:16 ` Takashi Iwai 0 siblings, 1 reply; 13+ messages in thread From: Bard liao @ 2018-12-20 2:11 UTC (permalink / raw) To: Takashi Iwai, Pierre-Louis Bossart Cc: daniel.baluta, andriy.shevchenko, alsa-devel, liam.r.girdwood, vkoul, broonie, sound-open-firmware, alan > Date: Wed, 12 Dec 2018 12:23:33 +0100 > From: Takashi Iwai <tiwai@suse.de> > To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: alsa-devel@alsa-project.org, andriy.shevchenko@intel.com, Daniel > Baluta <daniel.baluta@gmail.com>, liam.r.girdwood@linux.intel.com, > vkoul@kernel.org, broonie@kernel.org, Alan Cox <alan@linux.intel.com>, > sound-open-firmware@alsa-project.org > Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware > loader support > Message-ID: <s5htvjjj8ei.wl-tiwai@suse.de> > Content-Type: text/plain; charset=US-ASCII > > On Tue, 11 Dec 2018 22:23:13 +0100, > Pierre-Louis Bossart wrote: > >> + >> + 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. Did you mean we should double check the block->size to prevent access to an invalid address? > Also how is the endianess guaranteed? Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it? > > > thanks, > > Takashi > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support 2018-12-20 2:11 ` [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support Bard liao @ 2018-12-20 8:16 ` Takashi Iwai 2018-12-20 15:07 ` Pierre-Louis Bossart 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2018-12-20 8:16 UTC (permalink / raw) To: Bard liao Cc: daniel.baluta, andriy.shevchenko, alsa-devel, Pierre-Louis Bossart, liam.r.girdwood, vkoul, broonie, sound-open-firmware, alan On Thu, 20 Dec 2018 03:11:55 +0100, Bard liao wrote: > > > > Date: Wed, 12 Dec 2018 12:23:33 +0100 > > From: Takashi Iwai <tiwai@suse.de> > > To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Cc: alsa-devel@alsa-project.org, andriy.shevchenko@intel.com, Daniel > > Baluta <daniel.baluta@gmail.com>, liam.r.girdwood@linux.intel.com, > > vkoul@kernel.org, broonie@kernel.org, Alan Cox <alan@linux.intel.com>, > > sound-open-firmware@alsa-project.org > > Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware > > loader support > > Message-ID: <s5htvjjj8ei.wl-tiwai@suse.de> > > Content-Type: text/plain; charset=US-ASCII > > > > On Tue, 11 Dec 2018 22:23:13 +0100, > > Pierre-Louis Bossart wrote: > > > >> + > >> + 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. > > Did you mean we should double check the block->size to > prevent access to an invalid address? You need two types of checks for the given data: - The bounce check of block->size; We need to avoid out-of-bounce access. - Alignment of block->size; For some non-x86 platforms, the access to an unaligned address might be illegal. Oh, and recently another thing is sometimes needed for avoiding Spectre. This can be covered by array_index_nospec(). > > Also how is the endianess guaranteed? > > Did you mean we should guarantee the driver can work no > matter what kernel's endianess is? > ie. Use le32_to_cpu() to handle it? Depends on the implementation. IIRC, topology API refuses the data in a different endianess by checking the magic number at beginning. thanks, Takashi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support 2018-12-20 8:16 ` Takashi Iwai @ 2018-12-20 15:07 ` Pierre-Louis Bossart 2018-12-21 9:05 ` Bard liao 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2018-12-20 15:07 UTC (permalink / raw) To: Takashi Iwai, Bard liao Cc: daniel.baluta, andriy.shevchenko, alsa-devel, liam.r.girdwood, vkoul, broonie, sound-open-firmware, alan >>>> + 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. >> Did you mean we should double check the block->size to >> prevent access to an invalid address? > You need two types of checks for the given data: > - The bounce check of block->size; > We need to avoid out-of-bounce access. s/bounce/bounds ? > > - Alignment of block->size; > For some non-x86 platforms, the access to an unaligned address might > be illegal. Maybe I am missing something but I don't see any sort of explicit restriction on alignment in the SOF tools. it looks implicit based on address offsets and bases. Liam, do you see any negative side effects if we enforce a 32-bit alignment for all blocks (which essentially means all block sizes are multiple of 4)? we can try and experiment but it's better if we have an agreement on the design. > > Oh, and recently another thing is sometimes needed for avoiding > Spectre. This can be covered by array_index_nospec(). > > >>> Also how is the endianess guaranteed? >> Did you mean we should guarantee the driver can work no >> matter what kernel's endianess is? >> ie. Use le32_to_cpu() to handle it? > Depends on the implementation. IIRC, topology API refuses the data > in a different endianess by checking the magic number at beginning. snd_sof_dsp_block_write() is implemented with a platform-specific callback, I'd expect any endianess issues to be handled in that platform-specific code? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support 2018-12-20 15:07 ` Pierre-Louis Bossart @ 2018-12-21 9:05 ` Bard liao 2018-12-21 9:57 ` Takashi Iwai 0 siblings, 1 reply; 13+ messages in thread From: Bard liao @ 2018-12-21 9:05 UTC (permalink / raw) To: Pierre-Louis Bossart, Takashi Iwai Cc: daniel.baluta, andriy.shevchenko, alsa-devel, liam.r.girdwood, vkoul, broonie, alan, sound-open-firmware On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote: > >>>>> + 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. >>> Did you mean we should double check the block->size to >>> prevent access to an invalid address? >> You need two types of checks for the given data: >> - The bounce check of block->size; >> We need to avoid out-of-bounce access. > s/bounce/bounds ? >> >> - Alignment of block->size; >> For some non-x86 platforms, the access to an unaligned address might >> be illegal. > > Maybe I am missing something but I don't see any sort of explicit > restriction on alignment in the SOF tools. it looks implicit based on > address offsets and bases. Maybe get_unaligned() is good enough to avoid unaligned access? > > Liam, do you see any negative side effects if we enforce a 32-bit > alignment for all blocks (which essentially means all block sizes are > multiple of 4)? we can try and experiment but it's better if we have > an agreement on the design. > > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support 2018-12-21 9:05 ` Bard liao @ 2018-12-21 9:57 ` Takashi Iwai 2018-12-21 15:43 ` Liam Girdwood 0 siblings, 1 reply; 13+ messages in thread From: Takashi Iwai @ 2018-12-21 9:57 UTC (permalink / raw) To: Bard liao Cc: daniel.baluta, andriy.shevchenko, alsa-devel, Pierre-Louis Bossart, liam.r.girdwood, vkoul, broonie, alan, sound-open-firmware On Fri, 21 Dec 2018 10:05:04 +0100, Bard liao wrote: > > > On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote: > > > >>>>> + 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. > >>> Did you mean we should double check the block->size to > >>> prevent access to an invalid address? > >> You need two types of checks for the given data: > >> - The bounce check of block->size; > >> We need to avoid out-of-bounce access. > > s/bounce/bounds ? > >> > >> - Alignment of block->size; > >> For some non-x86 platforms, the access to an unaligned address might > >> be illegal. > > > > Maybe I am missing something but I don't see any sort of explicit > > restriction on alignment in the SOF tools. it looks implicit based > > on address offsets and bases. > > Maybe get_unaligned() is good enough to avoid unaligned access? That's another option, but you'd need to put everywhere. An alternative is to just copy the data on a new block header on the stack temporarily and refer it. Or simply assuring the alignment by checking block->size as Pierre suggested... Takashi > > > > > Liam, do you see any negative side effects if we enforce a 32-bit > > alignment for all blocks (which essentially means all block sizes > > are multiple of 4)? we can try and experiment but it's better if we > > have an agreement on the design. > > > > > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support 2018-12-21 9:57 ` Takashi Iwai @ 2018-12-21 15:43 ` Liam Girdwood 0 siblings, 0 replies; 13+ messages in thread From: Liam Girdwood @ 2018-12-21 15:43 UTC (permalink / raw) To: Takashi Iwai, Bard liao Cc: daniel.baluta, andriy.shevchenko, alsa-devel, Pierre-Louis Bossart, vkoul, broonie, sound-open-firmware, alan On Fri, 2018-12-21 at 10:57 +0100, Takashi Iwai wrote: > On Fri, 21 Dec 2018 10:05:04 +0100, > Bard liao wrote: > > > > On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote: > > > > > > > + 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. > > > > > Did you mean we should double check the block->size to > > > > > prevent access to an invalid address? > > > > You need two types of checks for the given data: > > > > - The bounce check of block->size; > > > > We need to avoid out-of-bounce access. > > > s/bounce/bounds ? > > > > - Alignment of block->size; > > > > For some non-x86 platforms, the access to an unaligned address might > > > > be illegal. > > > > > > Maybe I am missing something but I don't see any sort of explicit > > > restriction on alignment in the SOF tools. it looks implicit based > > > on address offsets and bases. > > > > Maybe get_unaligned() is good enough to avoid unaligned access? > > That's another option, but you'd need to put everywhere. > > An alternative is to just copy the data on a new block header on the > stack temporarily and refer it. > > Or simply assuring the alignment by checking block->size as Pierre > suggested... Yep, checking size is good for me too. > > > Takashi > > > > > Liam, do you see any negative side effects if we enforce a 32-bit > > > alignment for all blocks (which essentially means all block sizes > > > are multiple of 4)? we can try and experiment but it's better if we > > > have an agreement on the design. This shouldn't cause any problems, iirc all block sizes are rounded up to nearest uint32_t anyway (as we have exception handler blocks that are 3 bytes of assembly in size and rounded up to 4). Liam > > > > > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 00/14] Sound Open Firmware (SOF) core @ 2018-12-11 21:23 Pierre-Louis Bossart 2018-12-11 21:23 ` [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Pierre-Louis Bossart 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2018-12-11 21:23 UTC (permalink / raw) To: alsa-devel Cc: Daniel Baluta, andriy.shevchenko, tiwai, Pierre-Louis Bossart, liam.r.girdwood, vkoul, broonie, Alan Cox, sound-open-firmware Sound Open Firmware (SOF) is a host and DSP architecture agnostic audio DSP firmware. SOF is not tied to any specific host architecture or any specific physical IO communication type (it will work with on SoC DSPs, or DSP connected via SPI/I2C). SOF is also not coupled to any particular DSP architecture and has abstraction similar to Linux to allow porting to other DSP architectures. https://www.sofproject.org/ This patch series introduces the SOF core and utilities. Support for Intel devices is provided as a follow-up series. The SOF core manages all the core DSP services and ALSA/ASoC IO. The core is responsible for loading firmware, parsing topology, exposing PCMs and kcontrols, providing debug and trace mechanisms and performing IPC between host and DSP. The SOF core also has logic to allow reuse of existing machine drivers for other platforms/machines without any code modification. i.e. DAI links can be modified at runtime to bind with SOF and SOF topologies instead of existing hard coded DAI links and topology. Main changes since v2 (end of August): There are honestly too many changes to be listed... The biggest changes are the introduction of an ABI check and runtime_pm, as well as countless improvements coming from product validation. We will provide a revision history starting with the next update. Precisions: Since I've been maintaining the SOF kernel parts for most of 2018, Liam and I mutually agreed that it would be more efficient for me to send the patches upstream to avoid any disconnects. The code in this patchset is directly squashed from the SOF development branch [1], which tracks Mark Brown's for-next branch on a weekly basis and the configurations used for testing are based on the defconfigs at [2] Full disclosure on known limitation and issues (full list at [3]) a) the interaction with ASoC is not always perfect, there are a couple of points where a better solution is desired (use of private data, support for link DMA, etc). The known points are explicitly documented in the code and will be updated. To be clearer, it's our belief that SOF does not cripple ASoC in any way, and that merging this SOF core does not harm others. It's just complicated to get things right, especially on a couple of cases where the topology and DPCM frameworks generate complex flows that very few people understand. b) The get/put methods for controls generate an IPC and possibly a wake-up. This is not optimal and is being fixed. c) The debugfs parts will be updated for runtime_pm parts d) The topology free, firmware release and Hdaudio exit sequences will be updated. It is our intention to fully support module load/unload and free all resources. e) runtime_pm is being hardened and the use of SMART_SUSPEND was suggested. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre [1] https://github.com/thesofproject/linux [2] https://github.com/thesofproject/kconfig [3] https://github.com/thesofproject/linux/issues Liam Girdwood (12): ASoC: SOF: Add Sound Open Firmware driver core ASoC: SOF: Add Sound Open Firmware KControl support ASoC: SOF: Add driver debug support. ASoC: SOF: Add support for IPC IO between DSP and Host ASoC: SOF: Add PCM operations support ASoC: SOF: Add support for loading topologies ASoC: SOF: Add DSP firmware logger support ASoC: SOF: Add DSP HW abstraction operations ASoC: SOF: Add firmware loader support ASoC: SOF: Add userspace ABI support ASoC: SOF: Add PM support ASoC: SOF: Add Nocodec machine driver support Pierre-Louis Bossart (2): ASoC: SOF: Add xtensa support ASoC: SOF: Add utils include/sound/soc.h | 3 + include/sound/sof.h | 91 + include/sound/sof/control.h | 125 ++ include/sound/sof/dai-intel.h | 175 ++ include/sound/sof/dai.h | 75 + include/sound/sof/header.h | 158 ++ include/sound/sof/info.h | 118 ++ include/sound/sof/pm.h | 48 + include/sound/sof/stream.h | 149 ++ include/sound/sof/topology.h | 256 +++ include/sound/sof/trace.h | 66 + include/sound/sof/xtensa.h | 44 + include/uapi/sound/sof/abi.h | 63 + include/uapi/sound/sof/eq.h | 164 ++ include/uapi/sound/sof/fw.h | 67 + include/uapi/sound/sof/header.h | 27 + include/uapi/sound/sof/manifest.h | 188 ++ include/uapi/sound/sof/tokens.h | 95 + include/uapi/sound/sof/tone.h | 21 + include/uapi/sound/sof/trace.h | 93 + sound/soc/sof/control.c | 421 +++++ sound/soc/sof/core.c | 398 +++++ sound/soc/sof/debug.c | 176 ++ sound/soc/sof/ipc.c | 813 +++++++++ sound/soc/sof/loader.c | 322 ++++ sound/soc/sof/nocodec.c | 81 + sound/soc/sof/ops.c | 200 +++ sound/soc/sof/ops.h | 379 ++++ sound/soc/sof/pcm.c | 755 ++++++++ sound/soc/sof/pm.c | 403 +++++ sound/soc/sof/sof-priv.h | 574 ++++++ sound/soc/sof/topology.c | 2775 +++++++++++++++++++++++++++++ sound/soc/sof/trace.c | 295 +++ sound/soc/sof/utils.c | 173 ++ sound/soc/sof/xtensa/Kconfig | 3 + sound/soc/sof/xtensa/Makefile | 5 + sound/soc/sof/xtensa/core.c | 152 ++ 37 files changed, 9951 insertions(+) create mode 100644 include/sound/sof.h create mode 100644 include/sound/sof/control.h create mode 100644 include/sound/sof/dai-intel.h create mode 100644 include/sound/sof/dai.h create mode 100644 include/sound/sof/header.h create mode 100644 include/sound/sof/info.h create mode 100644 include/sound/sof/pm.h create mode 100644 include/sound/sof/stream.h create mode 100644 include/sound/sof/topology.h create mode 100644 include/sound/sof/trace.h create mode 100644 include/sound/sof/xtensa.h create mode 100644 include/uapi/sound/sof/abi.h create mode 100644 include/uapi/sound/sof/eq.h create mode 100644 include/uapi/sound/sof/fw.h create mode 100644 include/uapi/sound/sof/header.h create mode 100644 include/uapi/sound/sof/manifest.h create mode 100644 include/uapi/sound/sof/tokens.h create mode 100644 include/uapi/sound/sof/tone.h create mode 100644 include/uapi/sound/sof/trace.h create mode 100644 sound/soc/sof/control.c create mode 100644 sound/soc/sof/core.c create mode 100644 sound/soc/sof/debug.c create mode 100644 sound/soc/sof/ipc.c create mode 100644 sound/soc/sof/loader.c create mode 100644 sound/soc/sof/nocodec.c create mode 100644 sound/soc/sof/ops.c create mode 100644 sound/soc/sof/ops.h create mode 100644 sound/soc/sof/pcm.c create mode 100644 sound/soc/sof/pm.c create mode 100644 sound/soc/sof/sof-priv.h create mode 100644 sound/soc/sof/topology.c create mode 100644 sound/soc/sof/trace.c create mode 100644 sound/soc/sof/utils.c create mode 100644 sound/soc/sof/xtensa/Kconfig create mode 100644 sound/soc/sof/xtensa/Makefile create mode 100644 sound/soc/sof/xtensa/core.c -- 2.17.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 21:23 [PATCH v3 00/14] Sound Open Firmware (SOF) core Pierre-Louis Bossart @ 2018-12-11 21:23 ` Pierre-Louis Bossart 2018-12-11 22:38 ` Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Pierre-Louis Bossart @ 2018-12-11 21:23 UTC (permalink / raw) To: alsa-devel Cc: Daniel Baluta, andriy.shevchenko, tiwai, Pierre-Louis Bossart, liam.r.girdwood, vkoul, broonie, Alan Cox, sound-open-firmware 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 | 322 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 322 insertions(+) create mode 100644 sound/soc/sof/loader.c 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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 21:23 ` [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Pierre-Louis Bossart @ 2018-12-11 22:38 ` Andy Shevchenko 2018-12-11 23:54 ` Pierre-Louis Bossart 2018-12-12 11:23 ` Takashi Iwai 2019-01-09 21:02 ` Mark Brown 2 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2018-12-11 22:38 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, tiwai, Daniel Baluta, liam.r.girdwood, vkoul, broonie, Alan Cox, sound-open-firmware 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); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 22:38 ` Andy Shevchenko @ 2018-12-11 23:54 ` Pierre-Louis Bossart 2019-01-09 20:55 ` Mark Brown 0 siblings, 1 reply; 13+ messages in thread From: Pierre-Louis Bossart @ 2018-12-11 23:54 UTC (permalink / raw) To: Andy Shevchenko Cc: alsa-devel, tiwai, Daniel Baluta, liam.r.girdwood, vkoul, broonie, sound-open-firmware, Alan Cox 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); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 23:54 ` Pierre-Louis Bossart @ 2019-01-09 20:55 ` Mark Brown 0 siblings, 0 replies; 13+ messages in thread From: Mark Brown @ 2019-01-09 20:55 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Daniel Baluta, Andy Shevchenko, tiwai, alsa-devel, liam.r.girdwood, vkoul, sound-open-firmware, Alan Cox [-- Attachment #1.1: Type: text/plain, Size: 1276 bytes --] 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 21:23 ` [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Pierre-Louis Bossart 2018-12-11 22:38 ` Andy Shevchenko @ 2018-12-12 11:23 ` Takashi Iwai 2019-01-09 21:02 ` Mark Brown 2 siblings, 0 replies; 13+ messages in thread From: Takashi Iwai @ 2018-12-12 11:23 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, andriy.shevchenko, Daniel Baluta, liam.r.girdwood, vkoul, broonie, Alan Cox, sound-open-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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2018-12-11 21:23 ` [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Pierre-Louis Bossart 2018-12-11 22:38 ` Andy Shevchenko 2018-12-12 11:23 ` Takashi Iwai @ 2019-01-09 21:02 ` Mark Brown 2019-01-09 21:24 ` Pierre-Louis Bossart 2 siblings, 1 reply; 13+ messages in thread From: Mark Brown @ 2019-01-09 21:02 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, andriy.shevchenko, tiwai, Daniel Baluta, liam.r.girdwood, vkoul, Alan Cox, sound-open-firmware [-- Attachment #1.1: Type: text/plain, Size: 1043 bytes --] 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support 2019-01-09 21:02 ` Mark Brown @ 2019-01-09 21:24 ` Pierre-Louis Bossart 0 siblings, 0 replies; 13+ messages in thread From: Pierre-Louis Bossart @ 2019-01-09 21:24 UTC (permalink / raw) To: Mark Brown Cc: Daniel Baluta, andriy.shevchenko, tiwai, alsa-devel, liam.r.girdwood, vkoul, sound-open-firmware, Alan Cox 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-09 21:24 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <mailman.3934.1544619824.753.alsa-devel@alsa-project.org> 2018-12-20 2:11 ` [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support Bard liao 2018-12-20 8:16 ` Takashi Iwai 2018-12-20 15:07 ` Pierre-Louis Bossart 2018-12-21 9:05 ` Bard liao 2018-12-21 9:57 ` Takashi Iwai 2018-12-21 15:43 ` Liam Girdwood 2018-12-11 21:23 [PATCH v3 00/14] Sound Open Firmware (SOF) core Pierre-Louis Bossart 2018-12-11 21:23 ` [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Pierre-Louis Bossart 2018-12-11 22:38 ` Andy Shevchenko 2018-12-11 23:54 ` Pierre-Louis Bossart 2019-01-09 20:55 ` Mark Brown 2018-12-12 11:23 ` Takashi Iwai 2019-01-09 21:02 ` Mark Brown 2019-01-09 21:24 ` Pierre-Louis Bossart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.