Re: [PATCH v2 3/3] hwmon: pmbus: add MPQ8646 driver

Hi Mr. Bot,

> [Severity: High]
> Is it safe to pass stack-allocated buffers to i2c_transfer() here?
> If the underlying I2C adapter driver uses DMA for transfers, dma_map_single()
> will attempt to map these stack addresses. On systems with virtually mapped
> stacks, this violates DMA mapping rules and could lead to memory corruption.

It was already argued into v1, no change. See v1's answer to your analysis.

> [Severity: Low]
> Does this hook effectively intercept page writes?
> The write_byte callback is used for the PMBus Send Byte protocol, but PMBus
> page changes use the Write Byte Data protocol via i2c_smbus_write_byte_data().
> It appears page changes will completely bypass this check.

You have a point, I'll send a v3

> [Severity: Medium]
> Does this sequence bypass the PMBus core serialization?
> By using the private mps_lock instead of pmbus_lock(), the PMBus core could
> concurrently issue sysfs reads during these NVMEM reads.

I'll propose something for the v3.

> [Severity: Low]
> Could concurrent debugfs writes cause torn diagnostic state here?
> The last_probe_rc and last_probe_data variables are updated sequentially
> without synchronization. This might expose mismatched values to userspace
> readers in mpq8646_dbg_last_probe_show().

Yes !, v3

> [Severity: Low]
> Could this direct hardware page write desynchronize the PMBus core page cache?
> Since the core's software page cache is not updated, and the driver declares
> pages = 1, the core assumes page switches are unnecessary and will not restore
> it. This could cause sysfs telemetry to return incorrect data until reload.

OK, I did revisit it. Because the chip is for .pages = 1, the core never re-writes
PMBUS_PAGE, so a raw write of a non-zero page via this debugfs hook would
fail with the hardware on that page and the core would keep reading it as page 0
until a reload

So v3 will remove probe_page_write entirely (same rationale as dropping the
now-useless PMBUS_PAGE swallow in write_byte).

> [Severity: Medium]
> Does this multi-step unlock sequence bypass pmbus_lock()?
> The PMBus core could issue interleaved sysfs reads while the chip is unlocked
> or busy writing to NVM, potentially leading to I2C NACKs.

yep, ok.
The v3 will wrap the whole sequence with pmbus_lock()/pmbus_unlock(),
nested outside the existing mps_lock.

> [Severity: Medium]
> How is the worker restarted if a user writes to this debugfs file after
> disabling polling?
> Because the variable is exported directly without a setter function, writing
> a non-zero value updates the integer but fails to call schedule_delayed_work()
> to re-arm the worker.

right, the raw debugfs_create_u32() export cannot re-arm the worker.
I'll check it.

> [Severity: Medium]
> Does this direct hardware telemetry access also bypass pmbus_lock()?
> Without the core lock, concurrent accesses from this background worker
> could conflict with other sysfs polling.

Confirmed. The next v3 will wraps the worker's STATUS_WORD read within
pmbus_lock()/pmbus_unlock(), so it shall no longer race the
core's sysfs telemetry accesses.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3