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