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

[Tudor Ambarus]

Hi, Krzysztof,

On 3/5/25 7:37 PM, Krzysztof Kozlowski wrote:
> On 24/02/2025 09:01, Tudor Ambarus wrote:

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

okay, will add.

cut

>> +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.

okay, will create per device instances.

>> +
>> +static void acpm_log_print_entry(struct acpm_info *acpm,
>> +				 const union acpm_log_entry *log_entry)
>> +{

cut

>> +		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.
> 

This is a logging feature, it's not oops/panic related. These logs are
referred to as "block logs". A "block" is the start of a mailbox command
to its end, so it logs every ACPM mailbox command issued to the
firmware. After each end of a block, we see the state of all regulators,
frequencies and devices up/down extracted from the block.

These are indeed system logs, and using the dmesg ring buffer seems fine
as we typically care about the recent logs, we don't care if the ring
starts all over again.

>> +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.
> 

This debugfs entry controls which SRAM log ring buffers are queried -
there are 2 SRAM buffers.

I guess you thought about the "logb_gprio_level" entry, which controls
the ACPM print verbosity. 0xf being ERR, and 0x0 all logs. The firmware
defaults on printing all logs. I can't tell right now other way of
controlling the logs verbosity, maybe via sysfs, but it's equivalent and
with more restrictions. Or maybe you think that I shall always print all
logs?

Thanks,
ta