Re: [PATCH v4 0/3] hwmon: (pmbus/adm1266) add clear_blackbox and powerup_counter debugfs entries

[Abdurrahman Hussain]

On Wed May 20, 2026 at 1:59 PM PDT, Guenter Roeck wrote:
> On 5/20/26 12:40, Abdurrahman Hussain wrote:
>> On Wed May 20, 2026 at 10:10 AM PDT, Abdurrahman Hussain wrote:
>>> On Wed May 20, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On 5/16/26 18:18, Abdurrahman Hussain wrote:
>>>>> This is what remains of the v3 series after Guenter applied patches
>>>>> 1/5 (firmware_revision) and 5/5 (GPIO line label) to hwmon-next, and
>>>>> asked for patch 4/5 (rtc_class) to be dropped.
>>>>>
>>>>> Patch 1 adds a write-only clear_blackbox debugfs file. Devices
>>>>> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
>>>>> an explicit clear once the 32-record buffer fills; the documented
>>>>> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
>>>>> from userspace. The patch also acquires pmbus_lock at the
>>>>> adm1266_nvmem_read() callback boundary so the memset of
>>>>> data->dev_mem, the blackbox refill, and the memcpy to userspace run
>>>>> as a single critical section -- the nvmem core does not serialize
>>>>> concurrent reg_read calls.
>>>>>
>>>>> Patch 2 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
>>>>> The same value is embedded in every blackbox record, so the live
>>>>> value lets userspace match a captured record back to the boot it
>>>>> came from when correlating logs. The block-read is taken under
>>>>> pmbus_lock to serialise against any pmbus_core PAGE+register
>>>>> sequence on the device.
>>>>>
>>>>> Patch 3 takes pmbus_lock in adm1266_state_read() (the pre-existing
>>>>> sequencer_state debugfs handler) for the same defensive-locking
>>>>> reason that motivates the new debugfs files in patches 1 and 2:
>>>>> any direct device access from outside pmbus_core should be ordered
>>>>> with respect to pmbus_core's own PAGE+register sequences.
>>>>>
>>>>> Signed-off-by: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
>>>>
>>>> The series no longer applies after applying the bug fix series.
>>>> Please rebase it on top of the hwmon-next branch of
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>>>> and resubmit.
>>>>
>>>> Sorry for the trouble, and thanks a lot for fixing all the problems
>>>> with the driver.
>>>>
>>>> Guenter
>>>
>>> Will do, thank you for your support!
>>>
>>> Abdurrahman
>> 
>> Hi Guenter,
>> 
>> Before I send v5 of the adm1266 series, I'd like to revisit the
>> SET_RTC exposure question from your v3 review [1].
>> 
>> The use case I keep landing on is the one the datasheet itself
>> recommends: a userspace agent (chrony hook, systemd-timesyncd
>> script, or a small periodic daemon) keeps the chip's seconds
>> counter aligned with wall-clock so the value embedded in each
>> blackbox record stays correct across long uptimes.  The probe-
>> time ktime_get_real_seconds() seed (now in hwmon-next) only fixes
>> this at boot; the chip's counter drifts after that.
>> 
>> You ruled out rtc_class and called a kernel-side periodic timer
>> "a bit excessive", which I agree, it is.  That leaves a
>> userspace-triggered sync.  Two debugfs shapes I'd consider, both under
>> pmbus/<hwmon>/adm1266/ (alongside the clear_blackbox,
>> powerup_counter, and sequencer_state entries v5 already adds):
>> 
>>    A. RW since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch.
>>       Read returns the chip's SET_RTC seconds counter, write sets
>>       it.  The read has the useful side benefit of letting
>>       userspace measure host-vs-chip drift without writing.
>> 
>>    B. Write-only set_rtc -- writing anything to the file makes
>>       the driver read ktime_get_real_seconds() itself and push it
>>       to SET_RTC.  Simpler interface (kernel owns the time
>>       source; userspace just triggers the sync), no payload to
>>       parse, no way for userspace to pass in a wrong value.
>> 
>
> How about a combination ? read returns the current value, anything
> written synchronizes with the kernel rtc.
>

Going with that for v5.  One naming question before I send:

  - set_rtc -- matches the PMBus command name (SET_RTC = 0xDF), but
    "set" reads as write-only at first glance, which is a bit
    misleading for an RW file.
  - since_epoch -- mirrors /sys/class/rtc/<dev>/since_epoch, but
    that one is read-only in the RTC subsystem so the dual
    semantic might surprise users coming from there.
  - rtc_sync -- describes the write semantic directly, but leaves
    the read side unnamed.
  - rtc -- shortest and most neutral; doesn't bias toward either
    operation.

Any preference, or should I just pick?

Thanks,
Abdurrahman