Re: [PATCH 3/3] firmware: samsung: add ACPM debugfs support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/02/2025 09:01, Tudor Ambarus wrote:
> The ACPM firmware saves debug information to SRAM. Add debugfs entries
> in order to expose the ACPM logs.
> 
> acpm_framework/logb_gprio_level controls the ACPM print verbosity to
> the SRAM log buffer. It encodes a 64 bit value, 4 bits for each of the
> 16 Plugin IDs, with verbosity levels from 0xf (log error) to
> 0x0 (log debug).
> 
> echo 0xffffffffffffff1f > /sys/kernel/debug/acpm_framework/logb_gprio_level
> Will allow only LOG_ERR prints for all Plugin IDs but Plugin ID 1,
> which will issue prints for any log levels greater or equal to 1.
> On the ACPM firmware side, logb_gprio_level has a default value of zero,
> all logs enabled for all Plugin IDs.
> 
> acpm_framework/log_level has a maximum value of 2 and controls which
> SRAM log buffers are printed.
> 
> Finally, acpm_framework/acpm_debug_cmd provides a way to issue
> ACPM DEBUG commands to the firmware.

Please add something like above also as a comment to the driver, so the
interface will be documented.

> 
> Add ACPM debugfs support with the above capabilities.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---


...

> +
> +union acpm_log_entry {
> +	u32 raw[4];
> +	struct {
> +		u32 systicks0 : 24;
> +		u32 dummy : 2;
> +		u32 is_err : 1;
> +		u32 is_raw : 1;
> +		u32 plugin_id : 4;
> +		u32 systicks24;
> +		u32 msg : 24;
> +		u32 systicks56 : 8;
> +		u32 data;
> +	} __packed;
> +};
> +
> +static struct dentry *rootdir;

exynos-apcm.c is not a singleton, so neither should this be. You should
create entries per device (so with device name as subdirectory), just
for correctness.

> +
> +static DEFINE_MUTEX(acpm_log_level_mutex);

And this also looks per-device-instance.

> +
> +static void acpm_log_print_entry(struct acpm_info *acpm,
> +				 const union acpm_log_entry *log_entry)
> +{
> +	u64 systicks, time, msg;
> +
> +	if (log_entry->is_err)
> +		return;
> +
> +	if (log_entry->is_raw) {
> +		dev_info(acpm->dev, "[ACPM_FW raw] : id:%u, %x, %x, %x\n",
> +			 log_entry->plugin_id, log_entry->raw[1],
> +			 log_entry->raw[2], log_entry->raw[3]);
> +	} else {
> +		systicks = ((u64)(log_entry->systicks56) << 56) +
> +			   ((u64)(log_entry->systicks24) << 24) +
> +			   log_entry->systicks0;
> +
> +		/* report time in ns */
> +		time = mul_u64_u32_div(systicks, ACPM_APM_SYSTICK_PERIOD_PS,
> +				       1000);
> +
> +		msg = readl(acpm->sram_base + log_entry->msg);
> +
> +		dev_info(acpm->dev, "[ACPM_FW] : %llu id:%u, %s, %x\n", time,
> +			 log_entry->plugin_id, (char *)&msg, log_entry->data);


I don't think these should be printed to dmesg - these are not system
logs. You either return the contents to the caller's read() on debugfs
entry or, if this is anyhow crashdump related, it goes to
pstore/minidump once triggered. Or to ramoops.

Depends what these logs are (so please also explain what do you find
there in the commit msg).

Maybe something like CHROMEOS_PSTORE?

IOW, if enabled, this should go to ramoops/pstore unconditionally. For
runtime debugging this should be returned somehow to the userspace
reading the file. I think usually debugfs and sysfs is not expected to
provide more than PAGE_SIZE data, so this second part has to be
rethinked still.

> +	}
> +}
> +
> +static void acpm_log_print_entries(struct acpm_info *acpm,
> +				   struct acpm_log_buf *lbuf)
> +{
> +	union acpm_log_entry log_entry = {0};
> +	u32 front, rear;
> +
> +	front = readl(lbuf->q.front);
> +	rear = lbuf->rear_index;
> +
> +	while (rear != front) {
> +		__ioread32_copy(&log_entry, lbuf->q.base + lbuf->mlen * rear,
> +				sizeof(log_entry) / 4);
> +
> +		acpm_log_print_entry(acpm, &log_entry);
> +
> +		if (lbuf->qlen == rear + 1)
> +			rear = 0;
> +		else
> +			rear++;
> +
> +		lbuf->rear_index = rear;
> +		front = readl(lbuf->q.front);
> +	}
> +}
> +
> +static void acpm_log_print(struct acpm_info *acpm)
> +{
> +	struct acpm_log_info *acpm_log = acpm->log;
> +
> +	guard(mutex)(&acpm_log_level_mutex);
> +
> +	if (acpm_log->level == 0)
> +		return;
> +
> +	if (acpm_log->level == ACPM_LOG_LEVEL_MAX)
> +		acpm_log_print_entries(acpm, &acpm_log->preempt);
> +
> +	acpm_log_print_entries(acpm, &acpm_log->normal);
> +}
> +
> +static void acpm_work_fn(struct work_struct *work)
> +{
> +	struct acpm_log_info *acpm_log =
> +		container_of(work, struct acpm_log_info, work.work);
> +	struct acpm_info *acpm = acpm_log->acpm;
> +
> +	acpm_log_print(acpm);
> +
> +	queue_delayed_work(acpm_log->wq, &acpm_log->work,
> +			   msecs_to_jiffies(acpm_log->poll_period));
> +}
> +
> +static int acpm_log_level_get(void *data, u64 *val)
> +{
> +	struct acpm_info *acpm = data;
> +
> +	*val = acpm->log->level;
> +
> +	return 0;
> +}
> +
> +static int acpm_log_level_set(void *data, u64 val)
> +{
> +	struct acpm_info *acpm = data;
> +	struct acpm_log_info *acpm_log = acpm->log;
> +
> +	if (val > ACPM_LOG_LEVEL_MAX) {
> +		dev_err(acpm->dev, "Log level %llu out of range [0:%u]!\n",
> +			val, ACPM_LOG_LEVEL_MAX);
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &acpm_log_level_mutex)
> +		acpm_log->level = val;
> +
> +	if (acpm_log->level == 0)
> +		cancel_delayed_work_sync(&acpm_log->work);
> +	else
> +		queue_delayed_work(acpm_log->wq, &acpm_log->work,
> +				   msecs_to_jiffies(acpm_log->poll_period));
> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(acpm_log_level_fops, acpm_log_level_get,
> +			 acpm_log_level_set, "0%llu\n");

I also do not think debugfs is a knob to control loglevel of messages
going to dmesg.

Best regards,
Krzysztof