Re: [PATCH v3 09/14] ASoC: SOF: Add firmware loader support

Pierre-Louis Bossart
2018-12-11T23:54:02Z

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);