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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
