Re: [PATCH v3 03/14] ASoC: SOF: Add driver debug support.

Pierre-Louis Bossart
2019-01-10T20:47:07Z

[combined feedback from Ranjani, Liam and me]

>> + /* copy from DSP MMIO */
>> + pm_runtime_get_noresume(sdev->dev);
> Why are we doing a _get_noresume() here? It won't actually do anything
> to the device...
the reason we currently do a get_noresume() is so that we can still read
the debugfs entries in case of DSP panic, where a regular resume would
not succeed.

That said this part will be reworked based on feedback from Rafael whose
suggestion was to get rid of the pm calls while reading debugfs. We can
make sure that all the values are flushed out of the dsp before
suspending which will make the need to wake it up redundant.

>
>> +
>> + memcpy_fromio(buf, dfse->buf + pos, size);
> Extra space?
yep. we run checkpatch.pl --strict all the time and missed this somehow.
>
>> + /*
>> + * TODO: revisit to check if we need mark_last_busy, or if we
>> + * should change to use xxx_put_sync[_suspend]().
>> + */
>> + ret = pm_runtime_put_sync_autosuspend(sdev->dev);
>> + if (ret < 0)
>> + dev_warn(sdev->dev, "warn: debugFS failed to autosuspend %zd\n",
>> + ret);
> It rather depends what you're doing... I'm definitely confused as to
> why you need a _sync operation - if you're doing autosuspend stuff
> presumably you don't care so much if the device gets powered down
> immediately, and I can't in general see why that'd be important.
yes, as stated above we'll rework all this
>
>> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
>> + dfse, &sof_dfs_fops);
>> + if (!dfse->dfsentry) {
>> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n");
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(snd_sof_debugfs_io_create_item);
> debugfs uses EXPORT_SYMBOL_GPL().

We don't have a burning desire to keep this as EXPORT_SYMBOL since
debugfs is really Linux specific, so will move all these statements as
EXPORT_SYMBOL_GPL.

Thanks Mark for your time, much appreciated. We've already dealt with
most of the feedback from Takashi/Andy/Daniel so should have a new
version of these patches in 1-2 weeks.