Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
On 12/11/18 5:21 PM, Andy Shevchenko wrote:
On Tue, Dec 11, 2018 at 03:23:11PM -0600, Pierre-Louis Bossart wrote:
From: Liam Girdwood liam.r.girdwood@linux.intel.com
This patch adds support for real-time DSP logging (timestamped events and bespoke binary data) for firmware debug. The current solution relies on DMA transfers to system memory that is then accessed by userspace tools such as sof-logger. For Intel platforms, two types of DMAs are currently used (GP-DMA for Baytrail/CherryTrail and HDaudio DMA for SKL+)
Due to historical reasons, the driver code follows the DSP firmware conventions and refers to 'traces', but it is currently unrelated to the Linux trace subsystem. Future solutions will include support for more advanced hardware (e.g. MIPI Sys-T), additional formats and the ability to enable/disable specific traces dynamically.
- if (count > buffer_size - lpos)
count = buffer_size - lpos;min() / max() ?
- /* make sure count is <= avail */
- count = avail > count ? count : avail;
ditto. Check entire series for such.
yep.
- dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root,
dfse, &sof_dfs_trace_fops);- if (!dfse->dfsentry) {
dev_err(sdev->dev,"error: cannot create debugfs entry for trace\n");kfree(dfse);return -ENODEV;Should not return an error. We must be fine w/o debugfs files.
Not sure I fully agree with the 'must'.
yes execution should proceed, but without debugfs we cannot, well, debug, so this is a real show-stopper
- }
- return 0;
+}
On Wed, 12 Dec 2018 00:43:26 +0100, Pierre-Louis Bossart wrote:
On 12/11/18 5:21 PM, Andy Shevchenko wrote:
On Tue, Dec 11, 2018 at 03:23:11PM -0600, Pierre-Louis Bossart wrote:
From: Liam Girdwood liam.r.girdwood@linux.intel.com
This patch adds support for real-time DSP logging (timestamped events and bespoke binary data) for firmware debug. The current solution relies on DMA transfers to system memory that is then accessed by userspace tools such as sof-logger. For Intel platforms, two types of DMAs are currently used (GP-DMA for Baytrail/CherryTrail and HDaudio DMA for SKL+)
Due to historical reasons, the driver code follows the DSP firmware conventions and refers to 'traces', but it is currently unrelated to the Linux trace subsystem. Future solutions will include support for more advanced hardware (e.g. MIPI Sys-T), additional formats and the ability to enable/disable specific traces dynamically.
- if (count > buffer_size - lpos)
count = buffer_size - lpos;min() / max() ?
- /* make sure count is <= avail */
- count = avail > count ? count : avail;
ditto. Check entire series for such.
yep.
- dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root,
dfse, &sof_dfs_trace_fops);- if (!dfse->dfsentry) {
dev_err(sdev->dev,"error: cannot create debugfs entry for trace\n");kfree(dfse);return -ENODEV;Should not return an error. We must be fine w/o debugfs files.
Not sure I fully agree with the 'must'.
yes execution should proceed, but without debugfs we cannot, well, debug, so this is a real show-stopper
So the caller would ignore the error from this function?
Note that debugfs is a pure optional matter, and you can build a kernel without CONFIG_DEBUGFS, too. Then you'll always get this error...
thanks,
Takashi
participants (2)
-
Pierre-Louis Bossart -
Takashi Iwai