* [PATCH 0/3] accel/qaic: Add debugfs entries
@ 2024-03-11 16:58 Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 1/3] accel/qaic: Add bootlog debugfs Jeffrey Hugo
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2024-03-11 16:58 UTC (permalink / raw
To: quic_carlv, quic_pkanojiy, stanislaw.gruszka, jacek.lawrynowicz
Cc: linux-arm-msm, dri-devel, ogabbay, Jeffrey Hugo
Add 3 debugfs entries that can be useful in debugging a variety of
issues.
bootlog - output the device bootloader log
fifo_size - output the configured dbc fifo size
queued - output how many requests are queued in the dbc fifo
Bootlog is unique to the device, where as fifo_size/queued is per-dbc.
Jeffrey Hugo (3):
accel/qaic: Add bootlog debugfs
accel/qaic: Add fifo size debugfs
accel/qaic: Add fifo queued debugfs
drivers/accel/qaic/Makefile | 2 +
drivers/accel/qaic/qaic.h | 9 +
drivers/accel/qaic/qaic_data.c | 9 +
drivers/accel/qaic/qaic_debugfs.c | 333 ++++++++++++++++++++++++++++++
drivers/accel/qaic/qaic_debugfs.h | 20 ++
drivers/accel/qaic/qaic_drv.c | 16 +-
6 files changed, 388 insertions(+), 1 deletion(-)
create mode 100644 drivers/accel/qaic/qaic_debugfs.c
create mode 100644 drivers/accel/qaic/qaic_debugfs.h
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] accel/qaic: Add bootlog debugfs
2024-03-11 16:58 [PATCH 0/3] accel/qaic: Add debugfs entries Jeffrey Hugo
@ 2024-03-11 16:58 ` Jeffrey Hugo
2024-03-14 11:41 ` Jacek Lawrynowicz
2024-03-11 16:58 ` [PATCH 2/3] accel/qaic: Add fifo size debugfs Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 3/3] accel/qaic: Add fifo queued debugfs Jeffrey Hugo
2 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2024-03-11 16:58 UTC (permalink / raw
To: quic_carlv, quic_pkanojiy, stanislaw.gruszka, jacek.lawrynowicz
Cc: linux-arm-msm, dri-devel, ogabbay, Jeffrey Hugo
During the boot process of AIC100, the bootloaders (PBL and SBL) log
messages to device RAM. During SBL, if the host opens the QAIC_LOGGING
channel, SBL will offload the contents of the log buffer to the host,
and stream any new messages that SBL logs.
This log of the boot process can be very useful for an initial triage of
any boot related issues. For example, if SBL rejects one of the runtime
firmware images for a validation failure, SBL will log a reason why.
Add the ability of the driver to open the logging channel, receive the
messages, and store them. Also define a debugfs entry called "bootlog"
by hooking into the DRM debugfs framework. When the bootlog debugfs
entry is read, the current contents of the log that the host is caching
is displayed to the user. The driver will retain the cache until it
detects that the device has rebooted. At that point, the cache will be
freed, and the driver will wait for a new log. With this scheme, the
driver will only have a cache of the log from the current device boot.
Note that if the driver initializes a device and it is already in the
runtime state (QSM), no bootlog will be available through this mechanism
because the driver and SBL have not communicated.
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
---
drivers/accel/qaic/Makefile | 2 +
drivers/accel/qaic/qaic.h | 8 +
drivers/accel/qaic/qaic_debugfs.c | 271 ++++++++++++++++++++++++++++++
drivers/accel/qaic/qaic_debugfs.h | 20 +++
drivers/accel/qaic/qaic_drv.c | 16 +-
5 files changed, 316 insertions(+), 1 deletion(-)
create mode 100644 drivers/accel/qaic/qaic_debugfs.c
create mode 100644 drivers/accel/qaic/qaic_debugfs.h
diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
index 3f7f6dfde7f2..2cadcc1baa0e 100644
--- a/drivers/accel/qaic/Makefile
+++ b/drivers/accel/qaic/Makefile
@@ -11,3 +11,5 @@ qaic-y := \
qaic_data.o \
qaic_drv.o \
qaic_timesync.o
+
+qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 9256653b3036..03d9c9fbffb3 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -153,6 +153,14 @@ struct qaic_device {
struct mhi_device *qts_ch;
/* Work queue for tasks related to MHI "QAIC_TIMESYNC" channel */
struct workqueue_struct *qts_wq;
+ /* Head of list of page allocated by MHI bootlog device */
+ struct list_head bootlog;
+ /* MHI bootlog channel device */
+ struct mhi_device *bootlog_ch;
+ /* Work queue for tasks related to MHI bootlog device */
+ struct workqueue_struct *bootlog_wq;
+ /* Synchronizes access of pages in MHI bootlog device */
+ struct mutex bootlog_mutex;
};
struct qaic_drm_device {
diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
new file mode 100644
index 000000000000..4f87fe29be1a
--- /dev/null
+++ b/drivers/accel/qaic/qaic_debugfs.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
+/* Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved. */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/list.h>
+#include <linux/mhi.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/seq_file.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "qaic.h"
+#include "qaic_debugfs.h"
+
+#define BOOTLOG_POOL_SIZE 16
+#define BOOTLOG_MSG_SIZE 512
+
+struct bootlog_msg {
+ /* Buffer for bootlog messages */
+ char str[BOOTLOG_MSG_SIZE];
+ /* Root struct of device, used to access device resources */
+ struct qaic_device *qdev;
+ /* Work struct to schedule work coming on QAIC_LOGGING channel */
+ struct work_struct work;
+};
+
+struct bootlog_page {
+ /* Node in list of bootlog pages maintained by root device struct */
+ struct list_head node;
+ /* Total size of the buffer that holds the bootlogs. It is PAGE_SIZE */
+ unsigned int size;
+ /* Offset for the next bootlog */
+ unsigned int offset;
+};
+
+static int bootlog_show(struct seq_file *s, void *unused)
+{
+ struct bootlog_page *page;
+ struct qaic_device *qdev;
+ void *page_end;
+ void *log;
+
+ qdev = s->private;
+ mutex_lock(&qdev->bootlog_mutex);
+ list_for_each_entry(page, &qdev->bootlog, node) {
+ log = page + 1;
+ page_end = (void *)page + page->offset;
+ while (log < page_end) {
+ seq_printf(s, "%s", (char *)log);
+ log += strlen(log) + 1;
+ }
+ }
+ mutex_unlock(&qdev->bootlog_mutex);
+
+ return 0;
+}
+
+static int bootlog_fops_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, bootlog_show, inode->i_private);
+}
+
+static const struct file_operations bootlog_fops = {
+ .owner = THIS_MODULE,
+ .open = bootlog_fops_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+void qaic_debugfs_init(struct qaic_drm_device *qddev)
+{
+ struct qaic_device *qdev = qddev->qdev;
+ struct dentry *debugfs_root;
+
+ debugfs_root = to_drm(qddev)->debugfs_root;
+
+ debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
+}
+
+static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
+{
+ struct bootlog_page *page;
+
+ page = (struct bootlog_page *)devm_get_free_pages(&qdev->pdev->dev, GFP_KERNEL, 0);
+ if (!page)
+ return page;
+
+ page->size = PAGE_SIZE;
+ page->offset = sizeof(*page);
+ list_add_tail(&page->node, &qdev->bootlog);
+
+ return page;
+}
+
+static int reset_bootlog(struct qaic_device *qdev)
+{
+ struct bootlog_page *page;
+ struct bootlog_page *i;
+
+ list_for_each_entry_safe(page, i, &qdev->bootlog, node) {
+ list_del(&page->node);
+ devm_free_pages(&qdev->pdev->dev, (unsigned long)page);
+ }
+
+ page = alloc_bootlog_page(qdev);
+ if (!page)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void *bootlog_get_space(struct qaic_device *qdev, unsigned int size)
+{
+ struct bootlog_page *page;
+
+ page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
+
+ if (size > page->size - sizeof(*page))
+ return NULL;
+
+ if (page->offset + size > page->size) {
+ page = alloc_bootlog_page(qdev);
+ if (!page)
+ return NULL;
+ }
+
+ return (void *)page + page->offset;
+}
+
+static void bootlog_commit(struct qaic_device *qdev, unsigned int size)
+{
+ struct bootlog_page *page;
+
+ page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
+
+ page->offset += size;
+}
+
+static void bootlog_log(struct work_struct *work)
+{
+ struct bootlog_msg *msg = container_of(work, struct bootlog_msg, work);
+ unsigned int len = strlen(msg->str) + 1;
+ struct qaic_device *qdev = msg->qdev;
+ void *log;
+
+ mutex_lock(&qdev->bootlog_mutex);
+ log = bootlog_get_space(qdev, len);
+ if (log) {
+ memcpy(log, msg, len);
+ bootlog_commit(qdev, len);
+ }
+ mutex_unlock(&qdev->bootlog_mutex);
+
+ if (mhi_queue_buf(qdev->bootlog_ch, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT))
+ devm_kfree(&qdev->pdev->dev, msg);
+}
+
+static int qaic_bootlog_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
+{
+ struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
+ struct bootlog_msg *msg;
+ int i, ret;
+
+ qdev->bootlog_wq = alloc_ordered_workqueue("qaic_bootlog", 0);
+ if (!qdev->bootlog_wq) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ mutex_lock(&qdev->bootlog_mutex);
+ ret = reset_bootlog(qdev);
+ mutex_unlock(&qdev->bootlog_mutex);
+ if (ret)
+ goto destroy_workqueue;
+
+ ret = mhi_prepare_for_transfer(mhi_dev);
+ if (ret)
+ goto destroy_workqueue;
+
+ for (i = 0; i < BOOTLOG_POOL_SIZE; i++) {
+ msg = devm_kzalloc(&qdev->pdev->dev, sizeof(*msg), GFP_KERNEL);
+ if (!msg) {
+ ret = -ENOMEM;
+ goto mhi_unprepare;
+ }
+
+ msg->qdev = qdev;
+ INIT_WORK(&msg->work, bootlog_log);
+
+ ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT);
+ if (ret)
+ goto mhi_unprepare;
+ }
+
+ dev_set_drvdata(&mhi_dev->dev, qdev);
+ qdev->bootlog_ch = mhi_dev;
+ return 0;
+
+mhi_unprepare:
+ mhi_unprepare_from_transfer(mhi_dev);
+destroy_workqueue:
+ flush_workqueue(qdev->bootlog_wq);
+ destroy_workqueue(qdev->bootlog_wq);
+out:
+ return ret;
+}
+
+static void qaic_bootlog_mhi_remove(struct mhi_device *mhi_dev)
+{
+ struct qaic_device *qdev;
+
+ qdev = dev_get_drvdata(&mhi_dev->dev);
+
+ mhi_unprepare_from_transfer(qdev->bootlog_ch);
+ flush_workqueue(qdev->bootlog_wq);
+ destroy_workqueue(qdev->bootlog_wq);
+ qdev->bootlog_ch = NULL;
+}
+
+static void qaic_bootlog_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
+{
+}
+
+static void qaic_bootlog_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
+{
+ struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
+ struct bootlog_msg *msg = mhi_result->buf_addr;
+
+ if (mhi_result->transaction_status) {
+ devm_kfree(&qdev->pdev->dev, msg);
+ return;
+ }
+
+ /* Force a null at the end of the transferred string */
+ msg->str[mhi_result->bytes_xferd - 1] = 0;
+
+ queue_work(qdev->bootlog_wq, &msg->work);
+}
+
+static const struct mhi_device_id qaic_bootlog_mhi_match_table[] = {
+ { .chan = "QAIC_LOGGING", },
+ {},
+};
+
+static struct mhi_driver qaic_bootlog_mhi_driver = {
+ .id_table = qaic_bootlog_mhi_match_table,
+ .remove = qaic_bootlog_mhi_remove,
+ .probe = qaic_bootlog_mhi_probe,
+ .ul_xfer_cb = qaic_bootlog_mhi_ul_xfer_cb,
+ .dl_xfer_cb = qaic_bootlog_mhi_dl_xfer_cb,
+ .driver = {
+ .name = "qaic_bootlog",
+ },
+};
+
+int qaic_bootlog_register(void)
+{
+ return mhi_driver_register(&qaic_bootlog_mhi_driver);
+}
+
+void qaic_bootlog_unregister(void)
+{
+ mhi_driver_unregister(&qaic_bootlog_mhi_driver);
+}
diff --git a/drivers/accel/qaic/qaic_debugfs.h b/drivers/accel/qaic/qaic_debugfs.h
new file mode 100644
index 000000000000..ea3fd1a88405
--- /dev/null
+++ b/drivers/accel/qaic/qaic_debugfs.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
+/* Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */
+
+#ifndef __QAIC_DEBUGFS_H__
+#define __QAIC_DEBUGFS_H__
+
+#include <drm/drm_file.h>
+
+#ifdef CONFIG_DEBUG_FS
+int qaic_bootlog_register(void);
+void qaic_bootlog_unregister(void);
+void qaic_debugfs_init(struct qaic_drm_device *qddev);
+#else
+int qaic_bootlog_register(void) { return 0; }
+void qaic_bootlog_unregister(void) {}
+void qaic_debugfs_init(struct qaic_drm_device *qddev) {}
+#endif /* CONFIG_DEBUG_FS */
+#endif /* __QAIC_DEBUGFS_H__ */
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index d1a632dbaec6..f072edb74f22 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -28,6 +28,7 @@
#include "mhi_controller.h"
#include "qaic.h"
+#include "qaic_debugfs.h"
#include "qaic_timesync.h"
MODULE_IMPORT_NS(DMA_BUF);
@@ -229,8 +230,12 @@ static int qaic_create_drm_device(struct qaic_device *qdev, s32 partition_id)
qddev->partition_id = partition_id;
ret = drm_dev_register(drm, 0);
- if (ret)
+ if (ret) {
pci_dbg(qdev->pdev, "drm_dev_register failed %d\n", ret);
+ return ret;
+ }
+
+ qaic_debugfs_init(qddev);
return ret;
}
@@ -380,6 +385,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
if (ret)
return NULL;
ret = drmm_mutex_init(drm, &qdev->cntl_mutex);
+ if (ret)
+ return NULL;
+ ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
if (ret)
return NULL;
@@ -399,6 +407,7 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
qddev->qdev = qdev;
INIT_LIST_HEAD(&qdev->cntl_xfer_list);
+ INIT_LIST_HEAD(&qdev->bootlog);
INIT_LIST_HEAD(&qddev->users);
for (i = 0; i < qdev->num_dbc; ++i) {
@@ -639,6 +648,10 @@ static int __init qaic_init(void)
if (ret)
pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
+ ret = qaic_bootlog_register();
+ if (ret)
+ pr_debug("qaic: qaic_bootlog_register failed %d\n", ret);
+
return 0;
free_pci:
@@ -664,6 +677,7 @@ static void __exit qaic_exit(void)
* reinitializing the link_up state after the cleanup is done.
*/
link_up = true;
+ qaic_bootlog_unregister();
qaic_timesync_deinit();
mhi_driver_unregister(&qaic_mhi_driver);
pci_unregister_driver(&qaic_pci_driver);
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] accel/qaic: Add fifo size debugfs
2024-03-11 16:58 [PATCH 0/3] accel/qaic: Add debugfs entries Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 1/3] accel/qaic: Add bootlog debugfs Jeffrey Hugo
@ 2024-03-11 16:58 ` Jeffrey Hugo
2024-03-12 8:45 ` kernel test robot
2024-03-14 11:44 ` Jacek Lawrynowicz
2024-03-11 16:58 ` [PATCH 3/3] accel/qaic: Add fifo queued debugfs Jeffrey Hugo
2 siblings, 2 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2024-03-11 16:58 UTC (permalink / raw
To: quic_carlv, quic_pkanojiy, stanislaw.gruszka, jacek.lawrynowicz
Cc: linux-arm-msm, dri-devel, ogabbay, Jeffrey Hugo
Each DMA Bridge Channel (dbc) has a unique configured fifo size which is
specified by the userspace client of that dbc. Since the fifo is
circular, it is useful to know the configured size when debugging
issues.
Add a per-dbc subdirectory in debugfs and in each subdirectory add a
fifo_size entry that will display the size of that dbc's fifo when read.
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
---
drivers/accel/qaic/qaic_debugfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
index 4f87fe29be1a..9d56cd451b64 100644
--- a/drivers/accel/qaic/qaic_debugfs.c
+++ b/drivers/accel/qaic/qaic_debugfs.c
@@ -11,6 +11,7 @@
#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/seq_file.h>
+#include <linux/sprintf.h>
#include <linux/string.h>
#include <linux/types.h>
#include <linux/workqueue.h>
@@ -20,6 +21,7 @@
#define BOOTLOG_POOL_SIZE 16
#define BOOTLOG_MSG_SIZE 512
+#define QAIC_DBC_DIR_NAME 9
struct bootlog_msg {
/* Buffer for bootlog messages */
@@ -74,14 +76,43 @@ static const struct file_operations bootlog_fops = {
.release = single_release,
};
+static int read_dbc_fifo_size(struct seq_file *s, void *unused)
+{
+ struct dma_bridge_chan *dbc = s->private;
+
+ seq_printf(s, "%u\n", dbc->nelem);
+ return 0;
+}
+
+static int fifo_size_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, read_dbc_fifo_size, inode->i_private);
+}
+
+static const struct file_operations fifo_size_fops = {
+ .owner = THIS_MODULE,
+ .open = fifo_size_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
void qaic_debugfs_init(struct qaic_drm_device *qddev)
{
struct qaic_device *qdev = qddev->qdev;
struct dentry *debugfs_root;
+ struct dentry *debugfs_dir;
+ char name[QAIC_DBC_DIR_NAME];
+ u32 i;
debugfs_root = to_drm(qddev)->debugfs_root;
debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
+ for (i = 0; i < qdev->num_dbc; ++i) {
+ snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
+ debugfs_dir = debugfs_create_dir(name, debugfs_root);
+ debugfs_create_file("fifo_size", 0400, debugfs_dir, &qdev->dbc[i], &fifo_size_fops);
+ }
}
static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] accel/qaic: Add fifo queued debugfs
2024-03-11 16:58 [PATCH 0/3] accel/qaic: Add debugfs entries Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 1/3] accel/qaic: Add bootlog debugfs Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 2/3] accel/qaic: Add fifo size debugfs Jeffrey Hugo
@ 2024-03-11 16:58 ` Jeffrey Hugo
2024-03-14 11:44 ` Jacek Lawrynowicz
2 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2024-03-11 16:58 UTC (permalink / raw
To: quic_carlv, quic_pkanojiy, stanislaw.gruszka, jacek.lawrynowicz
Cc: linux-arm-msm, dri-devel, ogabbay, Jeffrey Hugo
When debugging functional issues with workload input processing, it is
useful to know if requests are backing up in the fifo, or perhaps
getting stuck elsewhere. To answer the question of how many requests are
in the fifo, implement a "queued" debugfs entry per-dbc that returns the
number of pending requests when read.
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
---
drivers/accel/qaic/qaic.h | 1 +
drivers/accel/qaic/qaic_data.c | 9 +++++++++
drivers/accel/qaic/qaic_debugfs.c | 31 +++++++++++++++++++++++++++++++
3 files changed, 41 insertions(+)
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 03d9c9fbffb3..02561b6cecc6 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -288,6 +288,7 @@ int disable_dbc(struct qaic_device *qdev, u32 dbc_id, struct qaic_user *usr);
void enable_dbc(struct qaic_device *qdev, u32 dbc_id, struct qaic_user *usr);
void wakeup_dbc(struct qaic_device *qdev, u32 dbc_id);
void release_dbc(struct qaic_device *qdev, u32 dbc_id);
+void qaic_data_get_fifo_info(struct dma_bridge_chan *dbc, u32 *head, u32 *tail);
void wake_all_cntl(struct qaic_device *qdev);
void qaic_dev_reset_clean_local_state(struct qaic_device *qdev);
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index 2459fe4a3f95..e86e71c1cdd8 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -1981,3 +1981,12 @@ void release_dbc(struct qaic_device *qdev, u32 dbc_id)
dbc->in_use = false;
wake_up(&dbc->dbc_release);
}
+
+void qaic_data_get_fifo_info(struct dma_bridge_chan *dbc, u32 *head, u32 *tail)
+{
+ if (!dbc || !head || !tail)
+ return;
+
+ *head = readl(dbc->dbc_base + REQHP_OFF);
+ *tail = readl(dbc->dbc_base + REQTP_OFF);
+}
diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
index 9d56cd451b64..12a65b98701d 100644
--- a/drivers/accel/qaic/qaic_debugfs.c
+++ b/drivers/accel/qaic/qaic_debugfs.c
@@ -97,6 +97,36 @@ static const struct file_operations fifo_size_fops = {
.release = single_release,
};
+static int read_dbc_queued(struct seq_file *s, void *unused)
+{
+ struct dma_bridge_chan *dbc = s->private;
+ u32 tail = 0, head = 0;
+
+ qaic_data_get_fifo_info(dbc, &head, &tail);
+
+ if (head == U32_MAX || tail == U32_MAX)
+ seq_printf(s, "%u\n", 0);
+ else if (head > tail)
+ seq_printf(s, "%u\n", dbc->nelem - head + tail);
+ else
+ seq_printf(s, "%u\n", tail - head);
+
+ return 0;
+}
+
+static int queued_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, read_dbc_queued, inode->i_private);
+}
+
+static const struct file_operations queued_fops = {
+ .owner = THIS_MODULE,
+ .open = queued_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
void qaic_debugfs_init(struct qaic_drm_device *qddev)
{
struct qaic_device *qdev = qddev->qdev;
@@ -112,6 +142,7 @@ void qaic_debugfs_init(struct qaic_drm_device *qddev)
snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
debugfs_dir = debugfs_create_dir(name, debugfs_root);
debugfs_create_file("fifo_size", 0400, debugfs_dir, &qdev->dbc[i], &fifo_size_fops);
+ debugfs_create_file("queued", 0400, debugfs_dir, &qdev->dbc[i], &queued_fops);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] accel/qaic: Add fifo size debugfs
2024-03-11 16:58 ` [PATCH 2/3] accel/qaic: Add fifo size debugfs Jeffrey Hugo
@ 2024-03-12 8:45 ` kernel test robot
2024-03-14 11:44 ` Jacek Lawrynowicz
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2024-03-12 8:45 UTC (permalink / raw
To: Jeffrey Hugo, quic_carlv, quic_pkanojiy, stanislaw.gruszka,
jacek.lawrynowicz
Cc: oe-kbuild-all, linux-arm-msm, dri-devel, ogabbay, Jeffrey Hugo
Hi Jeffrey,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on next-20240312]
[cannot apply to linus/master v6.8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jeffrey-Hugo/accel-qaic-Add-bootlog-debugfs/20240312-010045
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240311165826.1728693-3-quic_jhugo%40quicinc.com
patch subject: [PATCH 2/3] accel/qaic: Add fifo size debugfs
config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240312/202403121628.x2PPKBRx-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240312/202403121628.x2PPKBRx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403121628.x2PPKBRx-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/accel/qaic/qaic_debugfs.c: In function 'qaic_debugfs_init':
>> drivers/accel/qaic/qaic_debugfs.c:112:55: warning: '%03u' directive output may be truncated writing between 3 and 10 bytes into a region of size 6 [-Wformat-truncation=]
112 | snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
| ^~~~
drivers/accel/qaic/qaic_debugfs.c:112:51: note: directive argument in the range [0, 4294967294]
112 | snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
| ^~~~~~~~~
drivers/accel/qaic/qaic_debugfs.c:112:17: note: 'snprintf' output between 7 and 14 bytes into a destination of size 9
112 | snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +112 drivers/accel/qaic/qaic_debugfs.c
99
100 void qaic_debugfs_init(struct qaic_drm_device *qddev)
101 {
102 struct qaic_device *qdev = qddev->qdev;
103 struct dentry *debugfs_root;
104 struct dentry *debugfs_dir;
105 char name[QAIC_DBC_DIR_NAME];
106 u32 i;
107
108 debugfs_root = to_drm(qddev)->debugfs_root;
109
110 debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
111 for (i = 0; i < qdev->num_dbc; ++i) {
> 112 snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
113 debugfs_dir = debugfs_create_dir(name, debugfs_root);
114 debugfs_create_file("fifo_size", 0400, debugfs_dir, &qdev->dbc[i], &fifo_size_fops);
115 }
116 }
117
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] accel/qaic: Add bootlog debugfs
2024-03-11 16:58 ` [PATCH 1/3] accel/qaic: Add bootlog debugfs Jeffrey Hugo
@ 2024-03-14 11:41 ` Jacek Lawrynowicz
2024-03-15 15:39 ` Jeffrey Hugo
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Lawrynowicz @ 2024-03-14 11:41 UTC (permalink / raw
To: Jeffrey Hugo, quic_carlv, quic_pkanojiy, stanislaw.gruszka
Cc: linux-arm-msm, dri-devel, ogabbay
Hi,
On 11.03.2024 17:58, Jeffrey Hugo wrote:
> During the boot process of AIC100, the bootloaders (PBL and SBL) log
> messages to device RAM. During SBL, if the host opens the QAIC_LOGGING
> channel, SBL will offload the contents of the log buffer to the host,
> and stream any new messages that SBL logs.
>
> This log of the boot process can be very useful for an initial triage of
> any boot related issues. For example, if SBL rejects one of the runtime
> firmware images for a validation failure, SBL will log a reason why.
>
> Add the ability of the driver to open the logging channel, receive the
> messages, and store them. Also define a debugfs entry called "bootlog"
> by hooking into the DRM debugfs framework. When the bootlog debugfs
> entry is read, the current contents of the log that the host is caching
> is displayed to the user. The driver will retain the cache until it
> detects that the device has rebooted. At that point, the cache will be
> freed, and the driver will wait for a new log. With this scheme, the
> driver will only have a cache of the log from the current device boot.
> Note that if the driver initializes a device and it is already in the
> runtime state (QSM), no bootlog will be available through this mechanism
> because the driver and SBL have not communicated.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> ---
> drivers/accel/qaic/Makefile | 2 +
> drivers/accel/qaic/qaic.h | 8 +
> drivers/accel/qaic/qaic_debugfs.c | 271 ++++++++++++++++++++++++++++++
> drivers/accel/qaic/qaic_debugfs.h | 20 +++
> drivers/accel/qaic/qaic_drv.c | 16 +-
> 5 files changed, 316 insertions(+), 1 deletion(-)
> create mode 100644 drivers/accel/qaic/qaic_debugfs.c
> create mode 100644 drivers/accel/qaic/qaic_debugfs.h
>
> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
> index 3f7f6dfde7f2..2cadcc1baa0e 100644
> --- a/drivers/accel/qaic/Makefile
> +++ b/drivers/accel/qaic/Makefile
> @@ -11,3 +11,5 @@ qaic-y := \
> qaic_data.o \
> qaic_drv.o \
> qaic_timesync.o
> +
> +qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
> index 9256653b3036..03d9c9fbffb3 100644
> --- a/drivers/accel/qaic/qaic.h
> +++ b/drivers/accel/qaic/qaic.h
> @@ -153,6 +153,14 @@ struct qaic_device {
> struct mhi_device *qts_ch;
> /* Work queue for tasks related to MHI "QAIC_TIMESYNC" channel */
> struct workqueue_struct *qts_wq;
> + /* Head of list of page allocated by MHI bootlog device */
> + struct list_head bootlog;
> + /* MHI bootlog channel device */
> + struct mhi_device *bootlog_ch;
> + /* Work queue for tasks related to MHI bootlog device */
> + struct workqueue_struct *bootlog_wq;
> + /* Synchronizes access of pages in MHI bootlog device */
> + struct mutex bootlog_mutex;
> };
>
> struct qaic_drm_device {
> diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
> new file mode 100644
> index 000000000000..4f87fe29be1a
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_debugfs.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
> +/* Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved. */
> +
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/list.h>
> +#include <linux/mhi.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/seq_file.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +#include "qaic.h"
> +#include "qaic_debugfs.h"
> +
> +#define BOOTLOG_POOL_SIZE 16
> +#define BOOTLOG_MSG_SIZE 512
> +
> +struct bootlog_msg {
> + /* Buffer for bootlog messages */
> + char str[BOOTLOG_MSG_SIZE];
> + /* Root struct of device, used to access device resources */
> + struct qaic_device *qdev;
> + /* Work struct to schedule work coming on QAIC_LOGGING channel */
> + struct work_struct work;
> +};
> +
> +struct bootlog_page {
> + /* Node in list of bootlog pages maintained by root device struct */
> + struct list_head node;
> + /* Total size of the buffer that holds the bootlogs. It is PAGE_SIZE */
> + unsigned int size;
> + /* Offset for the next bootlog */
> + unsigned int offset;
> +};
> +
> +static int bootlog_show(struct seq_file *s, void *unused)
> +{
> + struct bootlog_page *page;
> + struct qaic_device *qdev;
> + void *page_end;
> + void *log;
> +
> + qdev = s->private;
> + mutex_lock(&qdev->bootlog_mutex);
> + list_for_each_entry(page, &qdev->bootlog, node) {
> + log = page + 1;
> + page_end = (void *)page + page->offset;
> + while (log < page_end) {
> + seq_printf(s, "%s", (char *)log);
> + log += strlen(log) + 1;
> + }
> + }
> + mutex_unlock(&qdev->bootlog_mutex);
> +
> + return 0;
> +}
> +
> +static int bootlog_fops_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, bootlog_show, inode->i_private);
> +}
> +
> +static const struct file_operations bootlog_fops = {
> + .owner = THIS_MODULE,
> + .open = bootlog_fops_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +void qaic_debugfs_init(struct qaic_drm_device *qddev)
> +{
> + struct qaic_device *qdev = qddev->qdev;
> + struct dentry *debugfs_root;
> +
> + debugfs_root = to_drm(qddev)->debugfs_root;
> +
> + debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
> +}
> +
> +static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
> +{
> + struct bootlog_page *page;
> +
> + page = (struct bootlog_page *)devm_get_free_pages(&qdev->pdev->dev, GFP_KERNEL, 0);
> + if (!page)
> + return page;
> +
> + page->size = PAGE_SIZE;
> + page->offset = sizeof(*page);
> + list_add_tail(&page->node, &qdev->bootlog);
> +
> + return page;
> +}
> +
> +static int reset_bootlog(struct qaic_device *qdev)
> +{
> + struct bootlog_page *page;
> + struct bootlog_page *i;
> +
> + list_for_each_entry_safe(page, i, &qdev->bootlog, node) {
> + list_del(&page->node);
> + devm_free_pages(&qdev->pdev->dev, (unsigned long)page);
> + }
This is currently dead code. reset is only used to init the bootlog. You may consider making this init_bootlog() if you are not planning to actually reset the bootlog.
> +
> + page = alloc_bootlog_page(qdev);
> + if (!page)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void *bootlog_get_space(struct qaic_device *qdev, unsigned int size)
> +{
> + struct bootlog_page *page;
> +
> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
> +
> + if (size > page->size - sizeof(*page))
Not critical but would be safer to use this condition: "sizeof(*page) + size > page->size"
> + return NULL;
> +
> + if (page->offset + size > page->size) {
> + page = alloc_bootlog_page(qdev);
> + if (!page)
> + return NULL;
> + }
> +
> + return (void *)page + page->offset;
> +}
> +
> +static void bootlog_commit(struct qaic_device *qdev, unsigned int size)
> +{
> + struct bootlog_page *page;
> +
> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
> +
> + page->offset += size;
> +}
> +
> +static void bootlog_log(struct work_struct *work)
> +{
> + struct bootlog_msg *msg = container_of(work, struct bootlog_msg, work);
> + unsigned int len = strlen(msg->str) + 1;
> + struct qaic_device *qdev = msg->qdev;
> + void *log;
> +
> + mutex_lock(&qdev->bootlog_mutex);
> + log = bootlog_get_space(qdev, len);
> + if (log) {
> + memcpy(log, msg, len);
> + bootlog_commit(qdev, len);
> + }
> + mutex_unlock(&qdev->bootlog_mutex);
> +
> + if (mhi_queue_buf(qdev->bootlog_ch, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT))
> + devm_kfree(&qdev->pdev->dev, msg);
You are freeing `struct work` while still in work callback. This is unsafe.
See https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2564.
Work ptr is kept in busy_hash after the callback has finished and may be still be accessed.
> +}
> +
> +static int qaic_bootlog_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
> +{
> + struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
> + struct bootlog_msg *msg;
> + int i, ret;
> +
> + qdev->bootlog_wq = alloc_ordered_workqueue("qaic_bootlog", 0);
> + if (!qdev->bootlog_wq) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + mutex_lock(&qdev->bootlog_mutex);
Looks like locking should be inside reset_bootlog(), like in other places.
> + ret = reset_bootlog(qdev);
> + mutex_unlock(&qdev->bootlog_mutex);
> + if (ret)
> + goto destroy_workqueue;
> +
> + ret = mhi_prepare_for_transfer(mhi_dev);
> + if (ret)
> + goto destroy_workqueue;
> +
> + for (i = 0; i < BOOTLOG_POOL_SIZE; i++) {
> + msg = devm_kzalloc(&qdev->pdev->dev, sizeof(*msg), GFP_KERNEL);
> + if (!msg) {
> + ret = -ENOMEM;
> + goto mhi_unprepare;
> + }
> +
> + msg->qdev = qdev;
> + INIT_WORK(&msg->work, bootlog_log);
> +
> + ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT);
> + if (ret)
> + goto mhi_unprepare;
> + }
> +
> + dev_set_drvdata(&mhi_dev->dev, qdev);
> + qdev->bootlog_ch = mhi_dev;
> + return 0;
> +
> +mhi_unprepare:
> + mhi_unprepare_from_transfer(mhi_dev);
> +destroy_workqueue:
> + flush_workqueue(qdev->bootlog_wq);
> + destroy_workqueue(qdev->bootlog_wq);
> +out:
> + return ret;
> +}
> +
> +static void qaic_bootlog_mhi_remove(struct mhi_device *mhi_dev)
> +{
> + struct qaic_device *qdev;
> +
> + qdev = dev_get_drvdata(&mhi_dev->dev);
> +
> + mhi_unprepare_from_transfer(qdev->bootlog_ch);
> + flush_workqueue(qdev->bootlog_wq);
> + destroy_workqueue(qdev->bootlog_wq);
> + qdev->bootlog_ch = NULL;
> +}
> +
> +static void qaic_bootlog_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
> +{
> +}
> +
> +static void qaic_bootlog_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
> +{
> + struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
> + struct bootlog_msg *msg = mhi_result->buf_addr;
> +
> + if (mhi_result->transaction_status) {
> + devm_kfree(&qdev->pdev->dev, msg);
> + return;
> + }
> +
> + /* Force a null at the end of the transferred string */
> + msg->str[mhi_result->bytes_xferd - 1] = 0;
Is it guaranteed that bytes_xferd will always be within valid range here?
> +
> + queue_work(qdev->bootlog_wq, &msg->work);
> +}
> +
> +static const struct mhi_device_id qaic_bootlog_mhi_match_table[] = {
> + { .chan = "QAIC_LOGGING", },
> + {},
> +};
> +
> +static struct mhi_driver qaic_bootlog_mhi_driver = {
> + .id_table = qaic_bootlog_mhi_match_table,
> + .remove = qaic_bootlog_mhi_remove,
> + .probe = qaic_bootlog_mhi_probe,
> + .ul_xfer_cb = qaic_bootlog_mhi_ul_xfer_cb,
> + .dl_xfer_cb = qaic_bootlog_mhi_dl_xfer_cb,
> + .driver = {
> + .name = "qaic_bootlog",
> + },
> +};
> +
> +int qaic_bootlog_register(void)
> +{
> + return mhi_driver_register(&qaic_bootlog_mhi_driver);
> +}
> +
> +void qaic_bootlog_unregister(void)
> +{
> + mhi_driver_unregister(&qaic_bootlog_mhi_driver);
> +}
> diff --git a/drivers/accel/qaic/qaic_debugfs.h b/drivers/accel/qaic/qaic_debugfs.h
> new file mode 100644
> index 000000000000..ea3fd1a88405
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_debugfs.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
> +/* Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */
> +
> +#ifndef __QAIC_DEBUGFS_H__
> +#define __QAIC_DEBUGFS_H__
> +
> +#include <drm/drm_file.h>
> +
> +#ifdef CONFIG_DEBUG_FS
> +int qaic_bootlog_register(void);
> +void qaic_bootlog_unregister(void);
> +void qaic_debugfs_init(struct qaic_drm_device *qddev);
> +#else
> +int qaic_bootlog_register(void) { return 0; }
> +void qaic_bootlog_unregister(void) {}
> +void qaic_debugfs_init(struct qaic_drm_device *qddev) {}
> +#endif /* CONFIG_DEBUG_FS */
> +#endif /* __QAIC_DEBUGFS_H__ */
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index d1a632dbaec6..f072edb74f22 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -28,6 +28,7 @@
>
> #include "mhi_controller.h"
> #include "qaic.h"
> +#include "qaic_debugfs.h"
> #include "qaic_timesync.h"
>
> MODULE_IMPORT_NS(DMA_BUF);
> @@ -229,8 +230,12 @@ static int qaic_create_drm_device(struct qaic_device *qdev, s32 partition_id)
> qddev->partition_id = partition_id;
>
> ret = drm_dev_register(drm, 0);
> - if (ret)
> + if (ret) {
> pci_dbg(qdev->pdev, "drm_dev_register failed %d\n", ret);
> + return ret;
> + }
> +
> + qaic_debugfs_init(qddev);
>
> return ret;
> }
> @@ -380,6 +385,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
> if (ret)
> return NULL;
> ret = drmm_mutex_init(drm, &qdev->cntl_mutex);
> + if (ret)
> + return NULL;
> + ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
> if (ret)
> return NULL;
>
> @@ -399,6 +407,7 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
> qddev->qdev = qdev;
>
> INIT_LIST_HEAD(&qdev->cntl_xfer_list);
> + INIT_LIST_HEAD(&qdev->bootlog);
> INIT_LIST_HEAD(&qddev->users);
>
> for (i = 0; i < qdev->num_dbc; ++i) {
> @@ -639,6 +648,10 @@ static int __init qaic_init(void)
> if (ret)
> pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
>
> + ret = qaic_bootlog_register();
> + if (ret)
> + pr_debug("qaic: qaic_bootlog_register failed %d\n", ret);
> +
> return 0;
>
> free_pci:
> @@ -664,6 +677,7 @@ static void __exit qaic_exit(void)
> * reinitializing the link_up state after the cleanup is done.
> */
> link_up = true;
> + qaic_bootlog_unregister();
> qaic_timesync_deinit();
> mhi_driver_unregister(&qaic_mhi_driver);
> pci_unregister_driver(&qaic_pci_driver);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] accel/qaic: Add fifo size debugfs
2024-03-11 16:58 ` [PATCH 2/3] accel/qaic: Add fifo size debugfs Jeffrey Hugo
2024-03-12 8:45 ` kernel test robot
@ 2024-03-14 11:44 ` Jacek Lawrynowicz
1 sibling, 0 replies; 10+ messages in thread
From: Jacek Lawrynowicz @ 2024-03-14 11:44 UTC (permalink / raw
To: Jeffrey Hugo, quic_carlv, quic_pkanojiy, stanislaw.gruszka
Cc: linux-arm-msm, dri-devel, ogabbay
Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
On 11.03.2024 17:58, Jeffrey Hugo wrote:
> Each DMA Bridge Channel (dbc) has a unique configured fifo size which is
> specified by the userspace client of that dbc. Since the fifo is
> circular, it is useful to know the configured size when debugging
> issues.
>
> Add a per-dbc subdirectory in debugfs and in each subdirectory add a
> fifo_size entry that will display the size of that dbc's fifo when read.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> ---
> drivers/accel/qaic/qaic_debugfs.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
> index 4f87fe29be1a..9d56cd451b64 100644
> --- a/drivers/accel/qaic/qaic_debugfs.c
> +++ b/drivers/accel/qaic/qaic_debugfs.c
> @@ -11,6 +11,7 @@
> #include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/seq_file.h>
> +#include <linux/sprintf.h>
> #include <linux/string.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
> @@ -20,6 +21,7 @@
>
> #define BOOTLOG_POOL_SIZE 16
> #define BOOTLOG_MSG_SIZE 512
> +#define QAIC_DBC_DIR_NAME 9
>
> struct bootlog_msg {
> /* Buffer for bootlog messages */
> @@ -74,14 +76,43 @@ static const struct file_operations bootlog_fops = {
> .release = single_release,
> };
>
> +static int read_dbc_fifo_size(struct seq_file *s, void *unused)
> +{
> + struct dma_bridge_chan *dbc = s->private;
> +
> + seq_printf(s, "%u\n", dbc->nelem);
> + return 0;
> +}
> +
> +static int fifo_size_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, read_dbc_fifo_size, inode->i_private);
> +}
> +
> +static const struct file_operations fifo_size_fops = {
> + .owner = THIS_MODULE,
> + .open = fifo_size_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> void qaic_debugfs_init(struct qaic_drm_device *qddev)
> {
> struct qaic_device *qdev = qddev->qdev;
> struct dentry *debugfs_root;
> + struct dentry *debugfs_dir;
> + char name[QAIC_DBC_DIR_NAME];
> + u32 i;
>
> debugfs_root = to_drm(qddev)->debugfs_root;
>
> debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
> + for (i = 0; i < qdev->num_dbc; ++i) {
> + snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
> + debugfs_dir = debugfs_create_dir(name, debugfs_root);
> + debugfs_create_file("fifo_size", 0400, debugfs_dir, &qdev->dbc[i], &fifo_size_fops);
> + }
> }
>
> static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] accel/qaic: Add fifo queued debugfs
2024-03-11 16:58 ` [PATCH 3/3] accel/qaic: Add fifo queued debugfs Jeffrey Hugo
@ 2024-03-14 11:44 ` Jacek Lawrynowicz
0 siblings, 0 replies; 10+ messages in thread
From: Jacek Lawrynowicz @ 2024-03-14 11:44 UTC (permalink / raw
To: Jeffrey Hugo, quic_carlv, quic_pkanojiy, stanislaw.gruszka
Cc: linux-arm-msm, dri-devel, ogabbay
Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
On 11.03.2024 17:58, Jeffrey Hugo wrote:
> When debugging functional issues with workload input processing, it is
> useful to know if requests are backing up in the fifo, or perhaps
> getting stuck elsewhere. To answer the question of how many requests are
> in the fifo, implement a "queued" debugfs entry per-dbc that returns the
> number of pending requests when read.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
> ---
> drivers/accel/qaic/qaic.h | 1 +
> drivers/accel/qaic/qaic_data.c | 9 +++++++++
> drivers/accel/qaic/qaic_debugfs.c | 31 +++++++++++++++++++++++++++++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
> index 03d9c9fbffb3..02561b6cecc6 100644
> --- a/drivers/accel/qaic/qaic.h
> +++ b/drivers/accel/qaic/qaic.h
> @@ -288,6 +288,7 @@ int disable_dbc(struct qaic_device *qdev, u32 dbc_id, struct qaic_user *usr);
> void enable_dbc(struct qaic_device *qdev, u32 dbc_id, struct qaic_user *usr);
> void wakeup_dbc(struct qaic_device *qdev, u32 dbc_id);
> void release_dbc(struct qaic_device *qdev, u32 dbc_id);
> +void qaic_data_get_fifo_info(struct dma_bridge_chan *dbc, u32 *head, u32 *tail);
>
> void wake_all_cntl(struct qaic_device *qdev);
> void qaic_dev_reset_clean_local_state(struct qaic_device *qdev);
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index 2459fe4a3f95..e86e71c1cdd8 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -1981,3 +1981,12 @@ void release_dbc(struct qaic_device *qdev, u32 dbc_id)
> dbc->in_use = false;
> wake_up(&dbc->dbc_release);
> }
> +
> +void qaic_data_get_fifo_info(struct dma_bridge_chan *dbc, u32 *head, u32 *tail)
> +{
> + if (!dbc || !head || !tail)
> + return;
> +
> + *head = readl(dbc->dbc_base + REQHP_OFF);
> + *tail = readl(dbc->dbc_base + REQTP_OFF);
> +}
> diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
> index 9d56cd451b64..12a65b98701d 100644
> --- a/drivers/accel/qaic/qaic_debugfs.c
> +++ b/drivers/accel/qaic/qaic_debugfs.c
> @@ -97,6 +97,36 @@ static const struct file_operations fifo_size_fops = {
> .release = single_release,
> };
>
> +static int read_dbc_queued(struct seq_file *s, void *unused)
> +{
> + struct dma_bridge_chan *dbc = s->private;
> + u32 tail = 0, head = 0;
> +
> + qaic_data_get_fifo_info(dbc, &head, &tail);
> +
> + if (head == U32_MAX || tail == U32_MAX)
> + seq_printf(s, "%u\n", 0);
> + else if (head > tail)
> + seq_printf(s, "%u\n", dbc->nelem - head + tail);
> + else
> + seq_printf(s, "%u\n", tail - head);
> +
> + return 0;
> +}
> +
> +static int queued_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, read_dbc_queued, inode->i_private);
> +}
> +
> +static const struct file_operations queued_fops = {
> + .owner = THIS_MODULE,
> + .open = queued_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> void qaic_debugfs_init(struct qaic_drm_device *qddev)
> {
> struct qaic_device *qdev = qddev->qdev;
> @@ -112,6 +142,7 @@ void qaic_debugfs_init(struct qaic_drm_device *qddev)
> snprintf(name, QAIC_DBC_DIR_NAME, "dbc%03u", i);
> debugfs_dir = debugfs_create_dir(name, debugfs_root);
> debugfs_create_file("fifo_size", 0400, debugfs_dir, &qdev->dbc[i], &fifo_size_fops);
> + debugfs_create_file("queued", 0400, debugfs_dir, &qdev->dbc[i], &queued_fops);
> }
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] accel/qaic: Add bootlog debugfs
2024-03-14 11:41 ` Jacek Lawrynowicz
@ 2024-03-15 15:39 ` Jeffrey Hugo
2024-03-18 8:49 ` Jacek Lawrynowicz
0 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2024-03-15 15:39 UTC (permalink / raw
To: Jacek Lawrynowicz, quic_carlv, quic_pkanojiy, stanislaw.gruszka
Cc: linux-arm-msm, dri-devel, ogabbay
On 3/14/2024 5:41 AM, Jacek Lawrynowicz wrote:
> Hi,
>
> On 11.03.2024 17:58, Jeffrey Hugo wrote:
>> During the boot process of AIC100, the bootloaders (PBL and SBL) log
>> messages to device RAM. During SBL, if the host opens the QAIC_LOGGING
>> channel, SBL will offload the contents of the log buffer to the host,
>> and stream any new messages that SBL logs.
>>
>> This log of the boot process can be very useful for an initial triage of
>> any boot related issues. For example, if SBL rejects one of the runtime
>> firmware images for a validation failure, SBL will log a reason why.
>>
>> Add the ability of the driver to open the logging channel, receive the
>> messages, and store them. Also define a debugfs entry called "bootlog"
>> by hooking into the DRM debugfs framework. When the bootlog debugfs
>> entry is read, the current contents of the log that the host is caching
>> is displayed to the user. The driver will retain the cache until it
>> detects that the device has rebooted. At that point, the cache will be
>> freed, and the driver will wait for a new log. With this scheme, the
>> driver will only have a cache of the log from the current device boot.
>> Note that if the driver initializes a device and it is already in the
>> runtime state (QSM), no bootlog will be available through this mechanism
>> because the driver and SBL have not communicated.
>>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
>> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>> ---
>> drivers/accel/qaic/Makefile | 2 +
>> drivers/accel/qaic/qaic.h | 8 +
>> drivers/accel/qaic/qaic_debugfs.c | 271 ++++++++++++++++++++++++++++++
>> drivers/accel/qaic/qaic_debugfs.h | 20 +++
>> drivers/accel/qaic/qaic_drv.c | 16 +-
>> 5 files changed, 316 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/accel/qaic/qaic_debugfs.c
>> create mode 100644 drivers/accel/qaic/qaic_debugfs.h
>>
>> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
>> index 3f7f6dfde7f2..2cadcc1baa0e 100644
>> --- a/drivers/accel/qaic/Makefile
>> +++ b/drivers/accel/qaic/Makefile
>> @@ -11,3 +11,5 @@ qaic-y := \
>> qaic_data.o \
>> qaic_drv.o \
>> qaic_timesync.o
>> +
>> +qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
>> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
>> index 9256653b3036..03d9c9fbffb3 100644
>> --- a/drivers/accel/qaic/qaic.h
>> +++ b/drivers/accel/qaic/qaic.h
>> @@ -153,6 +153,14 @@ struct qaic_device {
>> struct mhi_device *qts_ch;
>> /* Work queue for tasks related to MHI "QAIC_TIMESYNC" channel */
>> struct workqueue_struct *qts_wq;
>> + /* Head of list of page allocated by MHI bootlog device */
>> + struct list_head bootlog;
>> + /* MHI bootlog channel device */
>> + struct mhi_device *bootlog_ch;
>> + /* Work queue for tasks related to MHI bootlog device */
>> + struct workqueue_struct *bootlog_wq;
>> + /* Synchronizes access of pages in MHI bootlog device */
>> + struct mutex bootlog_mutex;
>> };
>>
>> struct qaic_drm_device {
>> diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
>> new file mode 100644
>> index 000000000000..4f87fe29be1a
>> --- /dev/null
>> +++ b/drivers/accel/qaic/qaic_debugfs.c
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
>> +/* Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved. */
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/list.h>
>> +#include <linux/mhi.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pci.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "qaic.h"
>> +#include "qaic_debugfs.h"
>> +
>> +#define BOOTLOG_POOL_SIZE 16
>> +#define BOOTLOG_MSG_SIZE 512
>> +
>> +struct bootlog_msg {
>> + /* Buffer for bootlog messages */
>> + char str[BOOTLOG_MSG_SIZE];
>> + /* Root struct of device, used to access device resources */
>> + struct qaic_device *qdev;
>> + /* Work struct to schedule work coming on QAIC_LOGGING channel */
>> + struct work_struct work;
>> +};
>> +
>> +struct bootlog_page {
>> + /* Node in list of bootlog pages maintained by root device struct */
>> + struct list_head node;
>> + /* Total size of the buffer that holds the bootlogs. It is PAGE_SIZE */
>> + unsigned int size;
>> + /* Offset for the next bootlog */
>> + unsigned int offset;
>> +};
>> +
>> +static int bootlog_show(struct seq_file *s, void *unused)
>> +{
>> + struct bootlog_page *page;
>> + struct qaic_device *qdev;
>> + void *page_end;
>> + void *log;
>> +
>> + qdev = s->private;
>> + mutex_lock(&qdev->bootlog_mutex);
>> + list_for_each_entry(page, &qdev->bootlog, node) {
>> + log = page + 1;
>> + page_end = (void *)page + page->offset;
>> + while (log < page_end) {
>> + seq_printf(s, "%s", (char *)log);
>> + log += strlen(log) + 1;
>> + }
>> + }
>> + mutex_unlock(&qdev->bootlog_mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int bootlog_fops_open(struct inode *inode, struct file *file)
>> +{
>> + return single_open(file, bootlog_show, inode->i_private);
>> +}
>> +
>> +static const struct file_operations bootlog_fops = {
>> + .owner = THIS_MODULE,
>> + .open = bootlog_fops_open,
>> + .read = seq_read,
>> + .llseek = seq_lseek,
>> + .release = single_release,
>> +};
>> +
>> +void qaic_debugfs_init(struct qaic_drm_device *qddev)
>> +{
>> + struct qaic_device *qdev = qddev->qdev;
>> + struct dentry *debugfs_root;
>> +
>> + debugfs_root = to_drm(qddev)->debugfs_root;
>> +
>> + debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
>> +}
>> +
>> +static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
>> +{
>> + struct bootlog_page *page;
>> +
>> + page = (struct bootlog_page *)devm_get_free_pages(&qdev->pdev->dev, GFP_KERNEL, 0);
>> + if (!page)
>> + return page;
>> +
>> + page->size = PAGE_SIZE;
>> + page->offset = sizeof(*page);
>> + list_add_tail(&page->node, &qdev->bootlog);
>> +
>> + return page;
>> +}
>> +
>> +static int reset_bootlog(struct qaic_device *qdev)
>> +{
>> + struct bootlog_page *page;
>> + struct bootlog_page *i;
>> +
>> + list_for_each_entry_safe(page, i, &qdev->bootlog, node) {
>> + list_del(&page->node);
>> + devm_free_pages(&qdev->pdev->dev, (unsigned long)page);
>> + }
> This is currently dead code. reset is only used to init the bootlog. You may consider making this init_bootlog() if you are not planning to actually reset the bootlog.
No, its not dead code.
We boot the device the first time. qaic_bootlog_mhi_probe() is called,
which calls reset_bootlog(). This code does not execute as the list is
empty. For this instance, reset_bootlog() is "init_bootlog". We
collect a bootlog and store it in the list. The device finishes
booting, and qaic_bootlog_mhi_remove() is called. We do not clear the
list at that time. This allows the log to be dumped at a later time.
Now, lets assume the device crashes. The device will reboot and go
through the boot flow again. In this example, this will be boot
instance 2, but this also applies to boot instance N+1.
qaic_bootlog_mhi_probe() is called again. Reset_bootlog() will be
called, and now this code will execute because the list is non-empty and
contains data from the previous boot. As we are only storing the
current bootlog, this loop clears the list and frees the resources.
Then we collect the new log for the current boot, and
qaic_bootlog_mhi_remove() is triggered again.
>> +
>> + page = alloc_bootlog_page(qdev);
>> + if (!page)
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +static void *bootlog_get_space(struct qaic_device *qdev, unsigned int size)
>> +{
>> + struct bootlog_page *page;
>> +
>> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
>> +
>> + if (size > page->size - sizeof(*page))
> Not critical but would be safer to use this condition: "sizeof(*page) + size > page->size"
I disagree. Your suggestion would appear to have the potential to
overflow because it is doing a calculation based on an untrusted value
(the size parameter). The current code restructures the check to avoid
this.
What would be safer is to utilize size_add(), which I think is better
than either the current code, or your suggestion, and is what I will
implement.
>
>> + return NULL;
>> +
>> + if (page->offset + size > page->size) {
>> + page = alloc_bootlog_page(qdev);
>> + if (!page)
>> + return NULL;
>> + }
>> +
>> + return (void *)page + page->offset;
>> +}
>> +
>> +static void bootlog_commit(struct qaic_device *qdev, unsigned int size)
>> +{
>> + struct bootlog_page *page;
>> +
>> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
>> +
>> + page->offset += size;
>> +}
>> +
>> +static void bootlog_log(struct work_struct *work)
>> +{
>> + struct bootlog_msg *msg = container_of(work, struct bootlog_msg, work);
>> + unsigned int len = strlen(msg->str) + 1;
>> + struct qaic_device *qdev = msg->qdev;
>> + void *log;
>> +
>> + mutex_lock(&qdev->bootlog_mutex);
>> + log = bootlog_get_space(qdev, len);
>> + if (log) {
>> + memcpy(log, msg, len);
>> + bootlog_commit(qdev, len);
>> + }
>> + mutex_unlock(&qdev->bootlog_mutex);
>> +
>> + if (mhi_queue_buf(qdev->bootlog_ch, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT))
>> + devm_kfree(&qdev->pdev->dev, msg);
> You are freeing `struct work` while still in work callback. This is unsafe.
> See https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2564.
> Work ptr is kept in busy_hash after the callback has finished and may be still be accessed.
Documentation says that is permitted -
https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2548
Also, the framework code documents that the struct work cannot be
accessed after the callback is invoked -
https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2635
>
>> +}
>> +
>> +static int qaic_bootlog_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
>> +{
>> + struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
>> + struct bootlog_msg *msg;
>> + int i, ret;
>> +
>> + qdev->bootlog_wq = alloc_ordered_workqueue("qaic_bootlog", 0);
>> + if (!qdev->bootlog_wq) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + mutex_lock(&qdev->bootlog_mutex);
> Looks like locking should be inside reset_bootlog(), like in other places.
Will do.
>
>> + ret = reset_bootlog(qdev);
>> + mutex_unlock(&qdev->bootlog_mutex);
>> + if (ret)
>> + goto destroy_workqueue;
>> +
>> + ret = mhi_prepare_for_transfer(mhi_dev);
>> + if (ret)
>> + goto destroy_workqueue;
>> +
>> + for (i = 0; i < BOOTLOG_POOL_SIZE; i++) {
>> + msg = devm_kzalloc(&qdev->pdev->dev, sizeof(*msg), GFP_KERNEL);
>> + if (!msg) {
>> + ret = -ENOMEM;
>> + goto mhi_unprepare;
>> + }
>> +
>> + msg->qdev = qdev;
>> + INIT_WORK(&msg->work, bootlog_log);
>> +
>> + ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT);
>> + if (ret)
>> + goto mhi_unprepare;
>> + }
>> +
>> + dev_set_drvdata(&mhi_dev->dev, qdev);
>> + qdev->bootlog_ch = mhi_dev;
>> + return 0;
>> +
>> +mhi_unprepare:
>> + mhi_unprepare_from_transfer(mhi_dev);
>> +destroy_workqueue:
>> + flush_workqueue(qdev->bootlog_wq);
>> + destroy_workqueue(qdev->bootlog_wq);
>> +out:
>> + return ret;
>> +}
>> +
>> +static void qaic_bootlog_mhi_remove(struct mhi_device *mhi_dev)
>> +{
>> + struct qaic_device *qdev;
>> +
>> + qdev = dev_get_drvdata(&mhi_dev->dev);
>> +
>> + mhi_unprepare_from_transfer(qdev->bootlog_ch);
>> + flush_workqueue(qdev->bootlog_wq);
>> + destroy_workqueue(qdev->bootlog_wq);
>> + qdev->bootlog_ch = NULL;
>> +}
>> +
>> +static void qaic_bootlog_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>> +{
>> +}
>> +
>> +static void qaic_bootlog_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>> +{
>> + struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
>> + struct bootlog_msg *msg = mhi_result->buf_addr;
>> +
>> + if (mhi_result->transaction_status) {
>> + devm_kfree(&qdev->pdev->dev, msg);
>> + return;
>> + }
>> +
>> + /* Force a null at the end of the transferred string */
>> + msg->str[mhi_result->bytes_xferd - 1] = 0;
> Is it guaranteed that bytes_xferd will always be within valid range here?
Yes. We provide the buffer size when we queue it to MHI. When the
buffer comes back, before this callback, MHI will clamp the transfered
size to the buffer size.
>
>> +
>> + queue_work(qdev->bootlog_wq, &msg->work);
>> +}
>> +
>> +static const struct mhi_device_id qaic_bootlog_mhi_match_table[] = {
>> + { .chan = "QAIC_LOGGING", },
>> + {},
>> +};
>> +
>> +static struct mhi_driver qaic_bootlog_mhi_driver = {
>> + .id_table = qaic_bootlog_mhi_match_table,
>> + .remove = qaic_bootlog_mhi_remove,
>> + .probe = qaic_bootlog_mhi_probe,
>> + .ul_xfer_cb = qaic_bootlog_mhi_ul_xfer_cb,
>> + .dl_xfer_cb = qaic_bootlog_mhi_dl_xfer_cb,
>> + .driver = {
>> + .name = "qaic_bootlog",
>> + },
>> +};
>> +
>> +int qaic_bootlog_register(void)
>> +{
>> + return mhi_driver_register(&qaic_bootlog_mhi_driver);
>> +}
>> +
>> +void qaic_bootlog_unregister(void)
>> +{
>> + mhi_driver_unregister(&qaic_bootlog_mhi_driver);
>> +}
>> diff --git a/drivers/accel/qaic/qaic_debugfs.h b/drivers/accel/qaic/qaic_debugfs.h
>> new file mode 100644
>> index 000000000000..ea3fd1a88405
>> --- /dev/null
>> +++ b/drivers/accel/qaic/qaic_debugfs.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
>> +/* Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */
>> +
>> +#ifndef __QAIC_DEBUGFS_H__
>> +#define __QAIC_DEBUGFS_H__
>> +
>> +#include <drm/drm_file.h>
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +int qaic_bootlog_register(void);
>> +void qaic_bootlog_unregister(void);
>> +void qaic_debugfs_init(struct qaic_drm_device *qddev);
>> +#else
>> +int qaic_bootlog_register(void) { return 0; }
>> +void qaic_bootlog_unregister(void) {}
>> +void qaic_debugfs_init(struct qaic_drm_device *qddev) {}
>> +#endif /* CONFIG_DEBUG_FS */
>> +#endif /* __QAIC_DEBUGFS_H__ */
>> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
>> index d1a632dbaec6..f072edb74f22 100644
>> --- a/drivers/accel/qaic/qaic_drv.c
>> +++ b/drivers/accel/qaic/qaic_drv.c
>> @@ -28,6 +28,7 @@
>>
>> #include "mhi_controller.h"
>> #include "qaic.h"
>> +#include "qaic_debugfs.h"
>> #include "qaic_timesync.h"
>>
>> MODULE_IMPORT_NS(DMA_BUF);
>> @@ -229,8 +230,12 @@ static int qaic_create_drm_device(struct qaic_device *qdev, s32 partition_id)
>> qddev->partition_id = partition_id;
>>
>> ret = drm_dev_register(drm, 0);
>> - if (ret)
>> + if (ret) {
>> pci_dbg(qdev->pdev, "drm_dev_register failed %d\n", ret);
>> + return ret;
>> + }
>> +
>> + qaic_debugfs_init(qddev);
>>
>> return ret;
>> }
>> @@ -380,6 +385,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>> if (ret)
>> return NULL;
>> ret = drmm_mutex_init(drm, &qdev->cntl_mutex);
>> + if (ret)
>> + return NULL;
>> + ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
>> if (ret)
>> return NULL;
>>
>> @@ -399,6 +407,7 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>> qddev->qdev = qdev;
>>
>> INIT_LIST_HEAD(&qdev->cntl_xfer_list);
>> + INIT_LIST_HEAD(&qdev->bootlog);
>> INIT_LIST_HEAD(&qddev->users);
>>
>> for (i = 0; i < qdev->num_dbc; ++i) {
>> @@ -639,6 +648,10 @@ static int __init qaic_init(void)
>> if (ret)
>> pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
>>
>> + ret = qaic_bootlog_register();
>> + if (ret)
>> + pr_debug("qaic: qaic_bootlog_register failed %d\n", ret);
>> +
>> return 0;
>>
>> free_pci:
>> @@ -664,6 +677,7 @@ static void __exit qaic_exit(void)
>> * reinitializing the link_up state after the cleanup is done.
>> */
>> link_up = true;
>> + qaic_bootlog_unregister();
>> qaic_timesync_deinit();
>> mhi_driver_unregister(&qaic_mhi_driver);
>> pci_unregister_driver(&qaic_pci_driver);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] accel/qaic: Add bootlog debugfs
2024-03-15 15:39 ` Jeffrey Hugo
@ 2024-03-18 8:49 ` Jacek Lawrynowicz
0 siblings, 0 replies; 10+ messages in thread
From: Jacek Lawrynowicz @ 2024-03-18 8:49 UTC (permalink / raw
To: Jeffrey Hugo, quic_carlv, quic_pkanojiy, stanislaw.gruszka
Cc: linux-arm-msm, dri-devel, ogabbay
On 15.03.2024 16:39, Jeffrey Hugo wrote:
> On 3/14/2024 5:41 AM, Jacek Lawrynowicz wrote:
>> Hi,
>>
>> On 11.03.2024 17:58, Jeffrey Hugo wrote:
>>> During the boot process of AIC100, the bootloaders (PBL and SBL) log
>>> messages to device RAM. During SBL, if the host opens the QAIC_LOGGING
>>> channel, SBL will offload the contents of the log buffer to the host,
>>> and stream any new messages that SBL logs.
>>>
>>> This log of the boot process can be very useful for an initial triage of
>>> any boot related issues. For example, if SBL rejects one of the runtime
>>> firmware images for a validation failure, SBL will log a reason why.
>>>
>>> Add the ability of the driver to open the logging channel, receive the
>>> messages, and store them. Also define a debugfs entry called "bootlog"
>>> by hooking into the DRM debugfs framework. When the bootlog debugfs
>>> entry is read, the current contents of the log that the host is caching
>>> is displayed to the user. The driver will retain the cache until it
>>> detects that the device has rebooted. At that point, the cache will be
>>> freed, and the driver will wait for a new log. With this scheme, the
>>> driver will only have a cache of the log from the current device boot.
>>> Note that if the driver initializes a device and it is already in the
>>> runtime state (QSM), no bootlog will be available through this mechanism
>>> because the driver and SBL have not communicated.
>>>
>>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>> Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
>>> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
>>> ---
>>> drivers/accel/qaic/Makefile | 2 +
>>> drivers/accel/qaic/qaic.h | 8 +
>>> drivers/accel/qaic/qaic_debugfs.c | 271 ++++++++++++++++++++++++++++++
>>> drivers/accel/qaic/qaic_debugfs.h | 20 +++
>>> drivers/accel/qaic/qaic_drv.c | 16 +-
>>> 5 files changed, 316 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/accel/qaic/qaic_debugfs.c
>>> create mode 100644 drivers/accel/qaic/qaic_debugfs.h
>>>
>>> diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
>>> index 3f7f6dfde7f2..2cadcc1baa0e 100644
>>> --- a/drivers/accel/qaic/Makefile
>>> +++ b/drivers/accel/qaic/Makefile
>>> @@ -11,3 +11,5 @@ qaic-y := \
>>> qaic_data.o \
>>> qaic_drv.o \
>>> qaic_timesync.o
>>> +
>>> +qaic-$(CONFIG_DEBUG_FS) += qaic_debugfs.o
>>> diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
>>> index 9256653b3036..03d9c9fbffb3 100644
>>> --- a/drivers/accel/qaic/qaic.h
>>> +++ b/drivers/accel/qaic/qaic.h
>>> @@ -153,6 +153,14 @@ struct qaic_device {
>>> struct mhi_device *qts_ch;
>>> /* Work queue for tasks related to MHI "QAIC_TIMESYNC" channel */
>>> struct workqueue_struct *qts_wq;
>>> + /* Head of list of page allocated by MHI bootlog device */
>>> + struct list_head bootlog;
>>> + /* MHI bootlog channel device */
>>> + struct mhi_device *bootlog_ch;
>>> + /* Work queue for tasks related to MHI bootlog device */
>>> + struct workqueue_struct *bootlog_wq;
>>> + /* Synchronizes access of pages in MHI bootlog device */
>>> + struct mutex bootlog_mutex;
>>> };
>>> struct qaic_drm_device {
>>> diff --git a/drivers/accel/qaic/qaic_debugfs.c b/drivers/accel/qaic/qaic_debugfs.c
>>> new file mode 100644
>>> index 000000000000..4f87fe29be1a
>>> --- /dev/null
>>> +++ b/drivers/accel/qaic/qaic_debugfs.c
>>> @@ -0,0 +1,271 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
>>> +/* Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved. */
>>> +
>>> +#include <linux/debugfs.h>
>>> +#include <linux/device.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/list.h>
>>> +#include <linux/mhi.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/seq_file.h>
>>> +#include <linux/string.h>
>>> +#include <linux/types.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#include "qaic.h"
>>> +#include "qaic_debugfs.h"
>>> +
>>> +#define BOOTLOG_POOL_SIZE 16
>>> +#define BOOTLOG_MSG_SIZE 512
>>> +
>>> +struct bootlog_msg {
>>> + /* Buffer for bootlog messages */
>>> + char str[BOOTLOG_MSG_SIZE];
>>> + /* Root struct of device, used to access device resources */
>>> + struct qaic_device *qdev;
>>> + /* Work struct to schedule work coming on QAIC_LOGGING channel */
>>> + struct work_struct work;
>>> +};
>>> +
>>> +struct bootlog_page {
>>> + /* Node in list of bootlog pages maintained by root device struct */
>>> + struct list_head node;
>>> + /* Total size of the buffer that holds the bootlogs. It is PAGE_SIZE */
>>> + unsigned int size;
>>> + /* Offset for the next bootlog */
>>> + unsigned int offset;
>>> +};
>>> +
>>> +static int bootlog_show(struct seq_file *s, void *unused)
>>> +{
>>> + struct bootlog_page *page;
>>> + struct qaic_device *qdev;
>>> + void *page_end;
>>> + void *log;
>>> +
>>> + qdev = s->private;
>>> + mutex_lock(&qdev->bootlog_mutex);
>>> + list_for_each_entry(page, &qdev->bootlog, node) {
>>> + log = page + 1;
>>> + page_end = (void *)page + page->offset;
>>> + while (log < page_end) {
>>> + seq_printf(s, "%s", (char *)log);
>>> + log += strlen(log) + 1;
>>> + }
>>> + }
>>> + mutex_unlock(&qdev->bootlog_mutex);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int bootlog_fops_open(struct inode *inode, struct file *file)
>>> +{
>>> + return single_open(file, bootlog_show, inode->i_private);
>>> +}
>>> +
>>> +static const struct file_operations bootlog_fops = {
>>> + .owner = THIS_MODULE,
>>> + .open = bootlog_fops_open,
>>> + .read = seq_read,
>>> + .llseek = seq_lseek,
>>> + .release = single_release,
>>> +};
>>> +
>>> +void qaic_debugfs_init(struct qaic_drm_device *qddev)
>>> +{
>>> + struct qaic_device *qdev = qddev->qdev;
>>> + struct dentry *debugfs_root;
>>> +
>>> + debugfs_root = to_drm(qddev)->debugfs_root;
>>> +
>>> + debugfs_create_file("bootlog", 0400, debugfs_root, qdev, &bootlog_fops);
>>> +}
>>> +
>>> +static struct bootlog_page *alloc_bootlog_page(struct qaic_device *qdev)
>>> +{
>>> + struct bootlog_page *page;
>>> +
>>> + page = (struct bootlog_page *)devm_get_free_pages(&qdev->pdev->dev, GFP_KERNEL, 0);
>>> + if (!page)
>>> + return page;
>>> +
>>> + page->size = PAGE_SIZE;
>>> + page->offset = sizeof(*page);
>>> + list_add_tail(&page->node, &qdev->bootlog);
>>> +
>>> + return page;
>>> +}
>>> +
>>> +static int reset_bootlog(struct qaic_device *qdev)
>>> +{
>>> + struct bootlog_page *page;
>>> + struct bootlog_page *i;
>>> +
>>> + list_for_each_entry_safe(page, i, &qdev->bootlog, node) {
>>> + list_del(&page->node);
>>> + devm_free_pages(&qdev->pdev->dev, (unsigned long)page);
>>> + }
>> This is currently dead code. reset is only used to init the bootlog. You may consider making this init_bootlog() if you are not planning to actually reset the bootlog.
>
> No, its not dead code.
>
> We boot the device the first time. qaic_bootlog_mhi_probe() is called, which calls reset_bootlog(). This code does not execute as the list is empty. For this instance, reset_bootlog() is "init_bootlog". We collect a bootlog and store it in the list. The device finishes booting, and qaic_bootlog_mhi_remove() is called. We do not clear the list at that time. This allows the log to be dumped at a later time.
>
> Now, lets assume the device crashes. The device will reboot and go through the boot flow again. In this example, this will be boot instance 2, but this also applies to boot instance N+1.
>
> qaic_bootlog_mhi_probe() is called again. Reset_bootlog() will be called, and now this code will execute because the list is non-empty and contains data from the previous boot. As we are only storing the current bootlog, this loop clears the list and frees the resources. Then we collect the new log for the current boot, and qaic_bootlog_mhi_remove() is triggered again.
OK, make sense.
>>> +
>>> + page = alloc_bootlog_page(qdev);
>>> + if (!page)
>>> + return -ENOMEM;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void *bootlog_get_space(struct qaic_device *qdev, unsigned int size)
>>> +{
>>> + struct bootlog_page *page;
>>> +
>>> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
>>> +
>>> + if (size > page->size - sizeof(*page))
>> Not critical but would be safer to use this condition: "sizeof(*page) + size > page->size"
>
> I disagree. Your suggestion would appear to have the potential to overflow because it is doing a calculation based on an untrusted value (the size parameter). The current code restructures the check to avoid this.
>
> What would be safer is to utilize size_add(), which I think is better than either the current code, or your suggestion, and is what I will implement.
Yeah, size_add() seems to be the best solution here.
>>
>>> + return NULL;
>>> +
>>> + if (page->offset + size > page->size) {
>>> + page = alloc_bootlog_page(qdev);
>>> + if (!page)
>>> + return NULL;
>>> + }
>>> +
>>> + return (void *)page + page->offset;
>>> +}
>>> +
>>> +static void bootlog_commit(struct qaic_device *qdev, unsigned int size)
>>> +{
>>> + struct bootlog_page *page;
>>> +
>>> + page = list_last_entry(&qdev->bootlog, struct bootlog_page, node);
>>> +
>>> + page->offset += size;
>>> +}
>>> +
>>> +static void bootlog_log(struct work_struct *work)
>>> +{
>>> + struct bootlog_msg *msg = container_of(work, struct bootlog_msg, work);
>>> + unsigned int len = strlen(msg->str) + 1;
>>> + struct qaic_device *qdev = msg->qdev;
>>> + void *log;
>>> +
>>> + mutex_lock(&qdev->bootlog_mutex);
>>> + log = bootlog_get_space(qdev, len);
>>> + if (log) {
>>> + memcpy(log, msg, len);
>>> + bootlog_commit(qdev, len);
>>> + }
>>> + mutex_unlock(&qdev->bootlog_mutex);
>>> +
>>> + if (mhi_queue_buf(qdev->bootlog_ch, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT))
>>> + devm_kfree(&qdev->pdev->dev, msg);
>> You are freeing `struct work` while still in work callback. This is unsafe.
>> See https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2564.
>> Work ptr is kept in busy_hash after the callback has finished and may be still be accessed.
>
> Documentation says that is permitted - https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2548
>
> Also, the framework code documents that the struct work cannot be accessed after the callback is invoked - https://elixir.bootlin.com/linux/v6.8/source/kernel/workqueue.c#L2635
It looks to me as find_worker_executing_work() may access the work data but maybe I'm missinterpreteing the code.
Anyway, this would be a kernel bug rather then yours.
>>
>>> +}
>>> +
>>> +static int qaic_bootlog_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
>>> +{
>>> + struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
>>> + struct bootlog_msg *msg;
>>> + int i, ret;
>>> +
>>> + qdev->bootlog_wq = alloc_ordered_workqueue("qaic_bootlog", 0);
>>> + if (!qdev->bootlog_wq) {
>>> + ret = -ENOMEM;
>>> + goto out;
>>> + }
>>> +
>>> + mutex_lock(&qdev->bootlog_mutex);
>> Looks like locking should be inside reset_bootlog(), like in other places.
>
> Will do.
>
>>
>>> + ret = reset_bootlog(qdev);
>>> + mutex_unlock(&qdev->bootlog_mutex);
>>> + if (ret)
>>> + goto destroy_workqueue;
>>> +
>>> + ret = mhi_prepare_for_transfer(mhi_dev);
>>> + if (ret)
>>> + goto destroy_workqueue;
>>> +
>>> + for (i = 0; i < BOOTLOG_POOL_SIZE; i++) {
>>> + msg = devm_kzalloc(&qdev->pdev->dev, sizeof(*msg), GFP_KERNEL);
>>> + if (!msg) {
>>> + ret = -ENOMEM;
>>> + goto mhi_unprepare;
>>> + }
>>> +
>>> + msg->qdev = qdev;
>>> + INIT_WORK(&msg->work, bootlog_log);
>>> +
>>> + ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, msg, BOOTLOG_MSG_SIZE, MHI_EOT);
>>> + if (ret)
>>> + goto mhi_unprepare;
>>> + }
>>> +
>>> + dev_set_drvdata(&mhi_dev->dev, qdev);
>>> + qdev->bootlog_ch = mhi_dev;
>>> + return 0;
>>> +
>>> +mhi_unprepare:
>>> + mhi_unprepare_from_transfer(mhi_dev);
>>> +destroy_workqueue:
>>> + flush_workqueue(qdev->bootlog_wq);
>>> + destroy_workqueue(qdev->bootlog_wq);
>>> +out:
>>> + return ret;
>>> +}
>>> +
>>> +static void qaic_bootlog_mhi_remove(struct mhi_device *mhi_dev)
>>> +{
>>> + struct qaic_device *qdev;
>>> +
>>> + qdev = dev_get_drvdata(&mhi_dev->dev);
>>> +
>>> + mhi_unprepare_from_transfer(qdev->bootlog_ch);
>>> + flush_workqueue(qdev->bootlog_wq);
>>> + destroy_workqueue(qdev->bootlog_wq);
>>> + qdev->bootlog_ch = NULL;
>>> +}
>>> +
>>> +static void qaic_bootlog_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>>> +{
>>> +}
>>> +
>>> +static void qaic_bootlog_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>>> +{
>>> + struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + struct bootlog_msg *msg = mhi_result->buf_addr;
>>> +
>>> + if (mhi_result->transaction_status) {
>>> + devm_kfree(&qdev->pdev->dev, msg);
>>> + return;
>>> + }
>>> +
>>> + /* Force a null at the end of the transferred string */
>>> + msg->str[mhi_result->bytes_xferd - 1] = 0;
>> Is it guaranteed that bytes_xferd will always be within valid range here?
>
> Yes. We provide the buffer size when we queue it to MHI. When the buffer comes back, before this callback, MHI will clamp the transfered size to the buffer size.
>
>>
>>> +
>>> + queue_work(qdev->bootlog_wq, &msg->work);
>>> +}
>>> +
>>> +static const struct mhi_device_id qaic_bootlog_mhi_match_table[] = {
>>> + { .chan = "QAIC_LOGGING", },
>>> + {},
>>> +};
>>> +
>>> +static struct mhi_driver qaic_bootlog_mhi_driver = {
>>> + .id_table = qaic_bootlog_mhi_match_table,
>>> + .remove = qaic_bootlog_mhi_remove,
>>> + .probe = qaic_bootlog_mhi_probe,
>>> + .ul_xfer_cb = qaic_bootlog_mhi_ul_xfer_cb,
>>> + .dl_xfer_cb = qaic_bootlog_mhi_dl_xfer_cb,
>>> + .driver = {
>>> + .name = "qaic_bootlog",
>>> + },
>>> +};
>>> +
>>> +int qaic_bootlog_register(void)
>>> +{
>>> + return mhi_driver_register(&qaic_bootlog_mhi_driver);
>>> +}
>>> +
>>> +void qaic_bootlog_unregister(void)
>>> +{
>>> + mhi_driver_unregister(&qaic_bootlog_mhi_driver);
>>> +}
>>> diff --git a/drivers/accel/qaic/qaic_debugfs.h b/drivers/accel/qaic/qaic_debugfs.h
>>> new file mode 100644
>>> index 000000000000..ea3fd1a88405
>>> --- /dev/null
>>> +++ b/drivers/accel/qaic/qaic_debugfs.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
>>> +/* Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */
>>> +
>>> +#ifndef __QAIC_DEBUGFS_H__
>>> +#define __QAIC_DEBUGFS_H__
>>> +
>>> +#include <drm/drm_file.h>
>>> +
>>> +#ifdef CONFIG_DEBUG_FS
>>> +int qaic_bootlog_register(void);
>>> +void qaic_bootlog_unregister(void);
>>> +void qaic_debugfs_init(struct qaic_drm_device *qddev);
>>> +#else
>>> +int qaic_bootlog_register(void) { return 0; }
>>> +void qaic_bootlog_unregister(void) {}
>>> +void qaic_debugfs_init(struct qaic_drm_device *qddev) {}
>>> +#endif /* CONFIG_DEBUG_FS */
>>> +#endif /* __QAIC_DEBUGFS_H__ */
>>> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
>>> index d1a632dbaec6..f072edb74f22 100644
>>> --- a/drivers/accel/qaic/qaic_drv.c
>>> +++ b/drivers/accel/qaic/qaic_drv.c
>>> @@ -28,6 +28,7 @@
>>> #include "mhi_controller.h"
>>> #include "qaic.h"
>>> +#include "qaic_debugfs.h"
>>> #include "qaic_timesync.h"
>>> MODULE_IMPORT_NS(DMA_BUF);
>>> @@ -229,8 +230,12 @@ static int qaic_create_drm_device(struct qaic_device *qdev, s32 partition_id)
>>> qddev->partition_id = partition_id;
>>> ret = drm_dev_register(drm, 0);
>>> - if (ret)
>>> + if (ret) {
>>> pci_dbg(qdev->pdev, "drm_dev_register failed %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + qaic_debugfs_init(qddev);
>>> return ret;
>>> }
>>> @@ -380,6 +385,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>>> if (ret)
>>> return NULL;
>>> ret = drmm_mutex_init(drm, &qdev->cntl_mutex);
>>> + if (ret)
>>> + return NULL;
>>> + ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
>>> if (ret)
>>> return NULL;
>>> @@ -399,6 +407,7 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev, const struct pci_de
>>> qddev->qdev = qdev;
>>> INIT_LIST_HEAD(&qdev->cntl_xfer_list);
>>> + INIT_LIST_HEAD(&qdev->bootlog);
>>> INIT_LIST_HEAD(&qddev->users);
>>> for (i = 0; i < qdev->num_dbc; ++i) {
>>> @@ -639,6 +648,10 @@ static int __init qaic_init(void)
>>> if (ret)
>>> pr_debug("qaic: qaic_timesync_init failed %d\n", ret);
>>> + ret = qaic_bootlog_register();
>>> + if (ret)
>>> + pr_debug("qaic: qaic_bootlog_register failed %d\n", ret);
>>> +
>>> return 0;
>>> free_pci:
>>> @@ -664,6 +677,7 @@ static void __exit qaic_exit(void)
>>> * reinitializing the link_up state after the cleanup is done.
>>> */
>>> link_up = true;
>>> + qaic_bootlog_unregister();
>>> qaic_timesync_deinit();
>>> mhi_driver_unregister(&qaic_mhi_driver);
>>> pci_unregister_driver(&qaic_pci_driver);
>
Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-18 8:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-11 16:58 [PATCH 0/3] accel/qaic: Add debugfs entries Jeffrey Hugo
2024-03-11 16:58 ` [PATCH 1/3] accel/qaic: Add bootlog debugfs Jeffrey Hugo
2024-03-14 11:41 ` Jacek Lawrynowicz
2024-03-15 15:39 ` Jeffrey Hugo
2024-03-18 8:49 ` Jacek Lawrynowicz
2024-03-11 16:58 ` [PATCH 2/3] accel/qaic: Add fifo size debugfs Jeffrey Hugo
2024-03-12 8:45 ` kernel test robot
2024-03-14 11:44 ` Jacek Lawrynowicz
2024-03-11 16:58 ` [PATCH 3/3] accel/qaic: Add fifo queued debugfs Jeffrey Hugo
2024-03-14 11:44 ` Jacek Lawrynowicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).