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

Hi Bot,

> [Severity: High]
> Does this pass stack-allocated buffers to i2c_transfer()? Since many I2C
> host controllers use DMA, mapping stack memory for DMA can cause a kernel
> panic if CONFIG_VMAP_STACK is enabled. Should these buffers be dynamically
> allocated?

According to Documentation/i2c/dma-considerations.rst:
"it is NOT mandatory that the buffer of an I2C message is DMA safe

Since DMA safety is opt-in per message via I2C_M_DMA_SAFE which means that
Users of i2c_transfer() shall set the I2C_M_DMA_SAFE flag manually,
and the host drivers doing DMA get "either msg->buf for the I2C_M_DMA_SAFE case
or a bounce buffer" from i2c_get_dma_safe_msg_buf().

These messages do not set the flag, and at 1 and 2 bytes they are far below the
about 8-byte DMA thresholds the same document describes. This is also a root-only
debugfs probe helper.

-> So no.

> Will these coefficients result in the correct voltage scaling? The Direct
> format equation converts register Y to physical X using
> X = (Y * 10^-R - b) / m. With m = 64, b = 0, and R = 1, the effective LSB is
> 10^-1 / 64 = 1.5625 mV/LSB.
> 
> To match the 6.4 mV/LSB mentioned in the macro comments, would the correct
> coefficients be m = 15625 and R = -2?

My code and comment were not aligned, comments updated for v2.

> [Severity: High]
> Does returning 0 here prevent the PMBus core from automatically unlocking the
> chip? The core initialization code checks PMBUS_WRITE_PROTECT and writes 0 to
> unlock it only if it appears enabled. By faking an unlocked state, the core
> skips the unlock command, which might cause the hardware to silently ignore
> sysfs configuration writes.

ok, shall be fixed for v2

> [Severity: Medium]
> Does this sequence need to hold mps_lock during the NVM register reads? If
> an nvmem read races with a force clear or an NVM store/restore operation, it
> might hit an undocumented NVM-busy window, resulting in NACKs and silently
> returning zero-filled snapshot data.

ok, shall be fixed for v2

> [Severity: Medium]
> Does this code leak the dentry reference returned by debugfs_lookup()? If the
> directory already exists, debugfs_lookup() returns it with an incremented
> reference count, which requires a matching dput() to prevent a memory leak.

ok, shall be fixed for v2

> [Severity: Medium]
> Does this unconditionally reschedule the worker even when an IRQ is already
> configured? If client->irq is present, taking the goto rearm path will cause
> the worker to perpetually wake up and reschedule itself without performing
> any checks.

ok, shall be fixed for v2

> [Severity: High]
> Can this early return cause a use-after-free in debugfs?
> 
> Since mpq8646_debugfs_register() has already created files referencing the
> devm_kzalloc-allocated priv struct, returning -EINVAL here leaves dangling
> dentry files. If these files are read later, they will dereference the freed
> priv structure. Should mpq8646_debugfs_unregister() be called before
> returning?

ok, shall be fixed for v2

Best regards,
  Vincent

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