[alsa-devel] [Sound-open-firmware] [PATCH v5 09/14] ASoC: SOF: Add firmware loader support

[Pierre-Louis Bossart]

>> +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.