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

[Krzysztof Kozlowski]

On 12/03/2025 08:11, Tudor Ambarus wrote:
> 
>>> +		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.

I claim these are not system logs, but firmware logs, thus dmesg ring
buffer is not suitable for them. Where do the other vendors log remoteprocs?


Best regards,
Krzysztof